Uh oh!
There was an error while loading. Please reload this page.
HBASE-25815 RSGroupBasedLoadBalancer online status never updates after being set to true for the first time - #3606
Closed
caroliney14 wants to merge 3 commits into
Closed
HBASE-25815 RSGroupBasedLoadBalancer online status never updates after being set to true for the first time#3606caroliney14 wants to merge 3 commits into
caroliney14 wants to merge 3 commits into
Conversation
…r being set to true for the first time
Apache-HBase
commented
Aug 19, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 19, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 19, 2021
💔 -1 overall
This message was automatically generated. |
| @@ -825,6 +827,20 @@ private void createRSGroupTable() throws IOException { | |||
| } | |||
| public boolean isOnline() { | |||
Contributor
There was a problem hiding this comment.
How often is this check run?
ContributorAuthor
There was a problem hiding this comment.
@saintstack It's called by RSGroupBasedLoadBalancer#balanceCluster (here), RSGroupInfoManagerImpl#refresh (here), and RSGroupInfoManagerImpl#flushConfig (here) so I would think semi-frequently? Not sure how often balanceCluster gets called, but refresh gets called upon RSGroupInfoManagerImpl startup and flushConfig gets called every time we add/remove servers or tables or change the rsgroups in any way.
Apache-HBase
commented
Aug 21, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 21, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 21, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 24, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 24, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 24, 2021
🎊 +1 overall
This message was automatically generated. |
Apache9
reviewed
Aug 25, 2021
| return; | ||
| } | ||
| // Make changes visible |
ContributorAuthor
There was a problem hiding this comment.
Because generally it is the case to update in-memory before persistent storage
Contributor
There was a problem hiding this comment.
@caroliney14 even if we don't change this order, are we still good with the main problem? I was wondering if this particular order of in-memory vs persistent update could be taken up in follow-up PR as well if we are good with the latest change in isOnline() method.
| @@ -825,6 +829,20 @@ private void createRSGroupTable() throws IOException { | |||
| } | |||
| public boolean isOnline() { | |||
Contributor
There was a problem hiding this comment.
I guess the intention here is we will not come back to offline mode after online?
ContributorAuthor
There was a problem hiding this comment.
I guess that was the original intention, but it seems misleading/strange. We have encountered errors in prod resulting from trying to flush to hbase:rsgroup when the table became unavailable after the initial check and set to "online" -- in that case, we should go the offline path, right? (Albeit this was in HBase 1, so I'm not 100% sure if HBase 2+ hasn't fixed this -- but from my understanding of the code, it hasn't?)
Contributor
There was a problem hiding this comment.
I think this part of code is written by me but I can not recall if it was already like this or I changed the implementation to make it only online once. Give some time to check code for different branches...
Contributor
There was a problem hiding this comment.
OK, it was not me. It was like this when we first introduced this class in HBASE-6721. Even on branch-1 backport, the implementation is just return the isOnline flag. So if you want to change the implementation, please explain a bit about the reason?
ContributorAuthor
There was a problem hiding this comment.
The reason is that if we do not periodically update the online status to reflect the availability of the hbase:rsgroup table, we could become blocked waiting on a flush to the hbase:rsgroup table when it can't be accessed (e.g. it's stuck in transition, offline, the rs hosting it has queueing, etc.). Each rsgroup functionality (add, move servers, move tables, remove, etc.) is synchronized, and furthermore the multiMutate function which does the persisting to hbase:rsgroup uses Future.get without timeout. So if hbase:rsgroup is unavailable we will keep getting blocked until the client times out, and we will be unable to serve another rsgroup request in the meantime, when we could have exited early by checking for the availability of hbase:rsgroup.
Instead of being blocked waiting like this, we can go through an "offline" code path. There already is an offline code path in flushConfig which only updates the in-memory state of the default group (here), but we could also change it so that it updates in-memory state while asynchronously trying to persist it to hbase:rsgroup in the background.
Please correct me if I misunderstood anything. What do you think about this rationale?
Contributor
There was a problem hiding this comment.
The logic seems good to me. @Apache9 Would you like to verify?
Apache-HBase
commented
Oct 26, 2021
💔 -1 overall
This message was automatically generated. |
2 similar comments
Apache-HBase
commented
Oct 26, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Oct 26, 2021
💔 -1 overall
This message was automatically generated. |
virajjasani
commented
Dec 2, 2021
Contributor
@caroliney14 can you please rebase once? Thanks |
virajjasani
left a comment
Contributor
There was a problem hiding this comment.
Left one comment, let's see how QA goes after latest rebase
| return; | ||
| } | ||
| // Make changes visible |
Contributor
There was a problem hiding this comment.
@caroliney14 even if we don't change this order, are we still good with the main problem? I was wondering if this particular order of in-memory vs persistent update could be taken up in follow-up PR as well if we are good with the latest change in isOnline() method.
virajjasani
left a comment
Contributor
There was a problem hiding this comment.
I see one problem here. We should remove online = true from waitForGroupTableOnline():
RSGroupInfoManagerImpl.this.refresh(true);
online = true;
Here, refresh(true) is already taking care of updating online flag. If it updates it to false, we should avoid resetting it to true.
| online = true; | ||
| } | ||
| } catch (Exception e) { | ||
| LOG.warn("Failed to read from " + RSGROUP_TABLE_NAME+ "; setting online = false"); |
Contributor
There was a problem hiding this comment.
nit: use log placeholder {} for table name?
| @@ -825,6 +829,20 @@ private void createRSGroupTable() throws IOException { | |||
| } | |||
| public boolean isOnline() { | |||
Contributor
There was a problem hiding this comment.
The logic seems good to me. @Apache9 Would you like to verify?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.