alex-rufous commented on a change in pull request #99:
URL: https://github.com/apache/qpid-broker-j/pull/99#discussion_r664695504
##########
File path:
bdbstore/src/main/java/org/apache/qpid/server/store/berkeleydb/EnvironmentFacade.java
##########
@@ -98,4 +98,6 @@
Map<String,Object> getDatabaseStatistics(String database, boolean reset);
void deleteDatabase(String databaseName);
+
+ void commitNoSync(final Transaction tx, boolean sync);
Review comment:
sync flag is redundant in methods commitNoSync and commit
the sync flag should be always 'false' for 'commitNoSync' and 'true' for
commit
##########
File path:
bdbstore/src/main/java/org/apache/qpid/server/store/berkeleydb/StandardEnvironmentFacade.java
##########
@@ -167,6 +167,11 @@ public Transaction beginTransaction(TransactionConfig
transactionConfig)
@Override
public void commit(com.sleepycat.je.Transaction tx, boolean syncCommit)
+ {
Review comment:
syncCommit parameter requires removal
##########
File path:
bdbstore/src/main/java/org/apache/qpid/server/store/berkeleydb/AbstractBDBMessageStore.java
##########
@@ -351,7 +351,7 @@ void removeMessage(long messageId, boolean sync) throws
StoreException
getLogger().debug("Deleted content for message {}",
messageId);
- getEnvironmentFacade().commit(tx, sync);
+ getEnvironmentFacade().commitNoSync(tx, sync);
Review comment:
sync parameter needs to be removed from both commitNoSync and
removeMessage.
##########
File path:
bdbstore/src/main/java/org/apache/qpid/server/store/berkeleydb/StandardEnvironmentFacade.java
##########
@@ -183,6 +188,12 @@ public void commit(com.sleepycat.je.Transaction tx,
boolean syncCommit)
_committer.commit(tx, syncCommit);
}
+ @Override
+ public void commitNoSync(final Transaction tx, boolean sync)
Review comment:
sync parameter requires removal
##########
File path:
bdbstore/src/test/java/org/apache/qpid/server/store/berkeleydb/replication/ReplicatedEnvironmentFacadeTest.java
##########
@@ -1196,6 +1202,36 @@ public void onNoMajority()
masterListener.awaitForStateChange(State.MASTER, _timeout,
TimeUnit.SECONDS));
}
+ public void testCommit() throws Exception
+ {
+ ReplicatedEnvironmentFacade master = createMaster();
+ assertEquals("Unexpected message store durability",
+ new Durability(Durability.SyncPolicy.NO_SYNC,
Durability.SyncPolicy.NO_SYNC, Durability.ReplicaAckPolicy.SIMPLE_MAJORITY),
+ master.getRealMessageStoreDurability());
+ assertEquals("Unexpected durability", TEST_DURABILITY,
master.getMessageStoreDurability());
+ assertTrue("Unexpected coalescing sync", master.isCoalescingSync());
+
+ doNothing().when(_tx).commit();
+ doNothing().when(_coalescingCommiter).commit(Mockito.any(),
Mockito.anyBoolean());
+ master.commit(_tx, false);
Review comment:
This test does not really a test commit functionality.
##########
File path:
bdbstore/src/test/java/org/apache/qpid/server/store/berkeleydb/replication/ReplicatedEnvironmentFacadeTest.java
##########
@@ -1196,6 +1202,36 @@ public void onNoMajority()
masterListener.awaitForStateChange(State.MASTER, _timeout,
TimeUnit.SECONDS));
}
+ public void testCommit() throws Exception
+ {
+ ReplicatedEnvironmentFacade master = createMaster();
+ assertEquals("Unexpected message store durability",
+ new Durability(Durability.SyncPolicy.NO_SYNC,
Durability.SyncPolicy.NO_SYNC, Durability.ReplicaAckPolicy.SIMPLE_MAJORITY),
+ master.getRealMessageStoreDurability());
+ assertEquals("Unexpected durability", TEST_DURABILITY,
master.getMessageStoreDurability());
+ assertTrue("Unexpected coalescing sync", master.isCoalescingSync());
+
+ doNothing().when(_tx).commit();
+ doNothing().when(_coalescingCommiter).commit(Mockito.any(),
Mockito.anyBoolean());
+ master.commit(_tx, false);
+
+ }
+
+ public void testCommitNoSync() throws Exception
+ {
+ ReplicatedEnvironmentFacade master = createMaster();
+ assertEquals("Unexpected message store durability",
+ new Durability(Durability.SyncPolicy.NO_SYNC,
Durability.SyncPolicy.NO_SYNC, Durability.ReplicaAckPolicy.SIMPLE_MAJORITY),
+ master.getRealMessageStoreDurability());
+ assertEquals("Unexpected durability", TEST_DURABILITY,
master.getMessageStoreDurability());
+ assertTrue("Unexpected coalescing sync", master.isCoalescingSync());
+
+ doNothing().when(_tx).commit();
+ doNothing().when(_coalescingCommiter).commit(Mockito.any(),
Mockito.anyBoolean());
+ master.commitNoSync(_tx);
Review comment:
the same as above. This test does not really a test commit functionality.
##########
File path:
bdbstore/src/main/java/org/apache/qpid/server/store/berkeleydb/replication/ReplicatedEnvironmentFacade.java
##########
@@ -346,19 +342,36 @@ public void commit(final Transaction tx, boolean
syncCommit)
}
- @Override
- public <X> ListenableFuture<X> commitAsync(final Transaction tx, final X
val)
+ public void commitInternal(final Transaction tx, final Durability
realMessageStoreDurability)
Review comment:
the visibility for the method should be private
please rename `realMessageStoreDurability` into `transactionDurability`
##########
File path:
bdbstore/src/main/java/org/apache/qpid/server/store/berkeleydb/replication/ReplicatedEnvironmentFacade.java
##########
@@ -346,19 +342,36 @@ public void commit(final Transaction tx, boolean
syncCommit)
}
- @Override
- public <X> ListenableFuture<X> commitAsync(final Transaction tx, final X
val)
+ public void commitInternal(final Transaction tx, final Durability
realMessageStoreDurability)
{
try
{
// Using commit() instead of commitNoSync() for the HA store to
allow
// the HA durability configuration to influence resulting
behaviour.
- tx.commit(_realMessageStoreDurability);
+ tx.commit(realMessageStoreDurability);
}
catch (DatabaseException de)
{
throw handleDatabaseException("Got DatabaseException on commit,
closing environment", de);
}
+ }
+
+ @Override
+ public void commitNoSync(final Transaction tx, boolean sync)
Review comment:
sync parameter has to be removed
##########
File path: tatus
##########
@@ -0,0 +1,91 @@
+[1mdiff --git
a/bdbstore/src/main/java/org/apache/qpid/server/store/berkeleydb/EnvironmentFacade.java
b/bdbstore/src/main/java/org/apache/qpid/server/store/berkeleydb/EnvironmentFacade.java[m
Review comment:
this is an unexpected file. Please delete
##########
File path:
bdbstore/src/test/java/org/apache/qpid/server/store/berkeleydb/replication/ReplicatedEnvironmentFacadeTest.java
##########
@@ -1196,6 +1202,36 @@ public void onNoMajority()
masterListener.awaitForStateChange(State.MASTER, _timeout,
TimeUnit.SECONDS));
}
+ public void testCommit() throws Exception
Review comment:
Annotation @Test is missed
##########
File path:
bdbstore/src/main/java/org/apache/qpid/server/store/berkeleydb/replication/ReplicatedEnvironmentFacade.java
##########
@@ -327,16 +332,7 @@ public Transaction beginTransaction(TransactionConfig
transactionConfig)
@Override
public void commit(final Transaction tx, boolean syncCommit)
{
- try
Review comment:
Parameter syncCommit needs to be removed
--
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]
