From 5156ac6b08a3807364b620762318dc02b93da8a5 Mon Sep 17 00:00:00 2001 From: Andrey Yegorov Date: Tue, 10 Dec 2024 14:41:18 -0800 Subject: [PATCH 1/2] Fix: recover command didn't accept rate limit parameter --- .../apache/bookkeeper/bookie/BookieShell.java | 2 +- .../bookkeeper/bookie/BookieShellTest.java | 98 +++++++++++++------ 2 files changed, 69 insertions(+), 31 deletions(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java index 00869c7fc85..b969e143d28 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java @@ -524,7 +524,7 @@ public RecoverCmd() { opts.addOption("sk", "skipOpenLedgers", false, "Skip recovering open ledgers"); opts.addOption("d", "deleteCookie", false, "Delete cookie node for the bookie."); opts.addOption("sku", "skipUnrecoverableLedgers", false, "Skip unrecoverable ledgers."); - opts.addOption("rate", "replicationRate", false, "Replication rate by bytes"); + opts.addOption("rate", "replicationRate", true, "Replication rate by bytes"); } @Override diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieShellTest.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieShellTest.java index fce3ce8bb30..d70c3988a75 100644 --- a/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieShellTest.java +++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieShellTest.java @@ -21,6 +21,7 @@ import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; @@ -35,6 +36,9 @@ import static org.mockito.Mockito.when; import com.google.common.collect.Maps; + +import java.io.ByteArrayOutputStream; +import java.io.PrintStream; import java.util.Set; import java.util.SortedMap; import java.util.function.Function; @@ -189,6 +193,14 @@ public void testRecoverCmdQuery() throws Exception { verify(admin, times(1)).close(); } + @SuppressWarnings("unchecked") + @Test + public void testRecoverLedgerWithRateLimit() throws Exception { + testRecoverCmdRecoverLedger( + 12345, false, false, false, + "-force", "-l", "12345", "-rate", "10000", "127.0.0.1:3181"); + } + @Test public void testRecoverCmdRecoverLedgerDefault() throws Exception { // default behavior @@ -235,21 +247,30 @@ void testRecoverCmdRecoverLedger(long ledgerId, boolean skipOpenLedgers, boolean removeCookies, String... args) throws Exception { - RecoverCmd cmd = (RecoverCmd) shell.commands.get("recover"); - CommandLine cmdLine = parseCommandLine(cmd, args); - assertEquals(0, cmd.runCmd(cmdLine)); - bookKeeperAdminMockedStatic.verify(() -> BookKeeperAdmin.newBookKeeperAdmin(any(ClientConfiguration.class)), - times(1)); - verify(admin, times(1)) - .recoverBookieData(eq(ledgerId), any(Set.class), eq(dryrun), eq(skipOpenLedgers)); - verify(admin, times(1)).close(); - if (removeCookies) { - MetadataDrivers.runFunctionWithRegistrationManager(any(ServerConfiguration.class), any(Function.class)); - verify(rm, times(1)).readCookie(any(BookieId.class)); - verify(rm, times(1)).removeCookie(any(BookieId.class), eq(version)); - } else { - verify(rm, times(0)).readCookie(any(BookieId.class)); - verify(rm, times(0)).removeCookie(any(BookieId.class), eq(version)); + final PrintStream oldPs = System.err; + try(ByteArrayOutputStream outContent = new ByteArrayOutputStream()) { + System.setErr(new PrintStream(outContent)); + + RecoverCmd cmd = (RecoverCmd) shell.commands.get("recover"); + CommandLine cmdLine = parseCommandLine(cmd, args); + assertEquals(0, cmd.runCmd(cmdLine)); + bookKeeperAdminMockedStatic.verify(() -> BookKeeperAdmin.newBookKeeperAdmin(any(ClientConfiguration.class)), + times(1)); + verify(admin, times(1)) + .recoverBookieData(eq(ledgerId), any(Set.class), eq(dryrun), eq(skipOpenLedgers)); + verify(admin, times(1)).close(); + if (removeCookies) { + MetadataDrivers.runFunctionWithRegistrationManager(any(ServerConfiguration.class), any(Function.class)); + verify(rm, times(1)).readCookie(any(BookieId.class)); + verify(rm, times(1)).removeCookie(any(BookieId.class), eq(version)); + } else { + verify(rm, times(0)).readCookie(any(BookieId.class)); + verify(rm, times(0)).removeCookie(any(BookieId.class), eq(version)); + } + assertFalse("invalid value for option detected:\n" + outContent, + outContent.toString().contains("invalid value for option")); + } finally { + System.setErr(oldPs); } } @@ -261,6 +282,14 @@ public void testRecoverCmdRecoverDefault() throws Exception { "-force", "127.0.0.1:3181"); } + @Test + public void testRecoverWithRateLimit() throws Exception { + // default behavior + testRecoverCmdRecover( + false, false, false, false, + "-force", "127.0.0.1:3181"); + } + @Test public void testRecoverCmdRecoverDeleteCookie() throws Exception { // dryrun @@ -307,21 +336,30 @@ void testRecoverCmdRecover(boolean dryrun, boolean removeCookies, boolean skipUnrecoverableLedgers, String... args) throws Exception { - RecoverCmd cmd = (RecoverCmd) shell.commands.get("recover"); - CommandLine cmdLine = parseCommandLine(cmd, args); - assertEquals(0, cmd.runCmd(cmdLine)); - bookKeeperAdminMockedStatic.verify(() -> BookKeeperAdmin.newBookKeeperAdmin(any(ClientConfiguration.class)), - times(1)); - verify(admin, times(1)) - .recoverBookieData(any(Set.class), eq(dryrun), eq(skipOpenLedgers), eq(skipUnrecoverableLedgers)); - verify(admin, times(1)).close(); - if (removeCookies) { - MetadataDrivers.runFunctionWithRegistrationManager(any(ServerConfiguration.class), any(Function.class)); - verify(rm, times(1)).readCookie(any(BookieId.class)); - verify(rm, times(1)).removeCookie(any(BookieId.class), eq(version)); - } else { - verify(rm, times(0)).readCookie(any(BookieId.class)); - verify(rm, times(0)).removeCookie(any(BookieId.class), eq(version)); + final PrintStream oldPs = System.err; + try(ByteArrayOutputStream outContent = new ByteArrayOutputStream()) { + System.setErr(new PrintStream(outContent)); + + RecoverCmd cmd = (RecoverCmd) shell.commands.get("recover"); + CommandLine cmdLine = parseCommandLine(cmd, args); + assertEquals(0, cmd.runCmd(cmdLine)); + bookKeeperAdminMockedStatic.verify(() -> BookKeeperAdmin.newBookKeeperAdmin(any(ClientConfiguration.class)), + times(1)); + verify(admin, times(1)) + .recoverBookieData(any(Set.class), eq(dryrun), eq(skipOpenLedgers), eq(skipUnrecoverableLedgers)); + verify(admin, times(1)).close(); + if (removeCookies) { + MetadataDrivers.runFunctionWithRegistrationManager(any(ServerConfiguration.class), any(Function.class)); + verify(rm, times(1)).readCookie(any(BookieId.class)); + verify(rm, times(1)).removeCookie(any(BookieId.class), eq(version)); + } else { + verify(rm, times(0)).readCookie(any(BookieId.class)); + verify(rm, times(0)).removeCookie(any(BookieId.class), eq(version)); + } + assertFalse("invalid value for option detected:\n" + outContent, + outContent.toString().contains("invalid value for option")); + } finally { + System.setErr(oldPs); } } From e5f41524672326e39e677845e893336391f2bc4c Mon Sep 17 00:00:00 2001 From: Andrey Yegorov Date: Tue, 10 Dec 2024 14:54:46 -0800 Subject: [PATCH 2/2] checkstyle --- .../java/org/apache/bookkeeper/bookie/BookieShellTest.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieShellTest.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieShellTest.java index d70c3988a75..a802fc4f6ce 100644 --- a/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieShellTest.java +++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieShellTest.java @@ -36,7 +36,6 @@ import static org.mockito.Mockito.when; import com.google.common.collect.Maps; - import java.io.ByteArrayOutputStream; import java.io.PrintStream; import java.util.Set; @@ -248,7 +247,7 @@ void testRecoverCmdRecoverLedger(long ledgerId, boolean removeCookies, String... args) throws Exception { final PrintStream oldPs = System.err; - try(ByteArrayOutputStream outContent = new ByteArrayOutputStream()) { + try (ByteArrayOutputStream outContent = new ByteArrayOutputStream()) { System.setErr(new PrintStream(outContent)); RecoverCmd cmd = (RecoverCmd) shell.commands.get("recover"); @@ -337,7 +336,7 @@ void testRecoverCmdRecover(boolean dryrun, boolean skipUnrecoverableLedgers, String... args) throws Exception { final PrintStream oldPs = System.err; - try(ByteArrayOutputStream outContent = new ByteArrayOutputStream()) { + try (ByteArrayOutputStream outContent = new ByteArrayOutputStream()) { System.setErr(new PrintStream(outContent)); RecoverCmd cmd = (RecoverCmd) shell.commands.get("recover");