Uh oh!
There was an error while loading. Please reload this page.
HBASE-27551 Add config options to delay assignment to retain last region location - #4945
Merged
Conversation
wchevreuil
requested review from
Apache9, ankitsinghal, pankaj72981, petersomogyi and taklwuJanuary 5, 2023 13:01
Apache-HBase
commented
Jan 5, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 5, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 5, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 5, 2023
🎊 +1 overall
This message was automatically generated. |
taklwu
reviewed
Jan 5, 2023
taklwu
left a comment
Contributor
There was a problem hiding this comment.
to clarify, we try to assign the region with queueAssign before we find the region server was dead , such it will not issue the SCP , right?
| long retries = 0; | ||
| while (!isOnline && retries < forceRegionRetainmentRetries) { | ||
| try { | ||
| synchronized (this) { |
Contributor
There was a problem hiding this comment.
does it mean there may have multiple procedure tries to use this checkAndWaitForOriginalServer ?
ContributorAuthor
There was a problem hiding this comment.
Not really, this would only be called by each TRSP individually. Removing this synchronized block on next commit.
Apache-HBase
commented
Jan 5, 2023
💔 -1 overall
This message was automatically generated. |
wchevreuil
commented
Jan 6, 2023
ContributorAuthor
Not really, in fact an SCP triggers a TRSP which will call |
Apache-HBase
commented
Jan 6, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 6, 2023
💔 -1 overall
This message was automatically generated. |
| public static final String FORCE_REGION_RETAINMENT_WAIT = | ||
| "hbase.master.scp.retain.assignment.force.wait"; | ||
| public static final long DEFAULT_FORCE_REGION_RETAINMENT_WAIT = 500; |
Contributor
There was a problem hiding this comment.
I first read this config as how long we should wait for this server to come online. However, the code uses this config to sleep for this period between checks, so the overall wait time is FORCE_REGION_RETAINMENT_WAIT * FORCE_REGION_RETAINMENT_RETRIES.
From the operator's perspective, I think it is better to manage this with overall wait time.
ContributorAuthor
There was a problem hiding this comment.
A single overall wait time would be less efficient, we would forcibly wait for the whole of the given timeout, even if the RS comes back online much sooner than that. Maybe better leave as this, but put comments on this constant to avoid confusion?
Contributor
There was a problem hiding this comment.
Documentation/comment is definitely good to have here. Is it costly to check the RS's availability? Maybe hbase.master.scp.retain.assignment.force.wait can be set to a small value so configuring it might not be that important.
Apache-HBase
commented
Jan 6, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 7, 2023
💔 -1 overall
This message was automatically generated. |
Apache9
commented
Jan 8, 2023
Contributor
Will take a look in the next few days. Please give me some time~ |
Apache-HBase
commented
Jan 9, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 9, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 9, 2023
🎊 +1 overall
This message was automatically generated. |
Change-Id: I58eba4f382974f73167fc8cd2bf1bb4f3d5be0a9
Apache-HBase
commented
Jan 9, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 9, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 9, 2023
💔 -1 overall
This message was automatically generated. |
Apache9
reviewed
Jan 11, 2023
| throws ProcedureSuspendedException { | ||
| boolean isOnline = serverManager.findServerWithSameHostnamePortWithLock(lastHost) != null; | ||
| long retries = 0; | ||
| while (!isOnline && retries < forceRegionRetainmentRetries) { |
Contributor
There was a problem hiding this comment.
In this way the procedure will hang here for a very long time without releasing the procedure worker. Better suspend the procedure and reschedule it again later.
ContributorAuthor
There was a problem hiding this comment.
Yes. Thanks for the suggestion, I had changed this in the last commit.
| private boolean forceRegionRetainment; | ||
| private ServerManager serverManager; |
Contributor
There was a problem hiding this comment.
ServerManager can be gotten from MasterProcedureEnv, so we do not need to store it here.
ContributorAuthor
There was a problem hiding this comment.
Problem here is that we don't keep any MasterProcedureEnv ref as class attribute.
Contributor
There was a problem hiding this comment.
When executing you will always have a MasterProcedureEnv...
| private boolean isSplit; | ||
| private boolean forceRegionRetainment; |
Contributor
There was a problem hiding this comment.
All the below fields should not be stored here, when reloading we will use the default constructor to create a procedure and use deserialize method to restore the fields, so if you want to store them here, you need to serialize them, or you should implement the afterReplay method to initialize them...
ContributorAuthor
There was a problem hiding this comment.
Thanks for pointing this out. Since these are all config readings, decided to go with the afterReplay overriding option.
Contributor
There was a problem hiding this comment.
Looking at other configs in TRSP, I prefer we store these configs in AssignmentManager, just like
private final boolean shouldAssignRegionsWithFavoredNodes;
private final int assignDispatchWaitQueueMaxSize;
private final int assignDispatchWaitMillis;
private final int assignMaxAttempts;
private final int assignRetryImmediatelyMaxAttempts;
Change-Id: Ie13da5dfad3969c1477eac0e37f808051c884bff
Apache-HBase
commented
Jan 12, 2023
🎊 +1 overall
This message was automatically generated. |
wchevreuil
commented
Jan 17, 2023
ContributorAuthor
Hi @Apache9@petersomogyi@taklwu , any other comments/suggestions? |
Apache-HBase
commented
Jan 17, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 17, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 18, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 18, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 18, 2023
🎊 +1 overall
This message was automatically generated. |
Apache9
reviewed
Jan 18, 2023
| private boolean forceRegionRetainment; | ||
| private ServerManager serverManager; |
Contributor
There was a problem hiding this comment.
When executing you will always have a MasterProcedureEnv...
| private boolean isSplit; | ||
| private boolean forceRegionRetainment; |
Contributor
There was a problem hiding this comment.
Looking at other configs in TRSP, I prefer we store these configs in AssignmentManager, just like
private final boolean shouldAssignRegionsWithFavoredNodes;
private final int assignDispatchWaitQueueMaxSize;
private final int assignDispatchWaitMillis;
private final int assignMaxAttempts;
private final int assignRetryImmediatelyMaxAttempts;
| retries++; | ||
| LOG.info("Suspending the TRSP PID={} because {} is true and previous host {} " | ||
| + "for region is not yet online.", this.getProcId(), FORCE_REGION_RETAINMENT, lastHost); | ||
| setTimeout(forceRegionRetainmentWait); |
Contributor
There was a problem hiding this comment.
So the forceRegionRetainmentWait here is interval? Not total time?
ContributorAuthor
There was a problem hiding this comment.
Yes, the wait interval between attempts, not the total time.
wchevreuil
commented
Jan 18, 2023
ContributorAuthor
Thanks for the comments, @Apache9 , I have applied your latest suggestions. Let me know if this is good to go. |
Apache-HBase
commented
Jan 18, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 18, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 18, 2023
🎊 +1 overall
This message was automatically generated. |
Apache9
reviewed
Jan 20, 2023
| public static final boolean DEFAULT_FORCE_REGION_RETAINMENT = false; | ||
| /** The wait time in millis before checking again if the region's previous RS is back online */ | ||
| public static final String FORCE_REGION_RETAINMENT_WAIT = |
Uh oh!
There was an error while loading. Please reload this page.
| retries++; | ||
| LOG.info("Suspending the TRSP PID={} because {} is true and previous host {} " | ||
| + "for region is not yet online.", this.getProcId(), FORCE_REGION_RETAINMENT, lastHost); | ||
| setTimeout(env.getAssignmentManager().getForceRegionRetainmentWait()); |
Contributor
There was a problem hiding this comment.
So here we do not want to use Exponential backoff?
ContributorAuthor
There was a problem hiding this comment.
Switched to use RetryCounter and exponential backoff.
wchevreuil
commented
Jan 20, 2023
ContributorAuthor
Thanks for the latest suggestions @Apache9 , I had pushed a new commit addressing those. |
Apache-HBase
commented
Jan 20, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 20, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 20, 2023
🎊 +1 overall
This message was automatically generated. |
wchevreuil
commented
Jan 23, 2023
ContributorAuthor
Hi @Apache9 , just checking if the latest commit is good to go. |
Apache9
approved these changes
Jan 23, 2023
wchevreuil added a commit
that referenced
this pull request
Jan 24, 2023
…ion location (#4945) Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org> Signed-off-by: Duo Zhang <zhangduo@apache.org>
vinayakphegde pushed a commit
to vinayakphegde/hbase
that referenced
this pull request
Apr 4, 2024
…ion location (apache#4945) Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org> Signed-off-by: Duo Zhang <zhangduo@apache.org> Change-Id: Ie92e5f00e9c9efacb939f9810002a9a1edeeb3ff
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.