alex-rufous commented on a change in pull request #99:
URL: https://github.com/apache/qpid-broker-j/pull/99#discussion_r667558511
##########
File path:
bdbstore/src/main/java/org/apache/qpid/server/store/berkeleydb/replication/ReplicatedEnvironmentFacade.java
##########
@@ -103,6 +104,7 @@
private static final int DEFAULT_ENVIRONMENT_RESTART_RETRY_LIMIT = 3;
private static final int DEFAULT_EXECUTOR_SHUTDOWN_TIMEOUT = 5000;
private static final boolean DEFAULT_DISABLE_COALESCING_COMMITTER = false;
+ private static final String DEFAULT_NO_SYNC_TX_DURABILITY_PROPERTY_NAME =
"NO_SYNC,NO_SYNC,NONE";
Review comment:
Dedeepya,
Do you know whether durability SYNC,NO_SYNC,NONE gives the same performance?
I think that using SYNC,NO_SYNC,NONE without coalescing committer and
NO_SYNC,NO_SYNC,NONE with a coalescing committer might be a better approach, as
a transaction would be at least synced locally...
What do you think?
##########
File path:
bdbstore/src/test/java/org/apache/qpid/server/store/berkeleydb/replication/ReplicatedEnvironmentFacadeTest.java
##########
@@ -1196,6 +1206,75 @@ public void onNoMajority()
masterListener.awaitForStateChange(State.MASTER, _timeout,
TimeUnit.SECONDS));
}
+ @Test
+ public void testNodeCommitNoSync() throws Exception
Review comment:
please move code to populate and read test db into a separate method.
Something like below
private void addTestKeyValue(Database db, Transaction txn, int key, String
value)
private String getTestKeyValue(Database db, int key)
That should improve test readability..
##########
File path:
bdbstore/src/test/java/org/apache/qpid/server/store/berkeleydb/replication/ReplicatedEnvironmentFacadeTest.java
##########
@@ -1315,10 +1406,14 @@ private ReplicatedEnvironmentFacade
addNode(StateChangeListener stateChangeListe
}
private ReplicatedEnvironmentConfiguration
createReplicatedEnvironmentConfiguration(String nodeName, String nodeHostPort,
boolean designatedPrimary)
{
- return
createReplicatedEnvironmentConfiguration(nodeName,nodeHostPort,designatedPrimary,false);
+ return createReplicatedEnvironmentConfiguration(nodeName,
nodeHostPort, designatedPrimary, false, null);
}
- private ReplicatedEnvironmentConfiguration
createReplicatedEnvironmentConfiguration(String nodeName, String nodeHostPort,
boolean designatedPrimary,final boolean disableCoalescing)
+ private ReplicatedEnvironmentConfiguration
createReplicatedEnvironmentConfiguration(String nodeName,
Review comment:
Please revert new durability parameter. I do not think we need to change
it in any test...
IMHO, it should be sufficient to set it to NO_SYNC,NO_SYNC,NONE in
createReplicatedEnvironmentConfiguration.
It is unlikely that another durability would be tested.
The changes caused by an introduction of durability parameter can be
reverted as well...
##########
File path:
bdbstore/src/test/java/org/apache/qpid/server/store/berkeleydb/replication/ReplicatedEnvironmentFacadeTest.java
##########
@@ -1196,6 +1206,75 @@ public void onNoMajority()
masterListener.awaitForStateChange(State.MASTER, _timeout,
TimeUnit.SECONDS));
}
+ @Test
+ public void testNodeCommitNoSync() throws Exception
+ {
+ DatabaseConfig createConfig = new DatabaseConfig();
+ createConfig.setAllowCreate(true);
+ createConfig.setTransactional(true);
+
+ TestStateChangeListener masterListener = new TestStateChangeListener();
+ ReplicatedEnvironmentFacade node1 = addNode(TEST_NODE_NAME,
TEST_NODE_HOST_PORT, true, masterListener, new NoopReplicationGroupListener(),
+ "NO_SYNC,NO_SYNC,NONE");
+ assertTrue("Environment was not created",
masterListener.awaitForStateChange(State.MASTER,
+
_timeout, TimeUnit.SECONDS));
+
+ String replicaNodeHostPort = "localhost:" +
_portHelper.getNextAvailable();
+ String replicaName = TEST_NODE_NAME + 1;
+ ReplicatedEnvironmentFacade node2 = createReplica(replicaName,
replicaNodeHostPort, new NoopReplicationGroupListener());
+
+ Database db = node1.openDatabase("mydb", createConfig);
+
+ // Put a record (using commitNoSync)
+ DatabaseEntry key = new DatabaseEntry();
+ DatabaseEntry data = new DatabaseEntry();
+
+ TransactionConfig transactionConfig = new TransactionConfig();
+ transactionConfig.setDurability(node1.getRealMessageStoreDurability());
+
+ Transaction txn = node1.beginTransaction(null);
+ IntegerBinding.intToEntry(1, key);
+ StringBinding.stringToEntry("value", data);
+
+ db.put(txn, key, data);
+ node1.commitNoSync(txn);
+ db.close();
+
+ // Stop node1
+ node1.close();
+ node2.close();
+
+ LOGGER.debug("RESTARTING " + TEST_NODE_NAME);
+
+ node1 = addNode(TEST_NODE_NAME, TEST_NODE_HOST_PORT, true,
masterListener, new NoopReplicationGroupListener(),
+ null);
+ boolean awaitForStateChange =
masterListener.awaitForStateChange(State.MASTER,
+
_timeout, TimeUnit.SECONDS);
+ LOGGER.debug("RESTARTING " + replicaName);
+ TestStateChangeListener node2StateChangeListener = new
TestStateChangeListener();
+ node2 = addNode(replicaName, replicaNodeHostPort, false,
node2StateChangeListener, new NoopReplicationGroupListener(),
+ null);
+ db = node1.openDatabase("mydb", DatabaseConfig.DEFAULT);
+ txn = node1.beginTransaction(transactionConfig);
Review comment:
This transaction is redundant. I think null can be passed into the get
method as a transaction. Removal transaction creation/committing for a read
operation should simplify the test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
