You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@bookkeeper.apache.org by Rakesh R <ra...@huawei.com> on 2014/12/09 05:05:08 UTC
Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/
-----------------------------------------------------------
Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
Bugs: BOKKEEPER-634
https://issues.apache.org/jira/browse/BOKKEEPER-634
Repository: bookkeeper-git
Description
-------
admin tool for changing the bookie's identifier to IP/hostname
Diffs
-----
bookkeeper-server/src/main/java/org/apache/bookkeeper/admin/UpdateLedgerOp.java PRE-CREATION
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java 49d8e59
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/28835/diff/
Testing
-------
Added few tests
Thanks,
Rakesh R
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Rakesh R <ra...@huawei.com>.
> On Dec. 9, 2014, 12:08 p.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/admin/UpdateLedgerOp.java, line 82
> > <https://reviews.apache.org/r/28835/diff/1/?file=786287#file786287line82>
> >
> > Latch is not needed. The executor will return a future. just call .get() on it to get the same behaviour as the latch.
>
> Rakesh R wrote:
> yes. I will do it.
>
> Rakesh R wrote:
> I feel latch is simple to control the outerloop. What do you say?
>
> Ivan Kelly wrote:
> but it's unnecessary. submit() returns a future. you just need to call .get() on that to have the exact same effect.
oops, I missed it. Will do:)
> On Dec. 9, 2014, 12:08 p.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java, line 1199
> > <https://reviews.apache.org/r/28835/diff/1/?file=786288#file786288line1199>
> >
> > what does 0 mean with regards to printing? should be (default false).
>
> Rakesh R wrote:
> Here I am taking option value as 'printMsgCount', using this I'm calculate the printing cycle. For example the value 100 means. After every 100, 200, 300... ledgers given for updation will print the message.
>
> Ivan Kelly wrote:
> This isn't clear to the user. Perhaps it would also be better to print every 5 seconds or so, rather than every X updates. Really what we want to show to the user is that the process is still working.
>
> Rakesh R wrote:
> If there are millions of ledgers, would be too noisy. Initially I had done periodically,but modified later based on Sijie's concerns.
> You want me to introduce one more parameter instead of reading the optionvalue from verbose, like 'printmessageon' counter value and print based on that?
>
> Ivan Kelly wrote:
> I'd definitely make verbose and the period a separate parameter. -v, --verbose are pretty standard parameters in command line tools. They normally never take an argument. I'd either rename 'verbose' to 'printprogress' or make 'verbose' turn output on/off and then use 'printprogress' to specify how much. I prefer the first option, because you generally want some output at least.
>
> Also, I don't think printing every 5 seconds is noisy. Noise is when there's too much info to read any of it. One line every 5 seconds is easy to handle. And it gives the user a hint that the process is progressing.
I agree to provide verbose option [on/off] and introduce one more option 'printprogress'. This will have the oprion valie to print the frequency of the message.
Since Sijie had given a comment earlier, it would be really great if Sijie can give a reply. Thanks!
Sijie, do you agree to print messages on every configured seconds?
- Rakesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/#review64360
-----------------------------------------------------------
On Dec. 16, 2014, 4:07 a.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28835/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2014, 4:07 a.m.)
>
>
> Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
>
>
> Bugs: BOKKEEPER-634
> https://issues.apache.org/jira/browse/BOKKEEPER-634
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> admin tool for changing the bookie's identifier to IP/hostname
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28835/diff/
>
>
> Testing
> -------
>
> Added few tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Rakesh R <ra...@huawei.com>.
> On Dec. 9, 2014, 12:08 p.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/admin/UpdateLedgerOp.java, line 81
> > <https://reviews.apache.org/r/28835/diff/1/?file=786287#file786287line81>
> >
> > Passing in stop as a variable makes the flow of operation unclear. In the while loop that its used in, its unclear as to what 'stop' relates to. Who sets it?
> >
> > It would be better to have a hasErrorOccurred() method in UpdateLedgerMetadataCallback and call that instead of !stop.get().
Am using stop to signal any errors when there is an exception during ack verification. UpdateLedgerMetadataCallback has been removed now.
> On Dec. 9, 2014, 12:08 p.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/admin/UpdateLedgerOp.java, line 82
> > <https://reviews.apache.org/r/28835/diff/1/?file=786287#file786287line82>
> >
> > Latch is not needed. The executor will return a future. just call .get() on it to get the same behaviour as the latch.
>
> Rakesh R wrote:
> yes. I will do it.
I feel latch is simple to control the outerloop. What do you say?
> On Dec. 9, 2014, 12:08 p.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/admin/UpdateLedgerOp.java, line 83
> > <https://reviews.apache.org/r/28835/diff/1/?file=786287#file786287line83>
> >
> > updateCb would be better than metaCb. there's a lot of metadata ops happening.
> >
> > Also, create it inside #run(). It doesn't need to be this far up in the scope.
>
> Rakesh R wrote:
> ok, i will rename it. I think I need to keep it outside #run() as it will wait for the completion like:
>
> long ledgerUpdatedCount = updateCb.get();
>
> isn't it?
>
> Ivan Kelly wrote:
> No, it the Future returned from submit that you have to all #get() on.
This logic has been modified by removing updateCb object.
> On Dec. 9, 2014, 12:08 p.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/admin/UpdateLedgerOp.java, line 125
> > <https://reviews.apache.org/r/28835/diff/1/?file=786287#file786287line125>
> >
> > It's a little ugly to mix business logic and presentation. I think it would be better to pass in a notifier/callback type like.
> >
> > interface BlahNotifier {
> > void progress(long updated, long issued);
> > }
> >
> > And then have BookieShell pass in an implementaiton of this.
>
> Rakesh R wrote:
> ok. let me try.
Provided UpdateLedgerNotifier interface in BookieShell
> On Dec. 9, 2014, 12:08 p.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/admin/UpdateLedgerOp.java, line 140
> > <https://reviews.apache.org/r/28835/diff/1/?file=786287#file786287line140>
> >
> > There's a lot of modification from afar going here around ledgersWaitForCompletion. Why do you need to countDown() the latch and to remove() it from the list? Really you only want to track how many requests have been started, and completed.
> >
> > It would be cleaner to allocate the tracker object in main #run thread, and pass it in at that point. That way, when we wait on it, its clear that we're waiting on something we have passed into the request. In fact, why not make ReadLedgerMetadataCb extends AbstractFuture<LedgerMetadata>? Then you can build a list of ReadLedgerMetadataCb objects in the run loop, and wait on all futures at the end. This will also handle the erroring, as you can make the future throw an error to tell you to stop.
>
> Rakesh R wrote:
> The idea for the 'ledgersWaitForCompletion' is after iterating over all the ledgers, will wait for the items which are yet to be completed. For example, assume there are 100,000 ledgers. It will be iterate and call the ledgers based on the ratelimitting factor. During the iteration time most of the ledgers might be completed their updations. Now, at the end of the iteration it will be have only few ledgers which are waiting for the updates. So this thread needs to wait for only those ledgers.
>
>
> Say, if there are many ledgers in the system, adding ReadLedgerMetadataCb objects into the list will create problem, sin't it? I'm afraid of keeping all the ledgers callback objects in memory. Thats the reason I thought of removing immedidately on success updation so that the list will shrink.
>
> Ivan Kelly wrote:
> I meant something like https://github.com/ivankelly/bookkeeper/commit/39cbacc582e661561fd19c978e38ac9bf4ac7b84
>
> I haven't tried to compile that, it's just to give an idea, but it would allow UpdateLedgerMetadataCb to be removed completely, and all the flow logic fits into one screen.
UpdateLedgerMetadataCb logic has been removed completely in the new patch.
> On Dec. 9, 2014, 12:08 p.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java, line 1199
> > <https://reviews.apache.org/r/28835/diff/1/?file=786288#file786288line1199>
> >
> > what does 0 mean with regards to printing? should be (default false).
>
> Rakesh R wrote:
> Here I am taking option value as 'printMsgCount', using this I'm calculate the printing cycle. For example the value 100 means. After every 100, 200, 300... ledgers given for updation will print the message.
>
> Ivan Kelly wrote:
> This isn't clear to the user. Perhaps it would also be better to print every 5 seconds or so, rather than every X updates. Really what we want to show to the user is that the process is still working.
If there are millions of ledgers, would be too noisy. Initially I had done periodically,but modified later based on Sijie's concerns.
You want me to introduce one more parameter instead of reading the optionvalue from verbose, like 'printmessageon' counter value and print based on that?
- Rakesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/#review64360
-----------------------------------------------------------
On Dec. 9, 2014, 4:05 a.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28835/
> -----------------------------------------------------------
>
> (Updated Dec. 9, 2014, 4:05 a.m.)
>
>
> Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
>
>
> Bugs: BOKKEEPER-634
> https://issues.apache.org/jira/browse/BOKKEEPER-634
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> admin tool for changing the bookie's identifier to IP/hostname
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/admin/UpdateLedgerOp.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java 49d8e59
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28835/diff/
>
>
> Testing
> -------
>
> Added few tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Rakesh R <ra...@huawei.com>.
> On Dec. 9, 2014, 12:08 p.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/admin/UpdateLedgerOp.java, line 82
> > <https://reviews.apache.org/r/28835/diff/1/?file=786287#file786287line82>
> >
> > Latch is not needed. The executor will return a future. just call .get() on it to get the same behaviour as the latch.
>
> Rakesh R wrote:
> yes. I will do it.
>
> Rakesh R wrote:
> I feel latch is simple to control the outerloop. What do you say?
>
> Ivan Kelly wrote:
> but it's unnecessary. submit() returns a future. you just need to call .get() on that to have the exact same effect.
>
> Rakesh R wrote:
> oops, I missed it. Will do:)
Hi Ivan, I'm reopening this comment. In the latest patch I've used latch to control syncing up.
I'm using FutureCallback to update the updatedLedgerCount. Now the updation of each ledger will be completed only after the futurecallback execution. When I tested I've observed that #get() api is coming out before invoking its futureCallback, this may give wrong log messages. For accuracy all outstanding ledgers needs to wait till the callback completion.
Do you agree to use latch way of controlling the sequence?
- Rakesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/#review64360
-----------------------------------------------------------
On Jan. 1, 2015, 7:43 p.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28835/
> -----------------------------------------------------------
>
> (Updated Jan. 1, 2015, 7:43 p.m.)
>
>
> Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
>
>
> Bugs: BOKKEEPER-634
> https://issues.apache.org/jira/browse/BOKKEEPER-634
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> admin tool for changing the bookie's identifier to IP/hostname
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerCmdTest.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28835/diff/
>
>
> Testing
> -------
>
> Added few tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Sijie Guo <gu...@gmail.com>.
> On Dec. 9, 2014, 12:08 p.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java, line 1199
> > <https://reviews.apache.org/r/28835/diff/1/?file=786288#file786288line1199>
> >
> > what does 0 mean with regards to printing? should be (default false).
>
> Rakesh R wrote:
> Here I am taking option value as 'printMsgCount', using this I'm calculate the printing cycle. For example the value 100 means. After every 100, 200, 300... ledgers given for updation will print the message.
>
> Ivan Kelly wrote:
> This isn't clear to the user. Perhaps it would also be better to print every 5 seconds or so, rather than every X updates. Really what we want to show to the user is that the process is still working.
>
> Rakesh R wrote:
> If there are millions of ledgers, would be too noisy. Initially I had done periodically,but modified later based on Sijie's concerns.
> You want me to introduce one more parameter instead of reading the optionvalue from verbose, like 'printmessageon' counter value and print based on that?
>
> Ivan Kelly wrote:
> I'd definitely make verbose and the period a separate parameter. -v, --verbose are pretty standard parameters in command line tools. They normally never take an argument. I'd either rename 'verbose' to 'printprogress' or make 'verbose' turn output on/off and then use 'printprogress' to specify how much. I prefer the first option, because you generally want some output at least.
>
> Also, I don't think printing every 5 seconds is noisy. Noise is when there's too much info to read any of it. One line every 5 seconds is easy to handle. And it gives the user a hint that the process is progressing.
>
> Rakesh R wrote:
> I agree to provide verbose option [on/off] and introduce one more option 'printprogress'. This will have the oprion valie to print the frequency of the message.
>
> Since Sijie had given a comment earlier, it would be really great if Sijie can give a reply. Thanks!
> Sijie, do you agree to print messages on every configured seconds?
I am fine with seconds, but it should be able to configure
- Sijie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/#review64360
-----------------------------------------------------------
On Dec. 16, 2014, 4:07 a.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28835/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2014, 4:07 a.m.)
>
>
> Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
>
>
> Bugs: BOKKEEPER-634
> https://issues.apache.org/jira/browse/BOKKEEPER-634
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> admin tool for changing the bookie's identifier to IP/hostname
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28835/diff/
>
>
> Testing
> -------
>
> Added few tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Rakesh R <ra...@huawei.com>.
> On Dec. 9, 2014, 12:08 p.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/admin/UpdateLedgerOp.java, line 82
> > <https://reviews.apache.org/r/28835/diff/1/?file=786287#file786287line82>
> >
> > Latch is not needed. The executor will return a future. just call .get() on it to get the same behaviour as the latch.
>
> Rakesh R wrote:
> yes. I will do it.
>
> Rakesh R wrote:
> I feel latch is simple to control the outerloop. What do you say?
>
> Ivan Kelly wrote:
> but it's unnecessary. submit() returns a future. you just need to call .get() on that to have the exact same effect.
>
> Rakesh R wrote:
> oops, I missed it. Will do:)
>
> Rakesh R wrote:
> Hi Ivan, I'm reopening this comment. In the latest patch I've used latch to control syncing up.
>
> I'm using FutureCallback to update the updatedLedgerCount. Now the updation of each ledger will be completed only after the futurecallback execution. When I tested I've observed that #get() api is coming out before invoking its futureCallback, this may give wrong log messages. For accuracy all outstanding ledgers needs to wait till the callback completion.
>
> Do you agree to use latch way of controlling the sequence?
>
> Ivan Kelly wrote:
> I don't understand the comment. Do you mean that submit(new Runnable(){...}).get() is returning before all callbacks have been completed? If this is the case, then latch doesn't solve it. The latch, as it is now, is functionally equivalent to calling get(). Timing may be different though. If it doesn't run correctly using the get() then there's probably a race condition lurking somewhere.
It seems my explanation was bad:(
>>>>>>>>>>The latch, as it is now, is functionally equivalent to calling get()
Latch is used to see all the outstandings future#callback() execution is completed rather than outstandings future#get()
Let me try to add more explanation. In the latest patch, it will register callback to the futures and this callback will be run when the Future's computation is complete. Callback has the logic to update the status of each ledger updation(success or failure).
Algorithm:
----------
1) Set syncObj; // outer
<Begin: iterate and for each ledger do>
1.1) Acquire thrttle
1.2)Creates ReadLedgerMetadataCb
1.3) Adds the respective ledgerid to 'outstandings'
1.4) Register Callback to update the status(success or failure) of the future. Also, callback has the logic to notify 'syncObj'
1.5) Update issuedLedgerCnt
1.6) Check & set the stop flag=true, if limit reaches
1.7) Call updateLedger operation
<End:>
2) syncObj.await() -> syncObj will be notified only after all the issued ledgers finishes executing the registered callback.
On callback, it will remove the respective ledgerid from 'outstandings' and updatedLedgerCnt#incr(). Finally, Callback will check whether outstandings#isEmpty() && stop==true, if yes notifies syncObj.
Initially I tried using ReadLedgerMetadataCb#get() to see there is no outstandings. But I've noticied, this call comes out before the callback execution finishes and is giving wrong 'updatedLedgerCnt' value. I meant, return of ReadLedgerMetadataCb#get() is not guarantees that its registered callback is triggered and executed. Thats the reason I tried using separate 'latch' to mark the completion of all the issued ledgers. Does this makes sense to you?
- Rakesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/#review64360
-----------------------------------------------------------
On Jan. 1, 2015, 7:43 p.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28835/
> -----------------------------------------------------------
>
> (Updated Jan. 1, 2015, 7:43 p.m.)
>
>
> Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
>
>
> Bugs: BOKKEEPER-634
> https://issues.apache.org/jira/browse/BOKKEEPER-634
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> admin tool for changing the bookie's identifier to IP/hostname
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerCmdTest.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28835/diff/
>
>
> Testing
> -------
>
> Added few tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Rakesh R <ra...@huawei.com>.
> On Dec. 9, 2014, 12:08 p.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/admin/UpdateLedgerOp.java, line 140
> > <https://reviews.apache.org/r/28835/diff/1/?file=786287#file786287line140>
> >
> > There's a lot of modification from afar going here around ledgersWaitForCompletion. Why do you need to countDown() the latch and to remove() it from the list? Really you only want to track how many requests have been started, and completed.
> >
> > It would be cleaner to allocate the tracker object in main #run thread, and pass it in at that point. That way, when we wait on it, its clear that we're waiting on something we have passed into the request. In fact, why not make ReadLedgerMetadataCb extends AbstractFuture<LedgerMetadata>? Then you can build a list of ReadLedgerMetadataCb objects in the run loop, and wait on all futures at the end. This will also handle the erroring, as you can make the future throw an error to tell you to stop.
The idea for the 'ledgersWaitForCompletion' is after iterating over all the ledgers, will wait for the items which are yet to be completed. For example, assume there are 100,000 ledgers. It will be iterate and call the ledgers based on the ratelimitting factor. During the iteration time most of the ledgers might be completed their updations. Now, at the end of the iteration it will be have only few ledgers which are waiting for the updates. So this thread needs to wait for only those ledgers.
Say, if there are many ledgers in the system, adding ReadLedgerMetadataCb objects into the list will create problem, sin't it? I'm afraid of keeping all the ledgers callback objects in memory. Thats the reason I thought of removing immedidately on success updation so that the list will shrink.
> On Dec. 9, 2014, 12:08 p.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java, line 1199
> > <https://reviews.apache.org/r/28835/diff/1/?file=786288#file786288line1199>
> >
> > what does 0 mean with regards to printing? should be (default false).
Here I am taking option value as 'printMsgCount', using this I'm calculate the printing cycle. For example the value 100 means. After every 100, 200, 300... ledgers given for updation will print the message.
> On Dec. 9, 2014, 12:08 p.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java, line 334
> > <https://reviews.apache.org/r/28835/diff/1/?file=786289#file786289line334>
> >
> > Why make this public rather than moving UpdateLedgerOp into client package?
yes, I will do.
- Rakesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/#review64360
-----------------------------------------------------------
On Dec. 9, 2014, 4:05 a.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28835/
> -----------------------------------------------------------
>
> (Updated Dec. 9, 2014, 4:05 a.m.)
>
>
> Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
>
>
> Bugs: BOKKEEPER-634
> https://issues.apache.org/jira/browse/BOKKEEPER-634
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> admin tool for changing the bookie's identifier to IP/hostname
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/admin/UpdateLedgerOp.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java 49d8e59
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28835/diff/
>
>
> Testing
> -------
>
> Added few tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Ivan Kelly <iv...@apache.org>.
> On Dec. 9, 2014, 12:08 p.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/admin/UpdateLedgerOp.java, line 82
> > <https://reviews.apache.org/r/28835/diff/1/?file=786287#file786287line82>
> >
> > Latch is not needed. The executor will return a future. just call .get() on it to get the same behaviour as the latch.
>
> Rakesh R wrote:
> yes. I will do it.
>
> Rakesh R wrote:
> I feel latch is simple to control the outerloop. What do you say?
>
> Ivan Kelly wrote:
> but it's unnecessary. submit() returns a future. you just need to call .get() on that to have the exact same effect.
>
> Rakesh R wrote:
> oops, I missed it. Will do:)
>
> Rakesh R wrote:
> Hi Ivan, I'm reopening this comment. In the latest patch I've used latch to control syncing up.
>
> I'm using FutureCallback to update the updatedLedgerCount. Now the updation of each ledger will be completed only after the futurecallback execution. When I tested I've observed that #get() api is coming out before invoking its futureCallback, this may give wrong log messages. For accuracy all outstanding ledgers needs to wait till the callback completion.
>
> Do you agree to use latch way of controlling the sequence?
I don't understand the comment. Do you mean that submit(new Runnable(){...}).get() is returning before all callbacks have been completed? If this is the case, then latch doesn't solve it. The latch, as it is now, is functionally equivalent to calling get(). Timing may be different though. If it doesn't run correctly using the get() then there's probably a race condition lurking somewhere.
- Ivan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/#review64360
-----------------------------------------------------------
On Jan. 1, 2015, 7:43 p.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28835/
> -----------------------------------------------------------
>
> (Updated Jan. 1, 2015, 7:43 p.m.)
>
>
> Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
>
>
> Bugs: BOKKEEPER-634
> https://issues.apache.org/jira/browse/BOKKEEPER-634
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> admin tool for changing the bookie's identifier to IP/hostname
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerCmdTest.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28835/diff/
>
>
> Testing
> -------
>
> Added few tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Ivan Kelly <iv...@apache.org>.
> On Dec. 9, 2014, 12:08 p.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/admin/UpdateLedgerOp.java, line 82
> > <https://reviews.apache.org/r/28835/diff/1/?file=786287#file786287line82>
> >
> > Latch is not needed. The executor will return a future. just call .get() on it to get the same behaviour as the latch.
>
> Rakesh R wrote:
> yes. I will do it.
>
> Rakesh R wrote:
> I feel latch is simple to control the outerloop. What do you say?
>
> Ivan Kelly wrote:
> but it's unnecessary. submit() returns a future. you just need to call .get() on that to have the exact same effect.
>
> Rakesh R wrote:
> oops, I missed it. Will do:)
>
> Rakesh R wrote:
> Hi Ivan, I'm reopening this comment. In the latest patch I've used latch to control syncing up.
>
> I'm using FutureCallback to update the updatedLedgerCount. Now the updation of each ledger will be completed only after the futurecallback execution. When I tested I've observed that #get() api is coming out before invoking its futureCallback, this may give wrong log messages. For accuracy all outstanding ledgers needs to wait till the callback completion.
>
> Do you agree to use latch way of controlling the sequence?
>
> Ivan Kelly wrote:
> I don't understand the comment. Do you mean that submit(new Runnable(){...}).get() is returning before all callbacks have been completed? If this is the case, then latch doesn't solve it. The latch, as it is now, is functionally equivalent to calling get(). Timing may be different though. If it doesn't run correctly using the get() then there's probably a race condition lurking somewhere.
>
> Rakesh R wrote:
> It seems my explanation was bad:(
>
> >>>>>>>>>>The latch, as it is now, is functionally equivalent to calling get()
> Latch is used to see all the outstandings future#callback() execution is completed rather than outstandings future#get()
>
> Let me try to add more explanation. In the latest patch, it will register callback to the futures and this callback will be run when the Future's computation is complete. Callback has the logic to update the status of each ledger updation(success or failure).
>
> Algorithm:
> ----------
> 1) Set syncObj; // outer
>
> <Begin: iterate and for each ledger do>
> 1.1) Acquire thrttle
> 1.2)Creates ReadLedgerMetadataCb
> 1.3) Adds the respective ledgerid to 'outstandings'
> 1.4) Register Callback to update the status(success or failure) of the future. Also, callback has the logic to notify 'syncObj'
> 1.5) Update issuedLedgerCnt
> 1.6) Check & set the stop flag=true, if limit reaches
> 1.7) Call updateLedger operation
> <End:>
>
> 2) syncObj.await() -> syncObj will be notified only after all the issued ledgers finishes executing the registered callback.
> On callback, it will remove the respective ledgerid from 'outstandings' and updatedLedgerCnt#incr(). Finally, Callback will check whether outstandings#isEmpty() && stop==true, if yes notifies syncObj.
>
> Initially I tried using ReadLedgerMetadataCb#get() to see there is no outstandings. But I've noticied, this call comes out before the callback execution finishes and is giving wrong 'updatedLedgerCnt' value. I meant, return of ReadLedgerMetadataCb#get() is not guarantees that its registered callback is triggered and executed. Thats the reason I tried using separate 'latch' to mark the completion of all the issued ledgers. Does this makes sense to you?
Actually, the misunderstanding was mine here. When I read your comment and clicked to look at the source, it brought me to the first patch, rather than the latest. I was talking about the original latch, which is gone now. The way you use syncObj is perfectly good. I've only made one more comment (about throwing exceptions). When that's addressed, this change is ready to go.
- Ivan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/#review64360
-----------------------------------------------------------
On Jan. 1, 2015, 7:43 p.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28835/
> -----------------------------------------------------------
>
> (Updated Jan. 1, 2015, 7:43 p.m.)
>
>
> Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
>
>
> Bugs: BOKKEEPER-634
> https://issues.apache.org/jira/browse/BOKKEEPER-634
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> admin tool for changing the bookie's identifier to IP/hostname
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerCmdTest.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28835/diff/
>
>
> Testing
> -------
>
> Added few tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Rakesh R <ra...@huawei.com>.
> On Dec. 9, 2014, 12:08 p.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/admin/UpdateLedgerOp.java, line 82
> > <https://reviews.apache.org/r/28835/diff/1/?file=786287#file786287line82>
> >
> > Latch is not needed. The executor will return a future. just call .get() on it to get the same behaviour as the latch.
>
> Rakesh R wrote:
> yes. I will do it.
>
> Rakesh R wrote:
> I feel latch is simple to control the outerloop. What do you say?
>
> Ivan Kelly wrote:
> but it's unnecessary. submit() returns a future. you just need to call .get() on that to have the exact same effect.
>
> Rakesh R wrote:
> oops, I missed it. Will do:)
>
> Rakesh R wrote:
> Hi Ivan, I'm reopening this comment. In the latest patch I've used latch to control syncing up.
>
> I'm using FutureCallback to update the updatedLedgerCount. Now the updation of each ledger will be completed only after the futurecallback execution. When I tested I've observed that #get() api is coming out before invoking its futureCallback, this may give wrong log messages. For accuracy all outstanding ledgers needs to wait till the callback completion.
>
> Do you agree to use latch way of controlling the sequence?
>
> Ivan Kelly wrote:
> I don't understand the comment. Do you mean that submit(new Runnable(){...}).get() is returning before all callbacks have been completed? If this is the case, then latch doesn't solve it. The latch, as it is now, is functionally equivalent to calling get(). Timing may be different though. If it doesn't run correctly using the get() then there's probably a race condition lurking somewhere.
>
> Rakesh R wrote:
> It seems my explanation was bad:(
>
> >>>>>>>>>>The latch, as it is now, is functionally equivalent to calling get()
> Latch is used to see all the outstandings future#callback() execution is completed rather than outstandings future#get()
>
> Let me try to add more explanation. In the latest patch, it will register callback to the futures and this callback will be run when the Future's computation is complete. Callback has the logic to update the status of each ledger updation(success or failure).
>
> Algorithm:
> ----------
> 1) Set syncObj; // outer
>
> <Begin: iterate and for each ledger do>
> 1.1) Acquire thrttle
> 1.2)Creates ReadLedgerMetadataCb
> 1.3) Adds the respective ledgerid to 'outstandings'
> 1.4) Register Callback to update the status(success or failure) of the future. Also, callback has the logic to notify 'syncObj'
> 1.5) Update issuedLedgerCnt
> 1.6) Check & set the stop flag=true, if limit reaches
> 1.7) Call updateLedger operation
> <End:>
>
> 2) syncObj.await() -> syncObj will be notified only after all the issued ledgers finishes executing the registered callback.
> On callback, it will remove the respective ledgerid from 'outstandings' and updatedLedgerCnt#incr(). Finally, Callback will check whether outstandings#isEmpty() && stop==true, if yes notifies syncObj.
>
> Initially I tried using ReadLedgerMetadataCb#get() to see there is no outstandings. But I've noticied, this call comes out before the callback execution finishes and is giving wrong 'updatedLedgerCnt' value. I meant, return of ReadLedgerMetadataCb#get() is not guarantees that its registered callback is triggered and executed. Thats the reason I tried using separate 'latch' to mark the completion of all the issued ledgers. Does this makes sense to you?
>
> Ivan Kelly wrote:
> Actually, the misunderstanding was mine here. When I read your comment and clicked to look at the source, it brought me to the first patch, rather than the latest. I was talking about the original latch, which is gone now. The way you use syncObj is perfectly good. I've only made one more comment (about throwing exceptions). When that's addressed, this change is ready to go.
OK.
- Rakesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/#review64360
-----------------------------------------------------------
On Jan. 11, 2015, 3:41 p.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28835/
> -----------------------------------------------------------
>
> (Updated Jan. 11, 2015, 3:41 p.m.)
>
>
> Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
>
>
> Bugs: BOKKEEPER-634
> https://issues.apache.org/jira/browse/BOKKEEPER-634
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> admin tool for changing the bookie's identifier to IP/hostname
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerCmdTest.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28835/diff/
>
>
> Testing
> -------
>
> Added few tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Ivan Kelly <iv...@apache.org>.
> On Dec. 9, 2014, 12:08 p.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/admin/UpdateLedgerOp.java, line 82
> > <https://reviews.apache.org/r/28835/diff/1/?file=786287#file786287line82>
> >
> > Latch is not needed. The executor will return a future. just call .get() on it to get the same behaviour as the latch.
>
> Rakesh R wrote:
> yes. I will do it.
>
> Rakesh R wrote:
> I feel latch is simple to control the outerloop. What do you say?
but it's unnecessary. submit() returns a future. you just need to call .get() on that to have the exact same effect.
> On Dec. 9, 2014, 12:08 p.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java, line 1199
> > <https://reviews.apache.org/r/28835/diff/1/?file=786288#file786288line1199>
> >
> > what does 0 mean with regards to printing? should be (default false).
>
> Rakesh R wrote:
> Here I am taking option value as 'printMsgCount', using this I'm calculate the printing cycle. For example the value 100 means. After every 100, 200, 300... ledgers given for updation will print the message.
>
> Ivan Kelly wrote:
> This isn't clear to the user. Perhaps it would also be better to print every 5 seconds or so, rather than every X updates. Really what we want to show to the user is that the process is still working.
>
> Rakesh R wrote:
> If there are millions of ledgers, would be too noisy. Initially I had done periodically,but modified later based on Sijie's concerns.
> You want me to introduce one more parameter instead of reading the optionvalue from verbose, like 'printmessageon' counter value and print based on that?
I'd definitely make verbose and the period a separate parameter. -v, --verbose are pretty standard parameters in command line tools. They normally never take an argument. I'd either rename 'verbose' to 'printprogress' or make 'verbose' turn output on/off and then use 'printprogress' to specify how much. I prefer the first option, because you generally want some output at least.
Also, I don't think printing every 5 seconds is noisy. Noise is when there's too much info to read any of it. One line every 5 seconds is easy to handle. And it gives the user a hint that the process is progressing.
- Ivan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/#review64360
-----------------------------------------------------------
On Dec. 16, 2014, 4:07 a.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28835/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2014, 4:07 a.m.)
>
>
> Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
>
>
> Bugs: BOKKEEPER-634
> https://issues.apache.org/jira/browse/BOKKEEPER-634
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> admin tool for changing the bookie's identifier to IP/hostname
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28835/diff/
>
>
> Testing
> -------
>
> Added few tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Ivan Kelly <iv...@apache.org>.
> On Dec. 9, 2014, 12:08 p.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java, line 1199
> > <https://reviews.apache.org/r/28835/diff/1/?file=786288#file786288line1199>
> >
> > what does 0 mean with regards to printing? should be (default false).
>
> Rakesh R wrote:
> Here I am taking option value as 'printMsgCount', using this I'm calculate the printing cycle. For example the value 100 means. After every 100, 200, 300... ledgers given for updation will print the message.
This isn't clear to the user. Perhaps it would also be better to print every 5 seconds or so, rather than every X updates. Really what we want to show to the user is that the process is still working.
> On Dec. 9, 2014, 12:08 p.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/admin/UpdateLedgerOp.java, line 140
> > <https://reviews.apache.org/r/28835/diff/1/?file=786287#file786287line140>
> >
> > There's a lot of modification from afar going here around ledgersWaitForCompletion. Why do you need to countDown() the latch and to remove() it from the list? Really you only want to track how many requests have been started, and completed.
> >
> > It would be cleaner to allocate the tracker object in main #run thread, and pass it in at that point. That way, when we wait on it, its clear that we're waiting on something we have passed into the request. In fact, why not make ReadLedgerMetadataCb extends AbstractFuture<LedgerMetadata>? Then you can build a list of ReadLedgerMetadataCb objects in the run loop, and wait on all futures at the end. This will also handle the erroring, as you can make the future throw an error to tell you to stop.
>
> Rakesh R wrote:
> The idea for the 'ledgersWaitForCompletion' is after iterating over all the ledgers, will wait for the items which are yet to be completed. For example, assume there are 100,000 ledgers. It will be iterate and call the ledgers based on the ratelimitting factor. During the iteration time most of the ledgers might be completed their updations. Now, at the end of the iteration it will be have only few ledgers which are waiting for the updates. So this thread needs to wait for only those ledgers.
>
>
> Say, if there are many ledgers in the system, adding ReadLedgerMetadataCb objects into the list will create problem, sin't it? I'm afraid of keeping all the ledgers callback objects in memory. Thats the reason I thought of removing immedidately on success updation so that the list will shrink.
I meant something like https://github.com/ivankelly/bookkeeper/commit/39cbacc582e661561fd19c978e38ac9bf4ac7b84
I haven't tried to compile that, it's just to give an idea, but it would allow UpdateLedgerMetadataCb to be removed completely, and all the flow logic fits into one screen.
> On Dec. 9, 2014, 12:08 p.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/admin/UpdateLedgerOp.java, line 83
> > <https://reviews.apache.org/r/28835/diff/1/?file=786287#file786287line83>
> >
> > updateCb would be better than metaCb. there's a lot of metadata ops happening.
> >
> > Also, create it inside #run(). It doesn't need to be this far up in the scope.
>
> Rakesh R wrote:
> ok, i will rename it. I think I need to keep it outside #run() as it will wait for the completion like:
>
> long ledgerUpdatedCount = updateCb.get();
>
> isn't it?
No, it the Future returned from submit that you have to all #get() on.
- Ivan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/#review64360
-----------------------------------------------------------
On Dec. 9, 2014, 4:05 a.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28835/
> -----------------------------------------------------------
>
> (Updated Dec. 9, 2014, 4:05 a.m.)
>
>
> Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
>
>
> Bugs: BOKKEEPER-634
> https://issues.apache.org/jira/browse/BOKKEEPER-634
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> admin tool for changing the bookie's identifier to IP/hostname
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/admin/UpdateLedgerOp.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java 49d8e59
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28835/diff/
>
>
> Testing
> -------
>
> Added few tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Rakesh R <ra...@huawei.com>.
> On Dec. 9, 2014, 12:08 p.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java, line 1199
> > <https://reviews.apache.org/r/28835/diff/1/?file=786288#file786288line1199>
> >
> > what does 0 mean with regards to printing? should be (default false).
>
> Rakesh R wrote:
> Here I am taking option value as 'printMsgCount', using this I'm calculate the printing cycle. For example the value 100 means. After every 100, 200, 300... ledgers given for updation will print the message.
>
> Ivan Kelly wrote:
> This isn't clear to the user. Perhaps it would also be better to print every 5 seconds or so, rather than every X updates. Really what we want to show to the user is that the process is still working.
>
> Rakesh R wrote:
> If there are millions of ledgers, would be too noisy. Initially I had done periodically,but modified later based on Sijie's concerns.
> You want me to introduce one more parameter instead of reading the optionvalue from verbose, like 'printmessageon' counter value and print based on that?
>
> Ivan Kelly wrote:
> I'd definitely make verbose and the period a separate parameter. -v, --verbose are pretty standard parameters in command line tools. They normally never take an argument. I'd either rename 'verbose' to 'printprogress' or make 'verbose' turn output on/off and then use 'printprogress' to specify how much. I prefer the first option, because you generally want some output at least.
>
> Also, I don't think printing every 5 seconds is noisy. Noise is when there's too much info to read any of it. One line every 5 seconds is easy to handle. And it gives the user a hint that the process is progressing.
>
> Rakesh R wrote:
> I agree to provide verbose option [on/off] and introduce one more option 'printprogress'. This will have the oprion valie to print the frequency of the message.
>
> Since Sijie had given a comment earlier, it would be really great if Sijie can give a reply. Thanks!
> Sijie, do you agree to print messages on every configured seconds?
>
> Sijie Guo wrote:
> I am fine with seconds, but it should be able to configure
Thanks Sijie for the quick reply. Will provide 'printprogress' option to define the frequency. This will be in conjunction with verbose flag(on true)
- Rakesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/#review64360
-----------------------------------------------------------
On Dec. 16, 2014, 4:07 a.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28835/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2014, 4:07 a.m.)
>
>
> Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
>
>
> Bugs: BOKKEEPER-634
> https://issues.apache.org/jira/browse/BOKKEEPER-634
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> admin tool for changing the bookie's identifier to IP/hostname
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28835/diff/
>
>
> Testing
> -------
>
> Added few tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Rakesh R <ra...@huawei.com>.
> On Dec. 9, 2014, 12:08 p.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java, line 1198
> > <https://reviews.apache.org/r/28835/diff/1/?file=786288#file786288line1198>
> >
> > updation -> update
> >
> > In fact I would reword.
> >
> > Maximum number of ledgers to update (default: no limit).
k. I'll do it.
> On Dec. 9, 2014, 12:08 p.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java, line 1196
> > <https://reviews.apache.org/r/28835/diff/1/?file=786288#file786288line1196>
> >
> > 'no:' should be 'no.'
ok. I'll do it
> On Dec. 9, 2014, 12:08 p.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java, line 1195
> > <https://reviews.apache.org/r/28835/diff/1/?file=786288#file786288line1195>
> >
> > long option should be updatesPerSec. Bandwidth is confusing in this context.
ok, I will use as "updatespersec"
> On Dec. 9, 2014, 12:08 p.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/admin/UpdateLedgerOp.java, line 125
> > <https://reviews.apache.org/r/28835/diff/1/?file=786287#file786287line125>
> >
> > It's a little ugly to mix business logic and presentation. I think it would be better to pass in a notifier/callback type like.
> >
> > interface BlahNotifier {
> > void progress(long updated, long issued);
> > }
> >
> > And then have BookieShell pass in an implementaiton of this.
ok. let me try.
> On Dec. 9, 2014, 12:08 p.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/admin/UpdateLedgerOp.java, line 83
> > <https://reviews.apache.org/r/28835/diff/1/?file=786287#file786287line83>
> >
> > updateCb would be better than metaCb. there's a lot of metadata ops happening.
> >
> > Also, create it inside #run(). It doesn't need to be this far up in the scope.
ok, i will rename it. I think I need to keep it outside #run() as it will wait for the completion like:
long ledgerUpdatedCount = updateCb.get();
isn't it?
> On Dec. 9, 2014, 12:08 p.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/admin/UpdateLedgerOp.java, line 82
> > <https://reviews.apache.org/r/28835/diff/1/?file=786287#file786287line82>
> >
> > Latch is not needed. The executor will return a future. just call .get() on it to get the same behaviour as the latch.
yes. I will do it.
- Rakesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/#review64360
-----------------------------------------------------------
On Dec. 9, 2014, 4:05 a.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28835/
> -----------------------------------------------------------
>
> (Updated Dec. 9, 2014, 4:05 a.m.)
>
>
> Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
>
>
> Bugs: BOKKEEPER-634
> https://issues.apache.org/jira/browse/BOKKEEPER-634
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> admin tool for changing the bookie's identifier to IP/hostname
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/admin/UpdateLedgerOp.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java 49d8e59
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28835/diff/
>
>
> Testing
> -------
>
> Added few tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Rakesh R <ra...@huawei.com>.
- Rakesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/#review64360
-----------------------------------------------------------
On Jan. 1, 2015, 7:43 p.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28835/
> -----------------------------------------------------------
>
> (Updated Jan. 1, 2015, 7:43 p.m.)
>
>
> Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
>
>
> Bugs: BOKKEEPER-634
> https://issues.apache.org/jira/browse/BOKKEEPER-634
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> admin tool for changing the bookie's identifier to IP/hostname
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerCmdTest.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28835/diff/
>
>
> Testing
> -------
>
> Added few tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Ivan Kelly <iv...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/#review64360
-----------------------------------------------------------
bookkeeper-server/src/main/java/org/apache/bookkeeper/admin/UpdateLedgerOp.java
<https://reviews.apache.org/r/28835/#comment107012>
trailing whitespace
bookkeeper-server/src/main/java/org/apache/bookkeeper/admin/UpdateLedgerOp.java
<https://reviews.apache.org/r/28835/#comment107011>
trailing whitespace
bookkeeper-server/src/main/java/org/apache/bookkeeper/admin/UpdateLedgerOp.java
<https://reviews.apache.org/r/28835/#comment107016>
Passing in stop as a variable makes the flow of operation unclear. In the while loop that its used in, its unclear as to what 'stop' relates to. Who sets it?
It would be better to have a hasErrorOccurred() method in UpdateLedgerMetadataCallback and call that instead of !stop.get().
bookkeeper-server/src/main/java/org/apache/bookkeeper/admin/UpdateLedgerOp.java
<https://reviews.apache.org/r/28835/#comment107013>
Latch is not needed. The executor will return a future. just call .get() on it to get the same behaviour as the latch.
bookkeeper-server/src/main/java/org/apache/bookkeeper/admin/UpdateLedgerOp.java
<https://reviews.apache.org/r/28835/#comment107017>
updateCb would be better than metaCb. there's a lot of metadata ops happening.
Also, create it inside #run(). It doesn't need to be this far up in the scope.
bookkeeper-server/src/main/java/org/apache/bookkeeper/admin/UpdateLedgerOp.java
<https://reviews.apache.org/r/28835/#comment107010>
You can use guava thread factory builder for this.
bookkeeper-server/src/main/java/org/apache/bookkeeper/admin/UpdateLedgerOp.java
<https://reviews.apache.org/r/28835/#comment107019>
There's a race here. What if nothing is in ledgersWaitForCompletion by the time we get to this line? But then something gets added after?
Also, these 4 lines should be wrapped into a method of UpdateLedgerMetadataCb.
bookkeeper-server/src/main/java/org/apache/bookkeeper/admin/UpdateLedgerOp.java
<https://reviews.apache.org/r/28835/#comment107014>
It's a little ugly to mix business logic and presentation. I think it would be better to pass in a notifier/callback type like.
interface BlahNotifier {
void progress(long updated, long issued);
}
And then have BookieShell pass in an implementaiton of this.
bookkeeper-server/src/main/java/org/apache/bookkeeper/admin/UpdateLedgerOp.java
<https://reviews.apache.org/r/28835/#comment107021>
There's a lot of modification from afar going here around ledgersWaitForCompletion. Why do you need to countDown() the latch and to remove() it from the list? Really you only want to track how many requests have been started, and completed.
It would be cleaner to allocate the tracker object in main #run thread, and pass it in at that point. That way, when we wait on it, its clear that we're waiting on something we have passed into the request. In fact, why not make ReadLedgerMetadataCb extends AbstractFuture<LedgerMetadata>? Then you can build a list of ReadLedgerMetadataCb objects in the run loop, and wait on all futures at the end. This will also handle the erroring, as you can make the future throw an error to tell you to stop.
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java
<https://reviews.apache.org/r/28835/#comment107024>
long option should be updatesPerSec. Bandwidth is confusing in this context.
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java
<https://reviews.apache.org/r/28835/#comment107025>
'no:' should be 'no.'
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java
<https://reviews.apache.org/r/28835/#comment107022>
updation -> update
In fact I would reword.
Maximum number of ledgers to update (default: no limit).
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java
<https://reviews.apache.org/r/28835/#comment107023>
what does 0 mean with regards to printing? should be (default false).
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java
<https://reviews.apache.org/r/28835/#comment107026>
Why make this public rather than moving UpdateLedgerOp into client package?
- Ivan Kelly
On Dec. 9, 2014, 4:05 a.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28835/
> -----------------------------------------------------------
>
> (Updated Dec. 9, 2014, 4:05 a.m.)
>
>
> Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
>
>
> Bugs: BOKKEEPER-634
> https://issues.apache.org/jira/browse/BOKKEEPER-634
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> admin tool for changing the bookie's identifier to IP/hostname
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/admin/UpdateLedgerOp.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java 49d8e59
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28835/diff/
>
>
> Testing
> -------
>
> Added few tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Sijie Guo <gu...@gmail.com>.
> On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java, line 118
> > <https://reviews.apache.org/r/28835/diff/2/?file=792808#file792808line118>
> >
> > why you need #get() here? isn't Runnable called only when readCb is completed?
>
> Rakesh R wrote:
> readCb.get() is used to see any errors during the ledger update. In case of errors will stop issuing of ledgers and will exit.
readCb has a future, why can't you register a FutureCallback?
http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/util/concurrent/Futures.html#addCallback%28com.google.common.util.concurrent.ListenableFuture,%20com.google.common.util.concurrent.FutureCallback%29
so in the FutureCallback, you will know whether the operation is completed or failed.
> On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java, line 183
> > <https://reviews.apache.org/r/28835/diff/2/?file=792808#file792808line183>
> >
> > this is totally wrong.
> >
> > #addListener is "to Registers a listener to be run on the given executor. The listener will run when the Future's computation is complete or, if the computation is already complete, immediately."
> >
> > in your implementation, you submit the runnable immediately, without checking if the future's compuation is complete or not.
> >
> >
> > you should use SettableFuture<Void>
> >
> > private final static class ReadLedgerMetadataCb .. {
> >
> > final SettableFuture<Void> resultFuture = ...
> >
> > public addListener(...) {
> > resultFuture.addListener(...);
> > }
> >
> > public void operationComplete(...) {
> > resultFuture.set(...)
> > }
> >
> > }
>
> Rakesh R wrote:
> Its new learning for me. I tried using the SettableFuture in latest patch. Please let me know the usage is proper. thanks!
>
> I met the following exception as the listener is a kind of async notification. What I'm thinking is, readCb.get() will comeout immedaitely after future.set() is called but before invoking the registered listener. Now during the execution there could be chances of finshing the main thread and pExecutor shutdown. I feel the below exception is expected and is OK.
>
> 2014-12-21 22:54:07,268 - INFO - [UpdateLedgerThread:UpdateLedgerOp$1@142] - Total number of ledgers updated = 1
> Dec 21, 2014 10:54:07 PM com.google.common.util.concurrent.ExecutionList$RunnableExecutorPair execute
> SEVERE: RuntimeException while executing runnable org.apache.bookkeeper.client.UpdateLedgerOp$1$1@7da1f182 with executor java.util.concurrent.Executors$FinalizableDelegatedExecutorService@34741c9d
> java.util.concurrent.RejectedExecutionException: Task org.apache.bookkeeper.client.UpdateLedgerOp$1$1@7da1f182 rejected from java.util.concurrent.ThreadPoolExecutor@f64cb82[Terminated, pool size = 0, active threads = 0, queued tasks = 0, completed tasks = 0]
> at java.util.concurrent.ThreadPoolExecutor$AbortPolicy.rejectedExecution(Unknown Source)
> at java.util.concurrent.ThreadPoolExecutor.reject(Unknown Source)
> at java.util.concurrent.ThreadPoolExecutor.execute(Unknown Source)
> at java.util.concurrent.Executors$DelegatedExecutorService.execute(Unknown Source)
> at com.google.common.util.concurrent.ExecutionList$RunnableExecutorPair.execute(ExecutionList.java:149)
> at com.google.common.util.concurrent.ExecutionList.execute(ExecutionList.java:134)
> at com.google.common.util.concurrent.AbstractFuture.set(AbstractFuture.java:175)
> at com.google.common.util.concurrent.SettableFuture.set(SettableFuture.java:53)
> at org.apache.bookkeeper.client.UpdateLedgerOp$ReadLedgerMetadataCb$1.operationComplete(UpdateLedgerOp.java:241)
> at org.apache.bookkeeper.client.UpdateLedgerOp$ReadLedgerMetadataCb$1.operationComplete(UpdateLedgerOp.java:1)
> at org.apache.bookkeeper.meta.CleanupLedgerManager$CleanupGenericCallback.operationComplete(CleanupLedgerManager.java:51)
> at org.apache.bookkeeper.meta.AbstractZkLedgerManager$3.processResult(AbstractZkLedgerManager.java:360)
> at org.apache.zookeeper.ClientCnxn$EventThread.processEvent(ClientCnxn.java:545)
> at org.apache.zookeeper.ClientCnxn$EventThread.run(ClientCnxn.java:497)
you shutdown the executor before all futures are satisfied.
> On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java, line 1194
> > <https://reviews.apache.org/r/28835/diff/2/?file=792807#file792807line1194>
> >
> > The bookieId option is really weird. A much clear option would specify new bookieId, rather use 'ip' or 'hostname'.
> >
> > -1 on the option
>
> Rakesh R wrote:
> Followed same pattern of cookie updates. Make 'hostname' and 'ip' as constants, so the user could only specify "updatebookie -bookieId hostname" or "updatebookie -bookieId ip". Does this makes sense to you?
not at all. the cases to change bookie id are not just switching bookie id between ip and hostname. it also could be the case you need to change ip address.
- Sijie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/#review65458
-----------------------------------------------------------
On Dec. 21, 2014, 5:52 p.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28835/
> -----------------------------------------------------------
>
> (Updated Dec. 21, 2014, 5:52 p.m.)
>
>
> Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
>
>
> Bugs: BOKKEEPER-634
> https://issues.apache.org/jira/browse/BOKKEEPER-634
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> admin tool for changing the bookie's identifier to IP/hostname
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28835/diff/
>
>
> Testing
> -------
>
> Added few tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Rakesh R <ra...@huawei.com>.
> On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java, line 1190
> > <https://reviews.apache.org/r/28835/diff/2/?file=792807#file792807line1190>
> >
> > final
done
> On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java, line 1194
> > <https://reviews.apache.org/r/28835/diff/2/?file=792807#file792807line1194>
> >
> > The bookieId option is really weird. A much clear option would specify new bookieId, rather use 'ip' or 'hostname'.
> >
> > -1 on the option
Followed same pattern of cookie updates. Make 'hostname' and 'ip' as constants, so the user could only specify "updatebookie -bookieId hostname" or "updatebookie -bookieId ip". Does this makes sense to you?
> On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java, line 1214
> > <https://reviews.apache.org/r/28835/diff/2/?file=792807#file792807line1214>
> >
> > please update this reflecting to new options.
ok
> On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java, line 1226
> > <https://reviews.apache.org/r/28835/diff/2/?file=792807#file792807line1226>
> >
> > use '{}'
ok
> On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java, line 1238
> > <https://reviews.apache.org/r/28835/diff/2/?file=792807#file792807line1238>
> >
> > inconsistent parameter
ok
> On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java, line 1240
> > <https://reviews.apache.org/r/28835/diff/2/?file=792807#file792807line1240>
> >
> > update log statement to reflect correct parameter.
> >
> > "+" -> ","
ok
> On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java, line 1691
> > <https://reviews.apache.org/r/28835/diff/2/?file=792807#file792807line1691>
> >
> > static?
ok
> On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java, line 1283
> > <https://reviews.apache.org/r/28835/diff/2/?file=792807#file792807line1283>
> >
> > trailing spaces
ok
> On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java, line 94
> > <https://reviews.apache.org/r/28835/diff/2/?file=792808#file792808line94>
> >
> > move run method to a separate method
Extract #run logic as #updateLedgers
> On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java, line 95
> > <https://reviews.apache.org/r/28835/diff/2/?file=792808#file792808line95>
> >
> > ThreadFactoryBuilder
ok
> On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java, line 105
> > <https://reviews.apache.org/r/28835/diff/2/?file=792808#file792808line105>
> >
> > outstandings
ok
> On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java, line 118
> > <https://reviews.apache.org/r/28835/diff/2/?file=792808#file792808line118>
> >
> > why you need #get() here? isn't Runnable called only when readCb is completed?
readCb.get() is used to see any errors during the ledger update. In case of errors will stop issuing of ledgers and will exit.
> On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java, line 154
> > <https://reviews.apache.org/r/28835/diff/2/?file=792808#file792808line154>
> >
> > why #shutdownNow? not shutdown?
ok
> On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java, line 165
> > <https://reviews.apache.org/r/28835/diff/2/?file=792808#file792808line165>
> >
> > in finally block
ok
> On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java, line 183
> > <https://reviews.apache.org/r/28835/diff/2/?file=792808#file792808line183>
> >
> > this is totally wrong.
> >
> > #addListener is "to Registers a listener to be run on the given executor. The listener will run when the Future's computation is complete or, if the computation is already complete, immediately."
> >
> > in your implementation, you submit the runnable immediately, without checking if the future's compuation is complete or not.
> >
> >
> > you should use SettableFuture<Void>
> >
> > private final static class ReadLedgerMetadataCb .. {
> >
> > final SettableFuture<Void> resultFuture = ...
> >
> > public addListener(...) {
> > resultFuture.addListener(...);
> > }
> >
> > public void operationComplete(...) {
> > resultFuture.set(...)
> > }
> >
> > }
Its new learning for me. I tried using the SettableFuture in latest patch. Please let me know the usage is proper. thanks!
I met the following exception as the listener is a kind of async notification. What I'm thinking is, readCb.get() will comeout immedaitely after future.set() is called but before invoking the registered listener. Now during the execution there could be chances of finshing the main thread and pExecutor shutdown. I feel the below exception is expected and is OK.
2014-12-21 22:54:07,268 - INFO - [UpdateLedgerThread:UpdateLedgerOp$1@142] - Total number of ledgers updated = 1
Dec 21, 2014 10:54:07 PM com.google.common.util.concurrent.ExecutionList$RunnableExecutorPair execute
SEVERE: RuntimeException while executing runnable org.apache.bookkeeper.client.UpdateLedgerOp$1$1@7da1f182 with executor java.util.concurrent.Executors$FinalizableDelegatedExecutorService@34741c9d
java.util.concurrent.RejectedExecutionException: Task org.apache.bookkeeper.client.UpdateLedgerOp$1$1@7da1f182 rejected from java.util.concurrent.ThreadPoolExecutor@f64cb82[Terminated, pool size = 0, active threads = 0, queued tasks = 0, completed tasks = 0]
at java.util.concurrent.ThreadPoolExecutor$AbortPolicy.rejectedExecution(Unknown Source)
at java.util.concurrent.ThreadPoolExecutor.reject(Unknown Source)
at java.util.concurrent.ThreadPoolExecutor.execute(Unknown Source)
at java.util.concurrent.Executors$DelegatedExecutorService.execute(Unknown Source)
at com.google.common.util.concurrent.ExecutionList$RunnableExecutorPair.execute(ExecutionList.java:149)
at com.google.common.util.concurrent.ExecutionList.execute(ExecutionList.java:134)
at com.google.common.util.concurrent.AbstractFuture.set(AbstractFuture.java:175)
at com.google.common.util.concurrent.SettableFuture.set(SettableFuture.java:53)
at org.apache.bookkeeper.client.UpdateLedgerOp$ReadLedgerMetadataCb$1.operationComplete(UpdateLedgerOp.java:241)
at org.apache.bookkeeper.client.UpdateLedgerOp$ReadLedgerMetadataCb$1.operationComplete(UpdateLedgerOp.java:1)
at org.apache.bookkeeper.meta.CleanupLedgerManager$CleanupGenericCallback.operationComplete(CleanupLedgerManager.java:51)
at org.apache.bookkeeper.meta.AbstractZkLedgerManager$3.processResult(AbstractZkLedgerManager.java:360)
at org.apache.zookeeper.ClientCnxn$EventThread.processEvent(ClientCnxn.java:545)
at org.apache.zookeeper.ClientCnxn$EventThread.run(ClientCnxn.java:497)
> On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java, line 194
> > <https://reviews.apache.org/r/28835/diff/2/?file=792808#file792808line194>
> >
> > please remove newObject[]. you don't need this
ok
> On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java, line 215
> > <https://reviews.apache.org/r/28835/diff/2/?file=792808#file792808line215>
> >
> > remove new Object[] {}
ok
> On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java, line 71
> > <https://reviews.apache.org/r/28835/diff/2/?file=792809#file792809line71>
> >
> > why you need add any entries? it is just a metadata change. it'd better to not add any entries.
ok
> On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java, line 91
> > <https://reviews.apache.org/r/28835/diff/2/?file=792809#file792809line91>
> >
> > please add Assert.assertFalse(ensemble.contains(curBookieAddr))
ok
> On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java, line 109
> > <https://reviews.apache.org/r/28835/diff/2/?file=792809#file792809line109>
> >
> > change entries to 0
ok
- Rakesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/#review65458
-----------------------------------------------------------
On Dec. 16, 2014, 4:07 a.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28835/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2014, 4:07 a.m.)
>
>
> Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
>
>
> Bugs: BOKKEEPER-634
> https://issues.apache.org/jira/browse/BOKKEEPER-634
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> admin tool for changing the bookie's identifier to IP/hostname
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28835/diff/
>
>
> Testing
> -------
>
> Added few tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Rakesh R <ra...@huawei.com>.
> On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java, line 1194
> > <https://reviews.apache.org/r/28835/diff/2/?file=792807#file792807line1194>
> >
> > The bookieId option is really weird. A much clear option would specify new bookieId, rather use 'ip' or 'hostname'.
> >
> > -1 on the option
>
> Rakesh R wrote:
> Followed same pattern of cookie updates. Make 'hostname' and 'ip' as constants, so the user could only specify "updatebookie -bookieId hostname" or "updatebookie -bookieId ip". Does this makes sense to you?
>
> Sijie Guo wrote:
> not at all. the cases to change bookie id are not just switching bookie id between ip and hostname. it also could be the case you need to change ip address.
Thanks Sijie for this usecase. Can we suggest users to change the 'ipaddress_1' to 'hostname', instead of changing 'ipaddress_1' to another 'ipaddress_2'. Considering that the hostname would not be changed frequently comparing to the ipaddress change. Do you agree with me?
After seeing this comment, if this is a real world usecase then I feel the same is applicable to Cookie also ?
- Rakesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/#review65458
-----------------------------------------------------------
On Dec. 21, 2014, 5:52 p.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28835/
> -----------------------------------------------------------
>
> (Updated Dec. 21, 2014, 5:52 p.m.)
>
>
> Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
>
>
> Bugs: BOKKEEPER-634
> https://issues.apache.org/jira/browse/BOKKEEPER-634
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> admin tool for changing the bookie's identifier to IP/hostname
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28835/diff/
>
>
> Testing
> -------
>
> Added few tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Rakesh R <ra...@huawei.com>.
> On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java, line 183
> > <https://reviews.apache.org/r/28835/diff/2/?file=792808#file792808line183>
> >
> > this is totally wrong.
> >
> > #addListener is "to Registers a listener to be run on the given executor. The listener will run when the Future's computation is complete or, if the computation is already complete, immediately."
> >
> > in your implementation, you submit the runnable immediately, without checking if the future's compuation is complete or not.
> >
> >
> > you should use SettableFuture<Void>
> >
> > private final static class ReadLedgerMetadataCb .. {
> >
> > final SettableFuture<Void> resultFuture = ...
> >
> > public addListener(...) {
> > resultFuture.addListener(...);
> > }
> >
> > public void operationComplete(...) {
> > resultFuture.set(...)
> > }
> >
> > }
>
> Rakesh R wrote:
> Its new learning for me. I tried using the SettableFuture in latest patch. Please let me know the usage is proper. thanks!
>
> I met the following exception as the listener is a kind of async notification. What I'm thinking is, readCb.get() will comeout immedaitely after future.set() is called but before invoking the registered listener. Now during the execution there could be chances of finshing the main thread and pExecutor shutdown. I feel the below exception is expected and is OK.
>
> 2014-12-21 22:54:07,268 - INFO - [UpdateLedgerThread:UpdateLedgerOp$1@142] - Total number of ledgers updated = 1
> Dec 21, 2014 10:54:07 PM com.google.common.util.concurrent.ExecutionList$RunnableExecutorPair execute
> SEVERE: RuntimeException while executing runnable org.apache.bookkeeper.client.UpdateLedgerOp$1$1@7da1f182 with executor java.util.concurrent.Executors$FinalizableDelegatedExecutorService@34741c9d
> java.util.concurrent.RejectedExecutionException: Task org.apache.bookkeeper.client.UpdateLedgerOp$1$1@7da1f182 rejected from java.util.concurrent.ThreadPoolExecutor@f64cb82[Terminated, pool size = 0, active threads = 0, queued tasks = 0, completed tasks = 0]
> at java.util.concurrent.ThreadPoolExecutor$AbortPolicy.rejectedExecution(Unknown Source)
> at java.util.concurrent.ThreadPoolExecutor.reject(Unknown Source)
> at java.util.concurrent.ThreadPoolExecutor.execute(Unknown Source)
> at java.util.concurrent.Executors$DelegatedExecutorService.execute(Unknown Source)
> at com.google.common.util.concurrent.ExecutionList$RunnableExecutorPair.execute(ExecutionList.java:149)
> at com.google.common.util.concurrent.ExecutionList.execute(ExecutionList.java:134)
> at com.google.common.util.concurrent.AbstractFuture.set(AbstractFuture.java:175)
> at com.google.common.util.concurrent.SettableFuture.set(SettableFuture.java:53)
> at org.apache.bookkeeper.client.UpdateLedgerOp$ReadLedgerMetadataCb$1.operationComplete(UpdateLedgerOp.java:241)
> at org.apache.bookkeeper.client.UpdateLedgerOp$ReadLedgerMetadataCb$1.operationComplete(UpdateLedgerOp.java:1)
> at org.apache.bookkeeper.meta.CleanupLedgerManager$CleanupGenericCallback.operationComplete(CleanupLedgerManager.java:51)
> at org.apache.bookkeeper.meta.AbstractZkLedgerManager$3.processResult(AbstractZkLedgerManager.java:360)
> at org.apache.zookeeper.ClientCnxn$EventThread.processEvent(ClientCnxn.java:545)
> at org.apache.zookeeper.ClientCnxn$EventThread.run(ClientCnxn.java:497)
>
> Sijie Guo wrote:
> you shutdown the executor before all futures are satisfied.
Latest patch doesn't requires this executor and I've removed this executor usage.
- Rakesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/#review65458
-----------------------------------------------------------
On Jan. 11, 2015, 3:41 p.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28835/
> -----------------------------------------------------------
>
> (Updated Jan. 11, 2015, 3:41 p.m.)
>
>
> Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
>
>
> Bugs: BOKKEEPER-634
> https://issues.apache.org/jira/browse/BOKKEEPER-634
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> admin tool for changing the bookie's identifier to IP/hostname
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerCmdTest.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28835/diff/
>
>
> Testing
> -------
>
> Added few tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Rakesh R <ra...@huawei.com>.
> On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java, line 1194
> > <https://reviews.apache.org/r/28835/diff/2/?file=792807#file792807line1194>
> >
> > The bookieId option is really weird. A much clear option would specify new bookieId, rather use 'ip' or 'hostname'.
> >
> > -1 on the option
>
> Rakesh R wrote:
> Followed same pattern of cookie updates. Make 'hostname' and 'ip' as constants, so the user could only specify "updatebookie -bookieId hostname" or "updatebookie -bookieId ip". Does this makes sense to you?
>
> Sijie Guo wrote:
> not at all. the cases to change bookie id are not just switching bookie id between ip and hostname. it also could be the case you need to change ip address.
>
> Rakesh R wrote:
> Thanks Sijie for this usecase. Can we suggest users to change the 'ipaddress_1' to 'hostname', instead of changing 'ipaddress_1' to another 'ipaddress_2'. Considering that the hostname would not be changed frequently comparing to the ipaddress change. Do you agree with me?
>
> After seeing this comment, if this is a real world usecase then I feel the same is applicable to Cookie also ?
>
> Sijie Guo wrote:
> it doesn't matter how often it happens. if it happens, the tool need to be able to handle this case. using explicit bookie id is much more straightfoward and clean behavior, rather than implicit hostname & ipaddress switching.
>
> I don't remember how cookie command. but yes, it applies for cookie too. I do remember I made similar comment when reviewing cookie tool, no?
Kindly refer the below comment:
https://issues.apache.org/jira/browse/BOOKKEEPER-773?focusedCommentId=14143268&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14143268
Initially I tried to make it more generic by taking bookieId from the user, but based on this comment it has been agreed upon to go ahead with the syntax,
updatecookie -bookieId <hostname|ip>
- Rakesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/#review65458
-----------------------------------------------------------
On Dec. 21, 2014, 5:52 p.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28835/
> -----------------------------------------------------------
>
> (Updated Dec. 21, 2014, 5:52 p.m.)
>
>
> Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
>
>
> Bugs: BOKKEEPER-634
> https://issues.apache.org/jira/browse/BOKKEEPER-634
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> admin tool for changing the bookie's identifier to IP/hostname
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28835/diff/
>
>
> Testing
> -------
>
> Added few tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Sijie Guo <gu...@gmail.com>.
> On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java, line 1194
> > <https://reviews.apache.org/r/28835/diff/2/?file=792807#file792807line1194>
> >
> > The bookieId option is really weird. A much clear option would specify new bookieId, rather use 'ip' or 'hostname'.
> >
> > -1 on the option
>
> Rakesh R wrote:
> Followed same pattern of cookie updates. Make 'hostname' and 'ip' as constants, so the user could only specify "updatebookie -bookieId hostname" or "updatebookie -bookieId ip". Does this makes sense to you?
>
> Sijie Guo wrote:
> not at all. the cases to change bookie id are not just switching bookie id between ip and hostname. it also could be the case you need to change ip address.
>
> Rakesh R wrote:
> Thanks Sijie for this usecase. Can we suggest users to change the 'ipaddress_1' to 'hostname', instead of changing 'ipaddress_1' to another 'ipaddress_2'. Considering that the hostname would not be changed frequently comparing to the ipaddress change. Do you agree with me?
>
> After seeing this comment, if this is a real world usecase then I feel the same is applicable to Cookie also ?
>
> Sijie Guo wrote:
> it doesn't matter how often it happens. if it happens, the tool need to be able to handle this case. using explicit bookie id is much more straightfoward and clean behavior, rather than implicit hostname & ipaddress switching.
>
> I don't remember how cookie command. but yes, it applies for cookie too. I do remember I made similar comment when reviewing cookie tool, no?
>
> Rakesh R wrote:
> Kindly refer the below comment:
> https://issues.apache.org/jira/browse/BOOKKEEPER-773?focusedCommentId=14143268&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14143268
>
>
> Initially I tried to make it more generic by taking bookieId from the user, but based on this comment it has been agreed upon to go ahead with the syntax,
>
> updatecookie -bookieId <hostname|ip>
ok. anyway, I am fine to stick to the constants right now. we might change it later.
- Sijie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/#review65458
-----------------------------------------------------------
On Dec. 21, 2014, 5:52 p.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28835/
> -----------------------------------------------------------
>
> (Updated Dec. 21, 2014, 5:52 p.m.)
>
>
> Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
>
>
> Bugs: BOKKEEPER-634
> https://issues.apache.org/jira/browse/BOKKEEPER-634
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> admin tool for changing the bookie's identifier to IP/hostname
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28835/diff/
>
>
> Testing
> -------
>
> Added few tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Rakesh R <ra...@huawei.com>.
> On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java, line 118
> > <https://reviews.apache.org/r/28835/diff/2/?file=792808#file792808line118>
> >
> > why you need #get() here? isn't Runnable called only when readCb is completed?
>
> Rakesh R wrote:
> readCb.get() is used to see any errors during the ledger update. In case of errors will stop issuing of ledgers and will exit.
>
> Sijie Guo wrote:
> readCb has a future, why can't you register a FutureCallback?
>
> http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/util/concurrent/Futures.html#addCallback%28com.google.common.util.concurrent.ListenableFuture,%20com.google.common.util.concurrent.FutureCallback%29
>
> so in the FutureCallback, you will know whether the operation is completed or failed.
I have used futurecallback and updated the result in #success and #failure implemetations.
> On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java, line 1194
> > <https://reviews.apache.org/r/28835/diff/2/?file=792807#file792807line1194>
> >
> > The bookieId option is really weird. A much clear option would specify new bookieId, rather use 'ip' or 'hostname'.
> >
> > -1 on the option
>
> Rakesh R wrote:
> Followed same pattern of cookie updates. Make 'hostname' and 'ip' as constants, so the user could only specify "updatebookie -bookieId hostname" or "updatebookie -bookieId ip". Does this makes sense to you?
>
> Sijie Guo wrote:
> not at all. the cases to change bookie id are not just switching bookie id between ip and hostname. it also could be the case you need to change ip address.
>
> Rakesh R wrote:
> Thanks Sijie for this usecase. Can we suggest users to change the 'ipaddress_1' to 'hostname', instead of changing 'ipaddress_1' to another 'ipaddress_2'. Considering that the hostname would not be changed frequently comparing to the ipaddress change. Do you agree with me?
>
> After seeing this comment, if this is a real world usecase then I feel the same is applicable to Cookie also ?
>
> Sijie Guo wrote:
> it doesn't matter how often it happens. if it happens, the tool need to be able to handle this case. using explicit bookie id is much more straightfoward and clean behavior, rather than implicit hostname & ipaddress switching.
>
> I don't remember how cookie command. but yes, it applies for cookie too. I do remember I made similar comment when reviewing cookie tool, no?
>
> Rakesh R wrote:
> Kindly refer the below comment:
> https://issues.apache.org/jira/browse/BOOKKEEPER-773?focusedCommentId=14143268&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14143268
>
>
> Initially I tried to make it more generic by taking bookieId from the user, but based on this comment it has been agreed upon to go ahead with the syntax,
>
> updatecookie -bookieId <hostname|ip>
>
> Sijie Guo wrote:
> ok. anyway, I am fine to stick to the constants right now. we might change it later.
ok. Thanks!
So I retained -bookieId <hostname|ip> options
- Rakesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/#review65458
-----------------------------------------------------------
On Jan. 1, 2015, 7:43 p.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28835/
> -----------------------------------------------------------
>
> (Updated Jan. 1, 2015, 7:43 p.m.)
>
>
> Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
>
>
> Bugs: BOKKEEPER-634
> https://issues.apache.org/jira/browse/BOKKEEPER-634
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> admin tool for changing the bookie's identifier to IP/hostname
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerCmdTest.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28835/diff/
>
>
> Testing
> -------
>
> Added few tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Sijie Guo <gu...@gmail.com>.
> On Dec. 18, 2014, 5:54 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java, line 1194
> > <https://reviews.apache.org/r/28835/diff/2/?file=792807#file792807line1194>
> >
> > The bookieId option is really weird. A much clear option would specify new bookieId, rather use 'ip' or 'hostname'.
> >
> > -1 on the option
>
> Rakesh R wrote:
> Followed same pattern of cookie updates. Make 'hostname' and 'ip' as constants, so the user could only specify "updatebookie -bookieId hostname" or "updatebookie -bookieId ip". Does this makes sense to you?
>
> Sijie Guo wrote:
> not at all. the cases to change bookie id are not just switching bookie id between ip and hostname. it also could be the case you need to change ip address.
>
> Rakesh R wrote:
> Thanks Sijie for this usecase. Can we suggest users to change the 'ipaddress_1' to 'hostname', instead of changing 'ipaddress_1' to another 'ipaddress_2'. Considering that the hostname would not be changed frequently comparing to the ipaddress change. Do you agree with me?
>
> After seeing this comment, if this is a real world usecase then I feel the same is applicable to Cookie also ?
it doesn't matter how often it happens. if it happens, the tool need to be able to handle this case. using explicit bookie id is much more straightfoward and clean behavior, rather than implicit hostname & ipaddress switching.
I don't remember how cookie command. but yes, it applies for cookie too. I do remember I made similar comment when reviewing cookie tool, no?
- Sijie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/#review65458
-----------------------------------------------------------
On Dec. 21, 2014, 5:52 p.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28835/
> -----------------------------------------------------------
>
> (Updated Dec. 21, 2014, 5:52 p.m.)
>
>
> Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
>
>
> Bugs: BOKKEEPER-634
> https://issues.apache.org/jira/browse/BOKKEEPER-634
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> admin tool for changing the bookie's identifier to IP/hostname
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28835/diff/
>
>
> Testing
> -------
>
> Added few tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Sijie Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/#review65458
-----------------------------------------------------------
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java
<https://reviews.apache.org/r/28835/#comment108606>
final
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java
<https://reviews.apache.org/r/28835/#comment108611>
The bookieId option is really weird. A much clear option would specify new bookieId, rather use 'ip' or 'hostname'.
-1 on the option
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java
<https://reviews.apache.org/r/28835/#comment108604>
trailing space
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java
<https://reviews.apache.org/r/28835/#comment108607>
please update this reflecting to new options.
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java
<https://reviews.apache.org/r/28835/#comment108608>
use '{}'
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java
<https://reviews.apache.org/r/28835/#comment108609>
inconsistent parameter
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java
<https://reviews.apache.org/r/28835/#comment108610>
update log statement to reflect correct parameter.
"+" -> ","
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java
<https://reviews.apache.org/r/28835/#comment108612>
trailing spaces
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java
<https://reviews.apache.org/r/28835/#comment108613>
static?
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java
<https://reviews.apache.org/r/28835/#comment108616>
move run method to a separate method
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java
<https://reviews.apache.org/r/28835/#comment108614>
ThreadFactoryBuilder
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java
<https://reviews.apache.org/r/28835/#comment108619>
outstandings
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java
<https://reviews.apache.org/r/28835/#comment108617>
why you need #get() here? isn't Runnable called only when readCb is completed?
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java
<https://reviews.apache.org/r/28835/#comment108618>
why #shutdownNow? not shutdown?
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java
<https://reviews.apache.org/r/28835/#comment108615>
in finally block
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java
<https://reviews.apache.org/r/28835/#comment108623>
this is totally wrong.
#addListener is "to Registers a listener to be run on the given executor. The listener will run when the Future's computation is complete or, if the computation is already complete, immediately."
in your implementation, you submit the runnable immediately, without checking if the future's compuation is complete or not.
you should use SettableFuture<Void>
private final static class ReadLedgerMetadataCb .. {
final SettableFuture<Void> resultFuture = ...
public addListener(...) {
resultFuture.addListener(...);
}
public void operationComplete(...) {
resultFuture.set(...)
}
}
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java
<https://reviews.apache.org/r/28835/#comment108620>
please remove newObject[]. you don't need this
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java
<https://reviews.apache.org/r/28835/#comment108624>
remove new Object[] {}
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java
<https://reviews.apache.org/r/28835/#comment108628>
why you need add any entries? it is just a metadata change. it'd better to not add any entries.
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java
<https://reviews.apache.org/r/28835/#comment108625>
please add Assert.assertFalse(ensemble.contains(curBookieAddr))
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java
<https://reviews.apache.org/r/28835/#comment108629>
change entries to 0
- Sijie Guo
On Dec. 16, 2014, 4:07 a.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28835/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2014, 4:07 a.m.)
>
>
> Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
>
>
> Bugs: BOKKEEPER-634
> https://issues.apache.org/jira/browse/BOKKEEPER-634
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> admin tool for changing the bookie's identifier to IP/hostname
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28835/diff/
>
>
> Testing
> -------
>
> Added few tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Sijie Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/#review65219
-----------------------------------------------------------
will review this later today
- Sijie Guo
On Dec. 16, 2014, 4:07 a.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28835/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2014, 4:07 a.m.)
>
>
> Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
>
>
> Bugs: BOKKEEPER-634
> https://issues.apache.org/jira/browse/BOKKEEPER-634
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> admin tool for changing the bookie's identifier to IP/hostname
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28835/diff/
>
>
> Testing
> -------
>
> Added few tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Rakesh R <ra...@huawei.com>.
> On Feb. 6, 2015, 7:22 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java, line 193
> > <https://reviews.apache.org/r/28835/diff/6/?file=816578#file816578line193>
> >
> > I don't like the idea to extend AbstractFuture. This is too tight with guava's internal implementation. I'd prefer using a variable for the settable future.
> >
> > class Read... implements GenericCallback {
> >
> > SettableFuture future = ...;
> >
> > public void operationComplete(..) {
> >
> > future.set(null);
> > // future.setException(..)
> >
> > }
> >
> > }
Thanks Sijie for the comment. Since this is closed, I've raised BOOKKEEPER-837 to address this.
- Rakesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/#review71391
-----------------------------------------------------------
On Jan. 12, 2015, 3:57 p.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28835/
> -----------------------------------------------------------
>
> (Updated Jan. 12, 2015, 3:57 p.m.)
>
>
> Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
>
>
> Bugs: BOKKEEPER-634
> https://issues.apache.org/jira/browse/BOKKEEPER-634
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> admin tool for changing the bookie's identifier to IP/hostname
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerCmdTest.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28835/diff/
>
>
> Testing
> -------
>
> Added few tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Sijie Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/#review71391
-----------------------------------------------------------
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java
<https://reviews.apache.org/r/28835/#comment117114>
I don't like the idea to extend AbstractFuture. This is too tight with guava's internal implementation. I'd prefer using a variable for the settable future.
class Read... implements GenericCallback {
SettableFuture future = ...;
public void operationComplete(..) {
future.set(null);
// future.setException(..)
}
}
- Sijie Guo
On Jan. 12, 2015, 3:57 p.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28835/
> -----------------------------------------------------------
>
> (Updated Jan. 12, 2015, 3:57 p.m.)
>
>
> Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
>
>
> Bugs: BOKKEEPER-634
> https://issues.apache.org/jira/browse/BOKKEEPER-634
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> admin tool for changing the bookie's identifier to IP/hostname
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerCmdTest.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28835/diff/
>
>
> Testing
> -------
>
> Added few tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Rakesh R <ra...@huawei.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/
-----------------------------------------------------------
(Updated Jan. 12, 2015, 3:57 p.m.)
Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
Bugs: BOKKEEPER-634
https://issues.apache.org/jira/browse/BOKKEEPER-634
Repository: bookkeeper-git
Description
-------
admin tool for changing the bookie's identifier to IP/hostname
Diffs (updated)
-----
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java PRE-CREATION
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerCmdTest.java PRE-CREATION
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/28835/diff/
Testing
-------
Added few tests
Thanks,
Rakesh R
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Rakesh R <ra...@huawei.com>.
> On Jan. 12, 2015, 3:28 p.m., Ivan Kelly wrote:
> > bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java, line 86
> > <https://reviews.apache.org/r/28835/diff/5/?file=816122#file816122line86>
> >
> > This causes test failures on my machine, and will on any machine where getLocalHostName() returns somethat resolves to 127.0.0.1 (most linux machines).
> >
> > Could you put baseConf.setAllowLoopback(true); in the constructor for BookKeeperClusterTestCase?
> >
> > It will fix the problem.
Thank you Ivan. It seems in windows machine it works fine.
- Rakesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/#review67649
-----------------------------------------------------------
On Jan. 11, 2015, 3:41 p.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28835/
> -----------------------------------------------------------
>
> (Updated Jan. 11, 2015, 3:41 p.m.)
>
>
> Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
>
>
> Bugs: BOKKEEPER-634
> https://issues.apache.org/jira/browse/BOKKEEPER-634
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> admin tool for changing the bookie's identifier to IP/hostname
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerCmdTest.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28835/diff/
>
>
> Testing
> -------
>
> Added few tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Ivan Kelly <iv...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/#review67649
-----------------------------------------------------------
I was committing when I found one issue. Once solved, i'll finish the commit.
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java
<https://reviews.apache.org/r/28835/#comment111713>
This causes test failures on my machine, and will on any machine where getLocalHostName() returns somethat resolves to 127.0.0.1 (most linux machines).
Could you put baseConf.setAllowLoopback(true); in the constructor for BookKeeperClusterTestCase?
It will fix the problem.
- Ivan Kelly
On Jan. 11, 2015, 3:41 p.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28835/
> -----------------------------------------------------------
>
> (Updated Jan. 11, 2015, 3:41 p.m.)
>
>
> Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
>
>
> Bugs: BOKKEEPER-634
> https://issues.apache.org/jira/browse/BOKKEEPER-634
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> admin tool for changing the bookie's identifier to IP/hostname
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerCmdTest.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28835/diff/
>
>
> Testing
> -------
>
> Added few tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Rakesh R <ra...@huawei.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/
-----------------------------------------------------------
(Updated Jan. 11, 2015, 3:41 p.m.)
Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
Bugs: BOKKEEPER-634
https://issues.apache.org/jira/browse/BOKKEEPER-634
Repository: bookkeeper-git
Description
-------
admin tool for changing the bookie's identifier to IP/hostname
Diffs (updated)
-----
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java PRE-CREATION
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerCmdTest.java PRE-CREATION
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/28835/diff/
Testing
-------
Added few tests
Thanks,
Rakesh R
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Rakesh R <ra...@huawei.com>.
> On Jan. 7, 2015, 1:11 p.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java, line 137
> > <https://reviews.apache.org/r/28835/diff/4/?file=805357#file805357line137>
> >
> > These exceptions should be thrown up to the caller.
Throws new IOException("Exception while updating ledger", e);. Also, handled #run() method exceptions and throws RuntimeException. any more comments?
- Rakesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/#review67018
-----------------------------------------------------------
On Jan. 11, 2015, 3:41 p.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28835/
> -----------------------------------------------------------
>
> (Updated Jan. 11, 2015, 3:41 p.m.)
>
>
> Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
>
>
> Bugs: BOKKEEPER-634
> https://issues.apache.org/jira/browse/BOKKEEPER-634
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> admin tool for changing the bookie's identifier to IP/hostname
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerCmdTest.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28835/diff/
>
>
> Testing
> -------
>
> Added few tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Ivan Kelly <iv...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/#review67018
-----------------------------------------------------------
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java
<https://reviews.apache.org/r/28835/#comment110844>
These exceptions should be thrown up to the caller.
- Ivan Kelly
On Jan. 1, 2015, 7:43 p.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28835/
> -----------------------------------------------------------
>
> (Updated Jan. 1, 2015, 7:43 p.m.)
>
>
> Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
>
>
> Bugs: BOKKEEPER-634
> https://issues.apache.org/jira/browse/BOKKEEPER-634
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> admin tool for changing the bookie's identifier to IP/hostname
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerCmdTest.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28835/diff/
>
>
> Testing
> -------
>
> Added few tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Rakesh R <ra...@huawei.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/
-----------------------------------------------------------
(Updated Jan. 1, 2015, 7:43 p.m.)
Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
Bugs: BOKKEEPER-634
https://issues.apache.org/jira/browse/BOKKEEPER-634
Repository: bookkeeper-git
Description
-------
admin tool for changing the bookie's identifier to IP/hostname
Diffs (updated)
-----
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java PRE-CREATION
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerCmdTest.java PRE-CREATION
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/28835/diff/
Testing
-------
Added few tests
Thanks,
Rakesh R
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Rakesh R <ra...@huawei.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/
-----------------------------------------------------------
(Updated Dec. 21, 2014, 5:52 p.m.)
Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
Bugs: BOKKEEPER-634
https://issues.apache.org/jira/browse/BOKKEEPER-634
Repository: bookkeeper-git
Description
-------
admin tool for changing the bookie's identifier to IP/hostname
Diffs (updated)
-----
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java PRE-CREATION
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/28835/diff/
Testing
-------
Added few tests
Thanks,
Rakesh R
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Rakesh R <ra...@huawei.com>.
> On Dec. 16, 2014, 11:24 a.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java, line 159
> > <https://reviews.apache.org/r/28835/diff/2/?file=792808#file792808line159>
> >
> > There's no need to have a separate method just to call another method. I would call progressable directly above.
ok
> On Dec. 16, 2014, 11:24 a.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java, line 1277
> > <https://reviews.apache.org/r/28835/diff/1-2/?file=786288#file786288line1277>
> >
> > trailing whitespace
ok
> On Dec. 16, 2014, 11:24 a.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java, line 84
> > <https://reviews.apache.org/r/28835/diff/2/?file=792808#file792808line84>
> >
> > Use guava threadfactorybuilder. Also, make the created thread a daemon thread, so that even if it does get leaked, it doesn't keep the process alive.
ok
> On Dec. 16, 2014, 11:24 a.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java, line 146
> > <https://reviews.apache.org/r/28835/diff/2/?file=792808#file792808line146>
> >
> > Need to reset interruptable status on thread. For the other exceptions, you can use the
> > catch (IOException|ExecutableException e)
> > syntax, as we compile to java7 now
ok
> On Dec. 16, 2014, 11:24 a.m., Ivan Kelly wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java, line 162
> > <https://reviews.apache.org/r/28835/diff/2/?file=792808#file792808line162>
> >
> > replace
> > });
> > with
> > }).get();
> >
> > Then the latch is completely unneeded.
ok
- Rakesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/#review65188
-----------------------------------------------------------
On Dec. 16, 2014, 4:07 a.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28835/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2014, 4:07 a.m.)
>
>
> Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
>
>
> Bugs: BOKKEEPER-634
> https://issues.apache.org/jira/browse/BOKKEEPER-634
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> admin tool for changing the bookie's identifier to IP/hostname
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28835/diff/
>
>
> Testing
> -------
>
> Added few tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Ivan Kelly <iv...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/#review65188
-----------------------------------------------------------
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java
<https://reviews.apache.org/r/28835/#comment108219>
trailing whitespace
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java
<https://reviews.apache.org/r/28835/#comment108217>
Use guava threadfactorybuilder. Also, make the created thread a daemon thread, so that even if it does get leaked, it doesn't keep the process alive.
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java
<https://reviews.apache.org/r/28835/#comment108215>
Need to reset interruptable status on thread. For the other exceptions, you can use the
catch (IOException|ExecutableException e)
syntax, as we compile to java7 now
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java
<https://reviews.apache.org/r/28835/#comment108218>
There's no need to have a separate method just to call another method. I would call progressable directly above.
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java
<https://reviews.apache.org/r/28835/#comment108216>
replace
});
with
}).get();
Then the latch is completely unneeded.
- Ivan Kelly
On Dec. 16, 2014, 4:07 a.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28835/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2014, 4:07 a.m.)
>
>
> Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
>
>
> Bugs: BOKKEEPER-634
> https://issues.apache.org/jira/browse/BOKKEEPER-634
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> admin tool for changing the bookie's identifier to IP/hostname
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/28835/diff/
>
>
> Testing
> -------
>
> Added few tests
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 28835: BOOKKEEPER-634: Provide admin tool to rename
bookie identifier in ledger metadata
Posted by Rakesh R <ra...@huawei.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28835/
-----------------------------------------------------------
(Updated Dec. 16, 2014, 4:07 a.m.)
Review request for bookkeeper, fpj, Ivan Kelly, and Sijie Guo.
Bugs: BOKKEEPER-634
https://issues.apache.org/jira/browse/BOKKEEPER-634
Repository: bookkeeper-git
Description
-------
admin tool for changing the bookie's identifier to IP/hostname
Diffs (updated)
-----
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java a1e4639
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/UpdateLedgerOp.java PRE-CREATION
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/UpdateLedgerOpTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/28835/diff/
Testing
-------
Added few tests
Thanks,
Rakesh R