[
https://issues.apache.org/jira/browse/BOOKKEEPER-634?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14234011#comment-14234011
]
Sijie Guo commented on BOOKKEEPER-634:
--------------------------------------
I only review UpdateLedgerOp. There are too many problems in this
implementation.
the main problem of update ledger op, the count down latch doesn't seem to wait
until completing updating all ledgers, which when it finished looping the
ledger lists, it would break the while loop and trigger count down. This
behavior doesn't seem to be right.
There are other issues:
- UpdateLedgerOp is pure client operation. Please don't add it to bookie
package. You could create a separated package o.a.b.admin and move
UpdateLedgerOp there.
- Please make UpdateLedgerMetadataCb & ReadLedgerMetadataCb static class
- Please remove empty class comments on UpdateLedgerOp
- updateBookieIdInLedgers(final String curBookieId, final String toBookieId
...) : curBookieId and toBookieId are a bit strange. Could you rename them to
'oldBookieId' and 'newBookieId'.
- please shutdown executor in updateBookieIdInLedgers
- ' final RateLimiter throttler = RateLimiter.create(batchSize);' : you are
using RateLimiter, 'batchSize' is a bit weird name. Please rename to 'rate'.
- 'final long printMessageCnt = completedLedgerCount % 30;' => please make 30
configurable. in a system that has huge number of ledgers, the verbose messages
is going to flood and useless.
- following code is problematic. you will have definitely race condition on
completedLedgerCount because checking and updating are in different threads.
what 'limit' means you need to stop updating (even issuing update operations)
if you already issued 'limit' update operations.
{code}
final long completedLedgerCount = metaCb.getCompletedLedgerCount();
+ // if verbose=true will print message on every 30
ledger
+ // updation
+ mayPrintTheProgress(completedLedgerCount, verbose);
+ if (limit != Integer.MIN_VALUE && completedLedgerCount
>= limit) {
+ latch.countDown();
+ break;
+ }
{code}
- 'changeEnsemble' is a confusing work, since we use 'change ensemble' for
ledger replacing bad bookies with new bookies. it'd better to avoid this
confusion. please rename it to 'updateEnsemble'.
- missing 'return;' in if branch, which will cause metaCb callback twice.
{code}
+ if (rc != BKException.Code.OK) {
+ // metadata update failed
+ LOG.error("Ledger {} metadata update failed. Error
code {}", new Object[] { ledgerId, rc });
+ metaCb.operationComplete(rc, null);
+ }
+ // successfully completed
+ metaCb.operationComplete(rc, null);
{code}
- UpdateLedgerCmdTest.java is actually testing UpdateLedgerOp. why not name it
'UpdateLedgerOpTest'?
BTW, could you upload your patch to review board next time you update the
patch? Since it isn't a straightforward change, it would be good to discuss
thru review board.
> Provide admin tool to rename bookie identifier in ledger metadata
> -----------------------------------------------------------------
>
> Key: BOOKKEEPER-634
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-634
> Project: Bookkeeper
> Issue Type: Sub-task
> Components: bookkeeper-client, bookkeeper-server
> Affects Versions: 4.2.1
> Reporter: Rakesh R
> Assignee: Rakesh R
> Fix For: 4.3.1
>
> Attachments: 0001-BOOKKEEPER-634-initial-draft-version.patch,
> 001-BOOKKEEPER-634-rename-bookieid-in-ledger.patch,
> 002-BOOKKEEPER-634-rename-bookieid-in-ledger.patch,
> 003-BOOKKEEPER-634-update-bookieid-in-ledger.patch,
> BOOKKEEPER-634-rename-bookieid-in-ledger.patch, BOOKKEEPER-634.patch,
> BOOKKEEPER-634.patch
>
>
> This JIRA to discuss about admin tool for changing the bookie's IP to
> hostname.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
