Copilot commented on code in PR #4640:
URL: https://github.com/apache/bookkeeper/pull/4640#discussion_r2252919469
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieStatus.java:
##########
@@ -216,6 +222,15 @@ public BookieStatus parse(BufferedReader reader)
if (status.layoutVersion == 1 && parts.length == 3) {
status.bookieMode = BookieMode.valueOf(parts[1]);
status.lastUpdateTime = Long.parseLong(parts[2].trim());
+ status.isManuallyModifiedToReadOnly = true;
Review Comment:
Setting `isManuallyModifiedToReadOnly = true` for old format (3 parts) is
incorrect. Old format indicates the state was set by disk monitoring, not
manually, so this should be `false`.
```suggestion
status.isManuallyModifiedToReadOnly = false;
```
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieInitializationTest.java:
##########
@@ -1445,30 +1452,154 @@ public void
testRetrieveBookieStatusWhenStatusFileIsCorrupted() throws Exception
.setPersistBookieStatusEnabled(true)
.setMetadataServiceUri(metadataServiceUri);
// start a new bookie
- BookieServer bookieServer = new BookieServer(
+ BookieServer bookieServer1 = new BookieServer(
conf, new TestBookieImpl(conf),
NullStatsLogger.INSTANCE, UnpooledByteBufAllocator.DEFAULT,
new MockUncleanShutdownDetection());
- bookieServer.start();
+ bookieServer1.start();
// transition in to read only and persist the status on disk
- Bookie bookie = (BookieImpl) bookieServer.getBookie();
- assertFalse(bookie.isReadOnly());
- bookie.getStateManager().transitionToReadOnlyMode().get();
- assertTrue(bookie.isReadOnly());
+ Bookie bookie1 = (BookieImpl) bookieServer1.getBookie();
+ assertFalse(bookie1.isReadOnly());
+ bookie1.getStateManager().transitionToReadOnlyMode().get();
+ assertTrue(bookie1.isReadOnly());
// corrupt status file
- List<File> ledgerDirs = ((BookieImpl)
bookie).getLedgerDirsManager().getAllLedgerDirs();
+ List<File> ledgerDirs = ((BookieImpl)
bookie1).getLedgerDirsManager().getAllLedgerDirs();
corruptFile(new File(ledgerDirs.get(0), BOOKIE_STATUS_FILENAME));
corruptFile(new File(ledgerDirs.get(1), BOOKIE_STATUS_FILENAME));
// restart the bookie should be in read only mode
- bookieServer.shutdown();
- bookieServer = new BookieServer(
+ bookieServer1.shutdown();
+ BookieServer bookieServer2 = new BookieServer(
conf, new TestBookieImpl(conf),
NullStatsLogger.INSTANCE, UnpooledByteBufAllocator.DEFAULT,
new MockUncleanShutdownDetection());
- bookieServer.start();
- bookie = bookieServer.getBookie();
- assertTrue(bookie.isReadOnly());
- bookieServer.shutdown();
+ bookieServer2.start();
+ BookieImpl bookie2 = (BookieImpl) bookieServer2.getBookie();
+ assertTrue(bookie2.isReadOnly());
+ // After a disk check, the bookie should switch to writable mode
because the disk usage is fine.
+ bookie2.dirsMonitor.check();
+ Awaitility.await().untilAsserted(() -> {
+ assertFalse(bookie2.isReadOnly());
+ });
+ bookieServer2.shutdown();
+ }
+
+
+ private void setBookieToReadOnly(Bookie bookie, boolean readOnly) throws
Exception {
+ BookieStateReadOnlyService.ReadOnlyState state = new
BookieStateReadOnlyService.ReadOnlyState();
+ state.setReadOnly(readOnly);
+ HttpServiceRequest request = new
HttpServiceRequest(JsonUtil.toJson(state), HttpServer.Method.PUT, null);
+ BookieStateReadOnlyService service = new
BookieStateReadOnlyService(bookie);
+ HttpServiceResponse response1 = service.handle(request);
+ assertEquals(HttpServer.StatusCode.OK.getValue(),
response1.getStatusCode());
+ }
+
+ /**
+ * Verify: once the state is manually modified to read-only by Admin API,
it should not be changed to writable
+ * by the monitor task.
+ * But it can be changed to read-only by monitor task if it was manually
set to writable by Admin API.
+ */
+ @Test(timeout = 1000 * 30)
+ public void
testRetrieveBookieStatusAdminModifiedWhenStatusFileIsCorrupted() throws
Exception {
+ File[] tmpLedgerDirs = new File[3];
+ String[] filePath = new String[tmpLedgerDirs.length];
+ for (int i = 0; i < tmpLedgerDirs.length; i++) {
+ tmpLedgerDirs[i] = tmpDirs.createNew("bookie", "test" + i);
+ filePath[i] = tmpLedgerDirs[i].getPath();
+ }
+ final ServerConfiguration conf =
TestBKConfiguration.newServerConfiguration();
+ conf.setJournalDirName(filePath[0])
+ .setLedgerDirNames(filePath)
+ .setReadOnlyModeEnabled(true)
+ .setPersistBookieStatusEnabled(true)
+ .setMetadataServiceUri(metadataServiceUri);
+ // start a new bookie
+ BookieServer bookieServer1 = new BookieServer(
+ conf, new TestBookieImpl(conf),
+ NullStatsLogger.INSTANCE, UnpooledByteBufAllocator.DEFAULT,
+ new MockUncleanShutdownDetection());
+ bookieServer1.start();
+ // transition in to read only and persist the status on disk
+ Bookie bookie1 = (BookieImpl) bookieServer1.getBookie();
+ assertFalse(bookie1.isReadOnly());
+ setBookieToReadOnly(bookie1, true);
+ assertTrue(bookie1.isReadOnly());
+ // corrupt status file
+ List<File> ledgerDirs = ((BookieImpl)
bookie1).getLedgerDirsManager().getAllLedgerDirs();
+ corruptFile(new File(ledgerDirs.get(0), BOOKIE_STATUS_FILENAME));
+ corruptFile(new File(ledgerDirs.get(1), BOOKIE_STATUS_FILENAME));
+ // restart the bookie should be in read only mode
+ bookieServer1.shutdown();
+ BookieServer bookieServer2 = new BookieServer(
+ conf, new TestBookieImpl(conf),
+ NullStatsLogger.INSTANCE, UnpooledByteBufAllocator.DEFAULT,
+ new MockUncleanShutdownDetection());
+ bookieServer2.start();
+ BookieImpl bookie2 = (BookieImpl) bookieServer2.getBookie();
+ assertTrue(bookie2.isReadOnly());
+ // After a disk check, the bookie should not switch to writable mode
because the state was set to read-only
+ // by admin api manually.
+ bookie2.dirsMonitor.check();
+ Thread.sleep(2000);
Review Comment:
Using `Thread.sleep()` in tests is unreliable and can cause flaky tests. The
`Awaitility.await()` call on the next line should be sufficient for waiting.
```suggestion
```
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerDirsMonitor.java:
##########
@@ -175,6 +181,14 @@ private void check(final LedgerDirsManager ldm) {
}
}
+ if (isFirstLoopOfCheckTaskLocalValue &&
ldm.getFullFilledLedgerDirs().isEmpty()) {
Review Comment:
The early return on line 189 bypasses all the existing disk monitoring logic
below. Consider moving this check after the existing monitoring logic or
documenting why the bypass is intentional.
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieInitializationTest.java:
##########
@@ -1445,30 +1452,154 @@ public void
testRetrieveBookieStatusWhenStatusFileIsCorrupted() throws Exception
.setPersistBookieStatusEnabled(true)
.setMetadataServiceUri(metadataServiceUri);
// start a new bookie
- BookieServer bookieServer = new BookieServer(
+ BookieServer bookieServer1 = new BookieServer(
conf, new TestBookieImpl(conf),
NullStatsLogger.INSTANCE, UnpooledByteBufAllocator.DEFAULT,
new MockUncleanShutdownDetection());
- bookieServer.start();
+ bookieServer1.start();
// transition in to read only and persist the status on disk
- Bookie bookie = (BookieImpl) bookieServer.getBookie();
- assertFalse(bookie.isReadOnly());
- bookie.getStateManager().transitionToReadOnlyMode().get();
- assertTrue(bookie.isReadOnly());
+ Bookie bookie1 = (BookieImpl) bookieServer1.getBookie();
+ assertFalse(bookie1.isReadOnly());
+ bookie1.getStateManager().transitionToReadOnlyMode().get();
+ assertTrue(bookie1.isReadOnly());
// corrupt status file
- List<File> ledgerDirs = ((BookieImpl)
bookie).getLedgerDirsManager().getAllLedgerDirs();
+ List<File> ledgerDirs = ((BookieImpl)
bookie1).getLedgerDirsManager().getAllLedgerDirs();
corruptFile(new File(ledgerDirs.get(0), BOOKIE_STATUS_FILENAME));
corruptFile(new File(ledgerDirs.get(1), BOOKIE_STATUS_FILENAME));
// restart the bookie should be in read only mode
- bookieServer.shutdown();
- bookieServer = new BookieServer(
+ bookieServer1.shutdown();
+ BookieServer bookieServer2 = new BookieServer(
conf, new TestBookieImpl(conf),
NullStatsLogger.INSTANCE, UnpooledByteBufAllocator.DEFAULT,
new MockUncleanShutdownDetection());
- bookieServer.start();
- bookie = bookieServer.getBookie();
- assertTrue(bookie.isReadOnly());
- bookieServer.shutdown();
+ bookieServer2.start();
+ BookieImpl bookie2 = (BookieImpl) bookieServer2.getBookie();
+ assertTrue(bookie2.isReadOnly());
+ // After a disk check, the bookie should switch to writable mode
because the disk usage is fine.
+ bookie2.dirsMonitor.check();
+ Awaitility.await().untilAsserted(() -> {
+ assertFalse(bookie2.isReadOnly());
+ });
+ bookieServer2.shutdown();
+ }
+
+
+ private void setBookieToReadOnly(Bookie bookie, boolean readOnly) throws
Exception {
+ BookieStateReadOnlyService.ReadOnlyState state = new
BookieStateReadOnlyService.ReadOnlyState();
+ state.setReadOnly(readOnly);
+ HttpServiceRequest request = new
HttpServiceRequest(JsonUtil.toJson(state), HttpServer.Method.PUT, null);
+ BookieStateReadOnlyService service = new
BookieStateReadOnlyService(bookie);
+ HttpServiceResponse response1 = service.handle(request);
+ assertEquals(HttpServer.StatusCode.OK.getValue(),
response1.getStatusCode());
+ }
+
+ /**
+ * Verify: once the state is manually modified to read-only by Admin API,
it should not be changed to writable
+ * by the monitor task.
+ * But it can be changed to read-only by monitor task if it was manually
set to writable by Admin API.
+ */
+ @Test(timeout = 1000 * 30)
+ public void
testRetrieveBookieStatusAdminModifiedWhenStatusFileIsCorrupted() throws
Exception {
+ File[] tmpLedgerDirs = new File[3];
+ String[] filePath = new String[tmpLedgerDirs.length];
+ for (int i = 0; i < tmpLedgerDirs.length; i++) {
+ tmpLedgerDirs[i] = tmpDirs.createNew("bookie", "test" + i);
+ filePath[i] = tmpLedgerDirs[i].getPath();
+ }
+ final ServerConfiguration conf =
TestBKConfiguration.newServerConfiguration();
+ conf.setJournalDirName(filePath[0])
+ .setLedgerDirNames(filePath)
+ .setReadOnlyModeEnabled(true)
+ .setPersistBookieStatusEnabled(true)
+ .setMetadataServiceUri(metadataServiceUri);
+ // start a new bookie
+ BookieServer bookieServer1 = new BookieServer(
+ conf, new TestBookieImpl(conf),
+ NullStatsLogger.INSTANCE, UnpooledByteBufAllocator.DEFAULT,
+ new MockUncleanShutdownDetection());
+ bookieServer1.start();
+ // transition in to read only and persist the status on disk
+ Bookie bookie1 = (BookieImpl) bookieServer1.getBookie();
+ assertFalse(bookie1.isReadOnly());
+ setBookieToReadOnly(bookie1, true);
+ assertTrue(bookie1.isReadOnly());
+ // corrupt status file
+ List<File> ledgerDirs = ((BookieImpl)
bookie1).getLedgerDirsManager().getAllLedgerDirs();
+ corruptFile(new File(ledgerDirs.get(0), BOOKIE_STATUS_FILENAME));
+ corruptFile(new File(ledgerDirs.get(1), BOOKIE_STATUS_FILENAME));
+ // restart the bookie should be in read only mode
+ bookieServer1.shutdown();
+ BookieServer bookieServer2 = new BookieServer(
+ conf, new TestBookieImpl(conf),
+ NullStatsLogger.INSTANCE, UnpooledByteBufAllocator.DEFAULT,
+ new MockUncleanShutdownDetection());
+ bookieServer2.start();
+ BookieImpl bookie2 = (BookieImpl) bookieServer2.getBookie();
+ assertTrue(bookie2.isReadOnly());
+ // After a disk check, the bookie should not switch to writable mode
because the state was set to read-only
+ // by admin api manually.
+ bookie2.dirsMonitor.check();
+ Thread.sleep(2000);
+ Awaitility.await().untilAsserted(() -> {
+ assertTrue(bookie2.isReadOnly());
+ });
+ // We can use Admin Api to change the state back to writable.
+ setBookieToReadOnly(bookie2, false);
+ assertFalse(bookie2.isReadOnly());
+ // The state can be changed to read-only by monitor task if it was
manually set to writable by Admin API.
+ bookie2.getStateManager().transitionToReadOnlyMode().get();
+ assertTrue(bookie2.isReadOnly());
+ bookieServer2.shutdown();
+ }
+
+ /**
+ * Verify: the newest version can read the old version payload of
persisted bookie status.
+ * old payload: "1,READ_ONLY,1752809349613"
+ * new payload: "1,READ_ONLY,1752809349613,false"
+ */
+ @Test(timeout = 10000)
+ public void testPersistBookieStatusCompatibility() throws Exception {
+ File[] tmpLedgerDirs = new File[1];
+ String[] filePath = new String[1];
+ tmpLedgerDirs[0] = tmpDirs.createNew("bookie", "test");
+ filePath[0] = tmpLedgerDirs[0].getPath();
+ final ServerConfiguration conf =
TestBKConfiguration.newServerConfiguration();
+ conf.setJournalDirName(filePath[0])
+ .setLedgerDirNames(filePath)
+ .setReadOnlyModeEnabled(true)
+ .setPersistBookieStatusEnabled(true)
+ .setMetadataServiceUri(metadataServiceUri);
+ // start a new bookie
+ BookieServer bookieServer1 = new BookieServer(
+ conf, new TestBookieImpl(conf),
+ NullStatsLogger.INSTANCE, UnpooledByteBufAllocator.DEFAULT,
+ new MockUncleanShutdownDetection());
+ bookieServer1.start();
+ // transition in to read only and persist the status on disk
+ Bookie bookie1 = (BookieImpl) bookieServer1.getBookie();
+ assertFalse(bookie1.isReadOnly());
+ bookie1.getStateManager().transitionToReadOnlyMode().get();
+ assertTrue(bookie1.isReadOnly());
+ // Rewrite file to the old payload.
+ List<File> ledgerDirs = ((BookieImpl)
bookie1).getLedgerDirsManager().getAllLedgerDirs();
+ FileUtils.writeStringToFile(new File(ledgerDirs.get(0),
BOOKIE_STATUS_FILENAME),
+ "1,READ_ONLY,1752809349613");
+ // restart the bookie should be in read only mode
+ bookieServer1.shutdown();
+ BookieServer bookieServer2 = new BookieServer(
+ conf, new TestBookieImpl(conf),
+ NullStatsLogger.INSTANCE, UnpooledByteBufAllocator.DEFAULT,
+ new MockUncleanShutdownDetection());
+ bookieServer2.start();
+ BookieImpl bookie2 = (BookieImpl) bookieServer2.getBookie();
+ assertTrue(bookie2.isReadOnly());
+ // After a disk check, the bookie should not switch to writable mode
because the state was set to read-only
+ // by admin api manually.
Review Comment:
The comment says the bookie should NOT switch to writable mode, but the test
assertion on line 1600 expects it to switch to writable mode
(`assertFalse(bookie2.isReadOnly())`). The comment should say 'should switch to
writable mode'.
```suggestion
// After a disk check, the bookie should switch to writable mode
because the persisted state is compatible
// and the disk is healthy.
```
--
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]
