From ffeb697e9e57ced650f704728b4e661fae4fe266 Mon Sep 17 00:00:00 2001
From: Jean-Noel Rouvignac <jean-noel.rouvignac@forgerock.com>
Date: Wed, 03 Jul 2013 15:39:45 +0000
Subject: [PATCH] EntryHistorical.java: In encodeAndPurge(), made the code more readable by extracting methods needsPurge() and encode().
---
opends/tests/unit-tests-testng/src/server/org/opends/server/replication/plugin/HistoricalTest.java | 61 +++++--------------
opends/src/server/org/opends/server/replication/plugin/EntryHistorical.java | 117 ++++++++++++++++++---------------------
2 files changed, 71 insertions(+), 107 deletions(-)
diff --git a/opends/src/server/org/opends/server/replication/plugin/EntryHistorical.java b/opends/src/server/org/opends/server/replication/plugin/EntryHistorical.java
index bfa329f..40951b1 100644
--- a/opends/src/server/org/opends/server/replication/plugin/EntryHistorical.java
+++ b/opends/src/server/org/opends/server/replication/plugin/EntryHistorical.java
@@ -477,57 +477,46 @@
for (AttrValueHistorical attrValHist : attrHist.getValuesHistorical()
.keySet())
{
- attrValHist.getAttributeValue();
+ final AttributeValue value = attrValHist.getAttributeValue();
// Encode an attribute value
final String strValue;
if (attrValHist.getValueDeleteTime() != null)
{
- // this hist must be purged now, so skip its encoding
- if ((purgeDelayInMillisec>0) &&
- (attrValHist.getValueDeleteTime().getTime()<=purgeDate))
+ if (needsPurge(attrValHist.getValueDeleteTime(), purgeDate))
{
- this.lastPurgedValuesCount++;
+ // this hist must be purged now, so skip its encoding
continue;
}
- strValue = type.getNormalizedPrimaryName() + optionsString + ":" +
- attrValHist.getValueDeleteTime().toString() +
- ":del:" + attrValHist.getAttributeValue().toString();
- AttributeValue val = AttributeValues.create(historicalAttrType,
- strValue);
- builder.add(val);
+ strValue = encode("del", type, optionsString, attrValHist
+ .getValueDeleteTime(), value);
+ builder.add(AttributeValues.create(historicalAttrType, strValue));
}
else if (attrValHist.getValueUpdateTime() != null)
{
- if ((purgeDelayInMillisec>0) &&
- (attrValHist.getValueUpdateTime().getTime()<=purgeDate))
+ if (needsPurge(attrValHist.getValueUpdateTime(), purgeDate))
{
// this hist must be purged now, so skip its encoding
- this.lastPurgedValuesCount++;
continue;
}
- if ((delAttr && attrValHist.getValueUpdateTime() == deleteTime)
- && (attrValHist.getAttributeValue() != null))
+
+ final ChangeNumber updateTime = attrValHist.getValueUpdateTime();
+ // FIXME very suspicious use of == in the next if statement,
+ // unit tests do not like changing it
+ if (delAttr && updateTime == deleteTime && value != null)
{
- strValue = type.getNormalizedPrimaryName() + optionsString + ":" +
- attrValHist.getValueUpdateTime().toString() + ":repl:" +
- attrValHist.getAttributeValue().toString();
+ strValue = encode("repl", type, optionsString, updateTime, value);
delAttr = false;
}
+ else if (value != null)
+ {
+ strValue = encode("add", type, optionsString, updateTime, value);
+ }
else
{
- if (attrValHist.getAttributeValue() == null)
- {
- strValue = type.getNormalizedPrimaryName() + optionsString
- + ":" + attrValHist.getValueUpdateTime().toString() +
- ":add";
- }
- else
- {
- strValue = type.getNormalizedPrimaryName() + optionsString
- + ":" + attrValHist.getValueUpdateTime().toString() +
- ":add:" + attrValHist.getAttributeValue().toString();
- }
+ // "add" without any value is suspicious. Tests never go there.
+ // Is this used to encode "add" with an empty string?
+ strValue = encode("add", type, optionsString, updateTime);
}
builder.add(AttributeValues.create(historicalAttrType, strValue));
@@ -536,57 +525,59 @@
if (delAttr)
{
- // Stores the attr deletion hist when not older than the purge delay
- if ((purgeDelayInMillisec>0) &&
- (deleteTime.getTime()<=purgeDate))
+ if (needsPurge(deleteTime, purgeDate))
{
// this hist must be purged now, so skip its encoding
- this.lastPurgedValuesCount++;
continue;
}
- String strValue = type.getNormalizedPrimaryName()
- + optionsString + ":" + deleteTime.toString()
- + ":attrDel";
- AttributeValue val =
- AttributeValues.create(historicalAttrType, strValue);
- builder.add(val);
+ String strValue = encode("attrDel", type, optionsString, deleteTime);
+ builder.add(AttributeValues.create(historicalAttrType, strValue));
}
}
}
- // Encode the historical information for the ADD Operation.
- if (entryADDDate != null)
+ if (entryADDDate != null && !needsPurge(entryADDDate, purgeDate))
{
+ // Encode the historical information for the ADD Operation.
// Stores the ADDDate when not older than the purge delay
- if ((purgeDelayInMillisec>0) &&
- (entryADDDate.getTime()<=purgeDate))
- {
- this.lastPurgedValuesCount++;
- }
- else
- {
- builder.add(encodeHistorical(entryADDDate, "add"));
- }
+ builder.add(encodeHistorical(entryADDDate, "add"));
}
- // Encode the historical information for the MODDN Operation.
- if (entryMODDNDate != null)
+ if (entryMODDNDate != null && !needsPurge(entryMODDNDate, purgeDate))
{
+ // Encode the historical information for the MODDN Operation.
// Stores the MODDNDate when not older than the purge delay
- if ((purgeDelayInMillisec>0) &&
- (entryMODDNDate.getTime()<=purgeDate))
- {
- this.lastPurgedValuesCount++;
- }
- else
- {
- builder.add(encodeHistorical(entryMODDNDate, "moddn"));
- }
+ builder.add(encodeHistorical(entryMODDNDate, "moddn"));
}
return builder.toAttribute();
}
+ private boolean needsPurge(ChangeNumber cn, long purgeDate)
+ {
+ boolean needsPurge = purgeDelayInMillisec > 0 && cn.getTime() <= purgeDate;
+ if (needsPurge)
+ {
+ // this hist must be purged now, because older than the purge delay
+ this.lastPurgedValuesCount++;
+ }
+ return needsPurge;
+ }
+
+ private String encode(String operation, AttributeType type,
+ String optionsString, ChangeNumber changeTime)
+ {
+ return type.getNormalizedPrimaryName() + optionsString + ":" + changeTime
+ + ":" + operation;
+ }
+
+ private String encode(String operation, AttributeType type,
+ String optionsString, ChangeNumber changeTime, AttributeValue value)
+ {
+ return type.getNormalizedPrimaryName() + optionsString + ":" + changeTime
+ + ":" + operation + ":" + value;
+ }
+
/**
* Set the delay to purge the historical information. The purge is applied
* only when historical attribute is updated (write operations).
diff --git a/opends/tests/unit-tests-testng/src/server/org/opends/server/replication/plugin/HistoricalTest.java b/opends/tests/unit-tests-testng/src/server/org/opends/server/replication/plugin/HistoricalTest.java
index 81d17a7..9c39663 100644
--- a/opends/tests/unit-tests-testng/src/server/org/opends/server/replication/plugin/HistoricalTest.java
+++ b/opends/tests/unit-tests-testng/src/server/org/opends/server/replication/plugin/HistoricalTest.java
@@ -29,6 +29,9 @@
package org.opends.server.replication.plugin;
+import static org.opends.server.TestCaseUtils.*;
+import static org.testng.Assert.*;
+
import org.opends.server.TestCaseUtils;
import org.opends.server.core.DirectoryServer;
import org.opends.server.protocols.internal.InternalClientConnection;
@@ -41,23 +44,10 @@
import org.opends.server.replication.protocol.ModifyMsg;
import org.opends.server.replication.service.ReplicationBroker;
import org.opends.server.tools.LDAPModify;
-import org.opends.server.types.Attribute;
-import org.opends.server.types.AttributeType;
-import org.opends.server.types.Attributes;
-import org.opends.server.types.ByteString;
-import org.opends.server.types.DN;
-import org.opends.server.types.Entry;
-import org.opends.server.types.Modification;
-import org.opends.server.types.ModificationType;
-import org.opends.server.types.Operation;
-import org.opends.server.types.ResultCode;
-import org.opends.server.types.SearchResultEntry;
-import org.opends.server.types.SearchScope;
+import org.opends.server.types.*;
import org.testng.annotations.BeforeClass;
import org.testng.annotations.Test;
-import static org.testng.Assert.*;
-import static org.opends.server.TestCaseUtils.*;
import java.net.ServerSocket;
import java.util.ArrayList;
import java.util.LinkedList;
@@ -67,20 +57,19 @@
/**
* Tests the Historical class.
*/
+@SuppressWarnings("javadoc")
public class HistoricalTest
extends ReplicationTestCase
{
private int replServerPort;
- String testName = "historicalTest";
+ private String testName = "historicalTest";
/**
* Set up replication on the test backend.
- * @throws Exception If an error occurs.
*/
@BeforeClass
@Override
- public void setUp()
- throws Exception
+ public void setUp() throws Exception
{
super.setUp();
@@ -124,11 +113,9 @@
* and written to an operational attribute of the entry.
* Also test that historical is purged according to the purge delay that
* is provided.
- * @throws Exception If the test fails.
*/
@Test(enabled=true)
- public void testEncodingAndPurge()
- throws Exception
+ public void testEncodingAndPurge() throws Exception
{
// Add a test entry.
TestCaseUtils.addEntry(
@@ -202,8 +189,6 @@
assertEquals(hist.getLastPurgedValuesCount(),0);
assertEquals(after, before);
- LDAPReplicationDomain domain = MultimasterReplication.findDomain(
- DN.decode("uid=user.1," + TEST_ROOT_DN_STRING), null);
Thread.sleep(1000);
args[9] = TestCaseUtils.createTempFile(
@@ -249,11 +234,9 @@
* broker API to simulate the ordering that would happen on the first server
* on one entry, and the reverse ordering that would happen on the
* second server on a different entry. Confused yet?
- * @throws Exception If the test fails.
*/
@Test(enabled=true, groups="slow")
- public void conflictSingleValue()
- throws Exception
+ public void conflictSingleValue() throws Exception
{
final DN dn1 = DN.decode("cn=test1," + TEST_ROOT_DN_STRING);
final DN dn2 = DN.decode("cn=test2," + TEST_ROOT_DN_STRING);
@@ -382,8 +365,7 @@
{
List<Modification> mods = new ArrayList<Modification>(1);
mods.add(mod);
- ModifyMsg modMsg = new ModifyMsg(changeNum, dn, mods, entryuuid);
- broker.publish(modMsg);
+ broker.publish(new ModifyMsg(changeNum, dn, mods, entryuuid));
}
/**
@@ -504,14 +486,12 @@
String parentId = LDAPReplicationDomain.findEntryUUID(dn1.getParent());
assertTrue(addmsg.getParentEntryUUID().equals(parentId));
addmsg.createOperation(InternalClientConnection.getRootConnection());
- } else
+ }
+ else if (count == 1)
{
- if (count == 1)
- {
// The first operation should be an ADD operation.
fail("FakeAddOperation was not correctly generated"
+ " from historical information");
- }
}
}
@@ -531,18 +511,18 @@
* the time to purge everything in 1 run .. and thus to relauch it to finish
* the purge. And verify that the second run starts on the changeNumber where
* the previous task run had stopped.
- *
- * @throws Exception If the test fails.
*/
@Test(enabled=true)
- public void testRecurringPurgeIn1Run()
- throws Exception
+ public void testRecurringPurgeIn1Run() throws Exception
{
int entryCnt = 10;
addEntriesWithHistorical(1, entryCnt);
- // set the purge delay to 1 sec
+ // set the purge delay to 1 minute
+ // FIXME could we change this setting to also accept seconds?
+ // This way this test would not take one minute to run
+ // (and it could also fail less often in jenkins).
TestCaseUtils.dsconfig(
"set-replication-domain-prop",
"--provider-name","Multimaster Synchronization",
@@ -565,8 +545,6 @@
addTask(taskInit, ResultCode.SUCCESS, null);
// every entry should be purged from its hist
- try
- {
// Search for matching entries in config backend
InternalSearchOperation op = connection.processSearch(
ByteString.valueOf(TEST_ROOT_DN_STRING),
@@ -579,17 +557,12 @@
LinkedList<SearchResultEntry> entries = op.getSearchEntries();
assertTrue(entries != null);
assertEquals(entries.size(), 0);
- } catch (Exception e)
- {
- fail("assertNoConfigEntriesWithFilter: could not search config backend" + e.getMessage());
- }
}
/**
* Add a provided number of generated entries containing historical.
* @param dnSuffix A suffix to be added to the dn
* @param entryCnt The number of entries to create
- * @throws Exception
*/
private void addEntriesWithHistorical(int dnSuffix, int entryCnt)
throws Exception
--
Gitblit v1.10.0