You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@bookkeeper.apache.org by "gaozhangmin (via GitHub)" <gi...@apache.org> on 2023/06/06 07:31:23 UTC
[GitHub] [bookkeeper] gaozhangmin opened a new pull request, #3980: Add #3979 unit test
gaozhangmin opened a new pull request, #3980:
URL: https://github.com/apache/bookkeeper/pull/3980
Add Unit test for #3979
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy commented on a diff in pull request #3980: Add #3979 unit test
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy commented on code in PR #3980:
URL: https://github.com/apache/bookkeeper/pull/3980#discussion_r1219709249
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieJournalForceTest.java:
##########
@@ -402,4 +406,70 @@ public void testFileChannelProvider() throws Exception {
verify(defaultFileChannel, times (1)).close();
}
+ @Test
+ public void testRecycleFlushedFlag() throws Exception {
+ File journalDir = tempDir.newFolder();
+ BookieImpl.checkDirectoryStructure(BookieImpl.getCurrentDirectory(journalDir));
+
+ final int journalBufferedEntriesThreshold = 10;
+
+ ServerConfiguration conf = TestBKConfiguration.newServerConfiguration();
+ conf.setJournalDirName(journalDir.getPath())
+ .setJournalBufferedEntriesThreshold(journalBufferedEntriesThreshold)
+ .setMetadataServiceUri(null)
+ .setJournalAdaptiveGroupWrites(false);
+
+ JournalChannel jc = spy(new JournalChannel(journalDir, 1));
+ whenNew(JournalChannel.class).withAnyArguments().thenReturn(jc);
+
+ LedgerDirsManager ledgerDirsManager = mock(LedgerDirsManager.class);
+ Journal journal = new Journal(0, journalDir, conf, ledgerDirsManager);
+
+ // Create a sample ForceWriteRequest object
+ long logId = 1L;
+ long lastFlushedPosition = 100L;
+ RecyclableArrayList<Journal.QueueEntry> forceWriteWaiters = new RecyclableArrayList<>();
+ boolean shouldClose = false;
+
+
+ Method createForceWriteRequest = Journal.class.getDeclaredMethod("createForceWriteRequest",
+ JournalChannel.class, long.class, long.class, RecyclableArrayList.class, boolean.class);
+ createForceWriteRequest.setAccessible(true);
+ ForceWriteRequest request = (ForceWriteRequest) createForceWriteRequest.invoke(journal, jc, logId,
+ lastFlushedPosition, forceWriteWaiters, shouldClose);
+
+ Method recycle = ForceWriteRequest.class.getDeclaredMethod("recycle");
+ recycle.setAccessible(true);
+ recycle.invoke(request);
+
+
+ request = (ForceWriteRequest) createForceWriteRequest.invoke(journal, jc, logId,
+ 200L, forceWriteWaiters, true);
+ Class<?> forceWriteRequestClass = request.getClass();
+
+ Field field = forceWriteRequestClass.getDeclaredField("logFile");
+ field.setAccessible(true);
+ assertEquals(jc, field.get(request));
+
+ field = forceWriteRequestClass.getDeclaredField("logId");
+ field.setAccessible(true);
+ assertEquals(logId, field.get(request));
+
+ field = forceWriteRequestClass.getDeclaredField("lastFlushedPosition");
+ field.setAccessible(true);
+ assertEquals(200L, field.get(request));
+
+ field = forceWriteRequestClass.getDeclaredField("forceWriteWaiters");
+ field.setAccessible(true);
+ assertEquals(forceWriteWaiters, field.get(request));
+
+ field = forceWriteRequestClass.getDeclaredField("shouldClose");
+ field.setAccessible(true);
+ assertTrue((boolean) field.get(request));
+
+ field = forceWriteRequestClass.getDeclaredField("flushed");
Review Comment:
We shouldn't use particular filed by name. If we add a new field in the future, it can't cover it.
We should cover it like the following:
```
Field[] fields = forceWriteRequestClass.getDeclaredFields();
fields.foreach(field -> {
Object v1 = field.get(newObject);
Object v2 = field.get(pooledObject);
assert(v1, v2)
});
```
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy commented on a diff in pull request #3980: Add #3979 unit test
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy commented on code in PR #3980:
URL: https://github.com/apache/bookkeeper/pull/3980#discussion_r1219709249
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieJournalForceTest.java:
##########
@@ -402,4 +406,70 @@ public void testFileChannelProvider() throws Exception {
verify(defaultFileChannel, times (1)).close();
}
+ @Test
+ public void testRecycleFlushedFlag() throws Exception {
+ File journalDir = tempDir.newFolder();
+ BookieImpl.checkDirectoryStructure(BookieImpl.getCurrentDirectory(journalDir));
+
+ final int journalBufferedEntriesThreshold = 10;
+
+ ServerConfiguration conf = TestBKConfiguration.newServerConfiguration();
+ conf.setJournalDirName(journalDir.getPath())
+ .setJournalBufferedEntriesThreshold(journalBufferedEntriesThreshold)
+ .setMetadataServiceUri(null)
+ .setJournalAdaptiveGroupWrites(false);
+
+ JournalChannel jc = spy(new JournalChannel(journalDir, 1));
+ whenNew(JournalChannel.class).withAnyArguments().thenReturn(jc);
+
+ LedgerDirsManager ledgerDirsManager = mock(LedgerDirsManager.class);
+ Journal journal = new Journal(0, journalDir, conf, ledgerDirsManager);
+
+ // Create a sample ForceWriteRequest object
+ long logId = 1L;
+ long lastFlushedPosition = 100L;
+ RecyclableArrayList<Journal.QueueEntry> forceWriteWaiters = new RecyclableArrayList<>();
+ boolean shouldClose = false;
+
+
+ Method createForceWriteRequest = Journal.class.getDeclaredMethod("createForceWriteRequest",
+ JournalChannel.class, long.class, long.class, RecyclableArrayList.class, boolean.class);
+ createForceWriteRequest.setAccessible(true);
+ ForceWriteRequest request = (ForceWriteRequest) createForceWriteRequest.invoke(journal, jc, logId,
+ lastFlushedPosition, forceWriteWaiters, shouldClose);
+
+ Method recycle = ForceWriteRequest.class.getDeclaredMethod("recycle");
+ recycle.setAccessible(true);
+ recycle.invoke(request);
+
+
+ request = (ForceWriteRequest) createForceWriteRequest.invoke(journal, jc, logId,
+ 200L, forceWriteWaiters, true);
+ Class<?> forceWriteRequestClass = request.getClass();
+
+ Field field = forceWriteRequestClass.getDeclaredField("logFile");
+ field.setAccessible(true);
+ assertEquals(jc, field.get(request));
+
+ field = forceWriteRequestClass.getDeclaredField("logId");
+ field.setAccessible(true);
+ assertEquals(logId, field.get(request));
+
+ field = forceWriteRequestClass.getDeclaredField("lastFlushedPosition");
+ field.setAccessible(true);
+ assertEquals(200L, field.get(request));
+
+ field = forceWriteRequestClass.getDeclaredField("forceWriteWaiters");
+ field.setAccessible(true);
+ assertEquals(forceWriteWaiters, field.get(request));
+
+ field = forceWriteRequestClass.getDeclaredField("shouldClose");
+ field.setAccessible(true);
+ assertTrue((boolean) field.get(request));
+
+ field = forceWriteRequestClass.getDeclaredField("flushed");
Review Comment:
We shouldn't use particular filed by name. If we add a new field in the future, it can't cover it.
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on pull request #3980: Add #3979 unit test
Posted by "gaozhangmin (via GitHub)" <gi...@apache.org>.
gaozhangmin commented on PR #3980:
URL: https://github.com/apache/bookkeeper/pull/3980#issuecomment-1578683360
rerun failure checks
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on pull request #3980: Add #3979 unit test
Posted by "gaozhangmin (via GitHub)" <gi...@apache.org>.
gaozhangmin commented on PR #3980:
URL: https://github.com/apache/bookkeeper/pull/3980#issuecomment-1579819179
> Thank for working on this.
>
> I think that the new test seems too much an "artifact":
>
> * it is full of reflective call
> * full of mocks
>
> I wonder if the test would be useful in the future.
>
> Here we are testing some values in the internal representation, if in the future we would change that data structure this test may become useless or misleading.
>
> It would be better to have a test that reproduces a problem and that fails without the fix.
>
> which is the original problem that you was ? that the Bookie didn't call "force" ? or "flush" ? We can intercept those calls and verify that they are called the expected number of times
Sure, I will add another 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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] gaozhangmin commented on pull request #3980: Add #3979 unit test
Posted by "gaozhangmin (via GitHub)" <gi...@apache.org>.
gaozhangmin commented on PR #3980:
URL: https://github.com/apache/bookkeeper/pull/3980#issuecomment-1578211858
rerun failure checks
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org