You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@bookkeeper.apache.org by GitBox <gi...@apache.org> on 2021/02/10 07:35:26 UTC
[GitHub] [bookkeeper] michaeljmarshall opened a new pull request #2597: Issue 2506: Skip unavailable bookies during verifyLedgerFragment
michaeljmarshall opened a new pull request #2597:
URL: https://github.com/apache/bookkeeper/pull/2597
Descriptions of the changes in this PR:
This PR improves the `verifyLedgerFragment` method in the `LedgerChecker` by skipping calls to bookies that are known to be unavailable. The "bad bookies" are calculated by using metadata available in ZK and accessed by the `BookKeeperAdmin`. Note that `verifyLedgerFragment` will still run checks on all other bookies that appear available.
### Motivation
The motivation for this change is demonstrated in #2506. As this code currently works, there are a ton of calls made to unavailable bookies with the intent of calculating bad (unavailable) bookies. This proposed change would greatly decrease the number of calls that the auditor and the replicator need to make to calculate which ledgers need replicating.
### Changes
1. Added `getUnavailableBookies` method to the `BookKeeperAdmin`. This method could attempt to use caching, but it's not actually called that often, so I think caching might not add complexity without much value.
2. Updated `verifyLedgerFragment` method signature to take a collection of `unavailableBookies`.
Master Issue: #2506
### Testing
If these changes are acceptable, I'd like some help identifying the best way to test these changes. I already added some coverage for the `getUnavailableBookies` method, but I haven't explicitly tested the fundamental change this PR proposes. Thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] michaeljmarshall commented on a change in pull request #2597: Issue 2506: Skip unavailable bookies during verifyLedgerFragment
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #2597:
URL: https://github.com/apache/bookkeeper/pull/2597#discussion_r576497226
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookieWatcherImpl.java
##########
@@ -173,6 +173,22 @@ public BookieAddressResolver getBookieAddressResolver() {
}
}
+ /**
+ * Determine if a bookie should be considered unavailable.
+ * This does not require a network call because this class
+ * maintains a current view of readonly and writable bookies.
+ * An unavailable bookie is one that is neither read only nor
+ * writable.
+ *
+ * @param id
+ * Bookie to check
+ * @return whether or not the given bookie is unavailable
+ */
+ @Override
+ public boolean isBookieUnavailable(BookieId id) {
+ return !readOnlyBookies.contains(id) && !writableBookies.contains(id);
Review comment:
There is any need to add more concurrency controls here. `readOnlyBookies` and `writableBookies` are already declared as `volatile`, which will ensure that this method call gets the latest values for these variables.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] michaeljmarshall commented on pull request #2597: Issue 2506: Skip unavailable bookies during verifyLedgerFragment
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #2597:
URL: https://github.com/apache/bookkeeper/pull/2597#issuecomment-778030524
@dlg99 - thanks for the detailed response, that was really helpful. I have something in the works that should be close to what you've described. It seems like scope limitations might require me to pass the `BookKeeper` class and then pass the `BookieWatcher` class, but nevertheless, I was able to add the logic you suggested, which is great.
Regarding the original test failure, I dug into this for a while tonight, and it looks like the logic that my PR proposes does not fully reach feature parity with the current BK behavior. It appears that ledgers with no entries end up classifying bookies as `OK`. I need to sign off for the night, but at this point, I think I should be able to fix up this PR by the end of the weekend and have it ready for review on Monday.
Here's the logic where we consider those bookies `OK`:
https://github.com/apache/bookkeeper/blob/462ff9d32a3870271421df04fbae3aba9427ad4b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerChecker.java#L190-L209
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] michaeljmarshall commented on pull request #2597: Issue 2506: Skip unavailable bookies during verifyLedgerFragment
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #2597:
URL: https://github.com/apache/bookkeeper/pull/2597#issuecomment-779523018
With the latest update, the code change is much simpler. I still need to figure out how best to test this code. It is clear that it was already being tested because it made that one test fail, but it'd be good to add tests for the `BookieWatcherImpl` class to ensure the logic for the `isBookieUnavailable` method is correct as well as test to ensure its usage is correct in the `LedgerChecker` class. I'll take another look at adding tests later tonight.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] michaeljmarshall commented on pull request #2597: Issue 2506: Skip unavailable bookies during verifyLedgerFragment
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #2597:
URL: https://github.com/apache/bookkeeper/pull/2597#issuecomment-780390525
> We are going to release 4.13.0.
In 4.13.0 is not a big release with breaking changes so I believe we can ask to upgrade BK to 4.13.0 in 2.7.1.
Most of the commits in 4.13 are about StreamStorage service and Pulsar is the only user
I asked @sijie about this during Tuesday's pulsar community meeting, and he mentioned the info I included above. Either approach works for me.
Let me know if I can do anything regarding cherry picking the commit. (I'm not sure if that's only an option for committers or if I can submit some kind of PR.)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on pull request #2597: Issue 2506: Skip unavailable bookies during verifyLedgerFragment
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #2597:
URL: https://github.com/apache/bookkeeper/pull/2597#issuecomment-780336264
@nicoloboschi @aluccaroni you will enjoy this patch as well. It is very useful for your usecases
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] michaeljmarshall commented on a change in pull request #2597: Issue 2506: Skip unavailable bookies during verifyLedgerFragment
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #2597:
URL: https://github.com/apache/bookkeeper/pull/2597#discussion_r574018100
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/Auditor.java
##########
@@ -1197,6 +1197,8 @@ void checkAllLedgers() throws BKException, IOException, InterruptedException, Ke
final CompletableFuture<Void> processFuture = new CompletableFuture<>();
+ Collection<BookieId> unavailableBookies = localAdmin.getUnavailableBookies();
Review comment:
Thanks for the feedback. I hadn't realized this task was long running, and your suggestion makes sense. I'll take a look at updating this PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] michaeljmarshall commented on pull request #2597: Issue 2506: Skip unavailable bookies during verifyLedgerFragment
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #2597:
URL: https://github.com/apache/bookkeeper/pull/2597#issuecomment-779639708
@eolivelli PTAL, thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] dlg99 edited a comment on pull request #2597: Issue 2506: Skip unavailable bookies during verifyLedgerFragment
Posted by GitBox <gi...@apache.org>.
dlg99 edited a comment on pull request #2597:
URL: https://github.com/apache/bookkeeper/pull/2597#issuecomment-777687525
> I am wondering about the role of `writableBookies` and `readOnlyBookies` collections declared on lines 107 and 108. I see that those are updated reactively based on listeners. Would it make sense to add a collection for `allBookies`, add an associated listener, and then compute the `unavailableBookies` based on those three collections? I find this solution interesting because it wouldn't require any calls to ZK to calculate `unavailableBookies` and it would likely be pretty accurate.
I don't think this is necessary. You can simply add `isBookieAvailable(bookieId)` which will check if the bookie is either writable or readable.
After all, you calculated unavailable as (all except (writable + readable)). Send request to bookies only if isBookieAvailable() is true.
Pass bookieWatcher, do this check for each bookie, whenever needed. it is a dictionary lookup.
`writableBookies` is a set of all bookies that are up and writable, `readOnlyBookies` - similar, but read-only.
Bookies register in ZK (if ZK is used) and the lists updated by listeners when statuses change/bookies crash etc.
these aren't guaranteed to be immediately updated - i.e. it takes ZK some time to detect that bookie crashed and update registration.
`getAllBookies()` IIRC - walks Zk path (long operation) to get set of all bookies in cluster (ever joined the cluster, not decommissioned)
`quarantinedBookies` - Zk does not consider them to be down but there were problems communicating with them. I.e. networking timeout/error, reconnect that caused ensemble change. I don't think you should worry about this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] dlg99 commented on a change in pull request #2597: Issue 2506: Skip unavailable bookies during verifyLedgerFragment
Posted by GitBox <gi...@apache.org>.
dlg99 commented on a change in pull request #2597:
URL: https://github.com/apache/bookkeeper/pull/2597#discussion_r574002177
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/Auditor.java
##########
@@ -1197,6 +1197,8 @@ void checkAllLedgers() throws BKException, IOException, InterruptedException, Ke
final CompletableFuture<Void> processFuture = new CompletableFuture<>();
+ Collection<BookieId> unavailableBookies = localAdmin.getUnavailableBookies();
Review comment:
IIRC, checkAllLedger() potentially is a lengthy process.
The bookies in the cluster can go up or down while it is running; marking a bookie as unavailable at the beginning might be too aggressive.
Imagine this scenario:
- rack-aware data placement; 1 mil of ledger to check
- some kind of maintenance starts in the cluster (os update, bookie upgrade etc) and it is done on per-rack basis
- rack 1 is down
- check ledger starts and puts all bookies from rack 1 into the list of unavailable bookies
- after ledger 1000 is checked, the rack 1 is up, rack 2 is down
- check ledgers assumes that rack 1 bookies are down, cannot talk to the rack 2 bookies
Using BookieWatcher (and extending it) to check for the bookie availability as-needed sounds like a better approach compared to doing it once in the beginning.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] michaeljmarshall commented on pull request #2597: Issue 2506: Skip unavailable bookies during verifyLedgerFragment
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #2597:
URL: https://github.com/apache/bookkeeper/pull/2597#issuecomment-779606151
Added tests.
Note that the optimization in this PR does not affect the code here: https://github.com/apache/bookkeeper/blob/eb9ed7f2325d04466a86f9248c4dd9d00bb2aab9/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerChecker.java#L397-L401
It might be valuable to add logic to optimize that call so that there are not unnecessary calls to unavailable bookkeeper nodes. I only just noticed that call tonight, so it might still be valuable to merge this PR before worrying about this code block.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] michaeljmarshall commented on pull request #2597: Issue 2506: Skip unavailable bookies during verifyLedgerFragment
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #2597:
URL: https://github.com/apache/bookkeeper/pull/2597#issuecomment-777226863
> Why aren't you using BookieWatcher in order leverage the internal cache of available bookies ?
I think I might be missing something.
My implementation uses the `BookKeeperAdmin`, which is using the `BookieWatcherImpl`. Would you prefer that I just get to the `BookieWatcherImpl` by using the `BookKeeper` class? Or are you just referring to how my admin method uses the bookie watcher?
Regarding caches, are you referring to the cache on line 105 below or something like the stored collections of wr/ro bookies?
https://github.com/apache/bookkeeper/blob/057f85ec1b977379938350f12d7ec99ff1162f29/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookieWatcherImpl.java#L104-L108
I am wondering about the role of `writableBookies` and `readOnlyBookies` collections declared on lines 107 and 108. I see that those are updated reactively based on listeners. Would it make sense to add a collection for `allBookies`, add an associated listener, and then compute the `unavailableBookies` based on those three collections? I find this solution interesting because it wouldn't require any calls to ZK to calculate `unavailableBookies` and it would likely be pretty accurate.
My concern about a cache is that the logic using the `getUnavailableBookies` result relies on the accuracy of that collection to skip unnecessary calls to bookies that won't respond. Given that a bookie could become unavailable If a bookie is incorrectly marked as unavailable, that would lead to unnecessary replication. If a bookie is incorrectly marked as available, the recovery process could make _many_ unnecessary calls. The latter is the current behavior, and I've seen my auto recovery pods get some very high cpu usage while trying to reach those unavailable bookies. If you think a cache is the right way to go, would you mind sharing a little more detail about implementation? Thanks!
> probably you can go with Mockito and verify that we are not trying to connect to the unavailable bookies
Perfect, thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on pull request #2597: Issue 2506: Skip unavailable bookies during verifyLedgerFragment
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #2597:
URL: https://github.com/apache/bookkeeper/pull/2597#issuecomment-780423604
I will try to cherry pick. If we have problems then I will ask you to send a PR against your favourite branch
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] michaeljmarshall commented on a change in pull request #2597: Issue 2506: Skip unavailable bookies during verifyLedgerFragment
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #2597:
URL: https://github.com/apache/bookkeeper/pull/2597#discussion_r576497226
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookieWatcherImpl.java
##########
@@ -173,6 +173,22 @@ public BookieAddressResolver getBookieAddressResolver() {
}
}
+ /**
+ * Determine if a bookie should be considered unavailable.
+ * This does not require a network call because this class
+ * maintains a current view of readonly and writable bookies.
+ * An unavailable bookie is one that is neither read only nor
+ * writable.
+ *
+ * @param id
+ * Bookie to check
+ * @return whether or not the given bookie is unavailable
+ */
+ @Override
+ public boolean isBookieUnavailable(BookieId id) {
+ return !readOnlyBookies.contains(id) && !writableBookies.contains(id);
Review comment:
Just want to note that I don't believe there is any need to add more concurrency controls here. `readOnlyBookies` and `writableBookies` are already declared as `volatile`, which will ensure that this method call gets the latest values for these variables.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli merged pull request #2597: Issue 2506: Skip unavailable bookies during verifyLedgerFragment
Posted by GitBox <gi...@apache.org>.
eolivelli merged pull request #2597:
URL: https://github.com/apache/bookkeeper/pull/2597
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] dlg99 commented on pull request #2597: Issue 2506: Skip unavailable bookies during verifyLedgerFragment
Posted by GitBox <gi...@apache.org>.
dlg99 commented on pull request #2597:
URL: https://github.com/apache/bookkeeper/pull/2597#issuecomment-777687525
> I am wondering about the role of `writableBookies` and `readOnlyBookies` collections declared on lines 107 and 108. I see that those are updated reactively based on listeners. Would it make sense to add a collection for `allBookies`, add an associated listener, and then compute the `unavailableBookies` based on those three collections? I find this solution interesting because it wouldn't require any calls to ZK to calculate `unavailableBookies` and it would likely be pretty accurate.
I don't think this is necessary. You can simply add `isBookieAvailable()` which will check if the bookie is either writable or readable.
After all, you calculated unavailable as (all except (writable + readable)). Send request to bookies only if isBookieAvailable() is true.
Pass bookieWatcher, do this check for each bookie, whenever needed. it is a dictionary lookup.
`writableBookies` is a set of all bookies that are up and writable, `readOnlyBookies` - similar, but read-only.
Bookies register in ZK (if ZK is used) and the lists updated by listeners when statuses change/bookies crash etc.
these aren't guaranteed to be immediately updated - i.e. it takes ZK some time to detect that bookie crashed and update registration.
`getAllBookies()` IIRC - walks Zk path (long operation) to get set of all bookies in cluster (ever joined the cluster, not decommissioned)
`quarantinedBookies` - Zk does not consider them to be down but there were problems communicating with them. I.e. networking timeout/error, reconnect that caused ensemble change. I don't think you should worry about this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] michaeljmarshall commented on a change in pull request #2597: Issue 2506: Skip unavailable bookies during verifyLedgerFragment
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #2597:
URL: https://github.com/apache/bookkeeper/pull/2597#discussion_r576494535
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/Auditor.java
##########
@@ -1197,6 +1197,8 @@ void checkAllLedgers() throws BKException, IOException, InterruptedException, Ke
final CompletableFuture<Void> processFuture = new CompletableFuture<>();
+ Collection<BookieId> unavailableBookies = localAdmin.getUnavailableBookies();
Review comment:
I took your comments into consideration for the latest commit. I think it's much cleaner.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on pull request #2597: Issue 2506: Skip unavailable bookies during verifyLedgerFragment
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #2597:
URL: https://github.com/apache/bookkeeper/pull/2597#issuecomment-776526745
Very useful work !
The overall approach is good to me.
One question:
Why aren't you using BookieWatcher in order leverage the internal cache of available bookies ?
Regarding tests:
probably you can go with Mockito and verify that we are not trying to connect to the unavailable bookies
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] michaeljmarshall commented on pull request #2597: Issue 2506: Skip unavailable bookies during verifyLedgerFragment
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #2597:
URL: https://github.com/apache/bookkeeper/pull/2597#issuecomment-780296429
@eolivelli and @dlg99 - is there any plan to do an additional 4.12 patch release of bookkeeper? Given that pulsar doesn't bump minor versions of bookkeeper in their patch releases, this won't be live for pulsar for at least a month, and even then, it'll only be in pulsar 2.8.x and not in 2.7.x. If there will be a patch release, it'd be great to cherry pick this and add it to a 4.12 patch update.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on pull request #2597: Issue 2506: Skip unavailable bookies during verifyLedgerFragment
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #2597:
URL: https://github.com/apache/bookkeeper/pull/2597#issuecomment-780335836
@michaeljmarshall we can pick this change to 4.12 sure.
We are going to release 4.13.0.
In 4.13.0 is not a big release with breaking changes so I believe we can ask to upgrade BK to 4.13.0 in 2.7.1.
Most of the commits in 4.13 are about StreamStorage service and Pulsar is the only user
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] michaeljmarshall commented on pull request #2597: Issue 2506: Skip unavailable bookies during verifyLedgerFragment
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #2597:
URL: https://github.com/apache/bookkeeper/pull/2597#issuecomment-776523352
The test failure is related to my changes. I'll take a closer look tomorrow.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org