captain-hoity-toity commented on code in PR #4032: URL: https://github.com/apache/bookkeeper/pull/4032#discussion_r1325274292
########## bookkeeper-server/src/main/java/org/apache/bookkeeper/tools/cli/commands/autorecovery/MarkLedgerReplicatedCommand.java: ########## @@ -0,0 +1,135 @@ +/* Review Comment: The code is generally well-written and follows good practices. However, there are a few points that could be improved: 1. Exception Handling: You are throwing `UncheckedExecutionException` for almost all types of exceptions. It would be better to handle different exceptions separately and provide more specific messages for each one. 2. Method `markLedgerReplicated()`: It is doing multiple operations like getting user confirmation, marking ledger replicated etc. Consider breaking it down into multiple smaller methods for better readability and maintainability. 3. Method `initLedgerIdFormatter()`: The if-else conditions can be simplified. You can initialize `ledgerIdFormatter` to a default value and only change it if `flags.ledgerIdFormatter` is not equal to `DEFAULT`. 4. Magic Number: Avoid using magic numbers in your code (like -1 in `flags.ledgerId < 0`). Instead, define them as constants with meaningful names. 5. `LedgerIdFormatter` initialization: The `initLedgerIdFormatter()` method is called from `apply()`, and then `markLedgerReplicated()` is called. It's not clear why the `LedgerIdFormatter` can't be initialized directly in the `markLedgerReplicated()` method. 6. Commenting: Comments explaining the purpose of each method will make the code easier to understand for other developers. 7. Use of `force` flag: The `force` flag is used to bypass the user confirmation. This could be risky if used unintentionally. It might be good to add a warning or additional confirmation when `force` is used. 8. Variable naming: The variable `confirm` is not very descriptive. Consider renaming it to something like `userConfirmed`. -- 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]
