-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBASE-28158 Decouple RIT list management from TRSP #7375
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
af75dcb to
99caa2c
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
...erver/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionInTransitionTracker.java
Show resolved
Hide resolved
| return regionInTransitionTracker.isRegionInTransition(regionInfo); | ||
| } | ||
|
|
||
| public int getOngoingTRSPCount() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Call it getInTransitCount()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Transit" means something different than "in transition". Javadoc the explaination.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about scheduledRegionTransition ?
hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java
Outdated
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| public boolean isInTransition() { | ||
| public boolean isOngoingTRSP() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about isInTransit?
And javadoc that "transit" means something different than "in transition".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about changing to isTransitionScheduled?
hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java
Outdated
Show resolved
Hide resolved
| removeRegionInTransition(regionInfo); | ||
| } | ||
|
|
||
| private List<RegionState.State> getExceptedRegionStates(RegionStateNode regionStateNode) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this method should be renamed and the code should be documented with comments. You are not really "excepting" regions.
You are basically waiting for the assignment state machine to complete and reach the desired terminal state by checking the region's current state against the table's state (ENABLED vs. DISABLED) to determine if the region is in the terminal state. If not, the region is added to the RIT list; otherwise, it is removed.
Your method naming and comments should communicate this theory of operation.
Related, there may be a minor race condition here. Consider if the table's state is changed (e.g., from ENABLED to DISABLING) at the same time a region for that table reports a state change. I think the tracker can momentarily use a stale table state here. However, this is self-correcting. As the region proceeds through its state transitions each subsequent call to the tracker will re-evaluate its status, and it will eventually be removed from RIT correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct me if I am wrong but I think this race condition might not occur as before changing the state of any table of region (or running any procedure) we first take a lock (setState using HBCK may be an exception). And this lock might save us from this case.
...erver/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionInTransitionTracker.java
Outdated
Show resolved
Hide resolved
...erver/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionInTransitionTracker.java
Outdated
Show resolved
Hide resolved
...erver/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionInTransitionTracker.java
Show resolved
Hide resolved
| static void removeNonDefaultReplicas(MasterProcedureEnv env, Stream<RegionInfo> regions, | ||
| int regionReplication) { | ||
| // Remove from in-memory states | ||
| // TODO should we not confirm here that replica region are closed or not ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@apurtell can you help me here? I was curious here why we are not confirming if replica regions are closed or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about this either.
0535506 to
1ebf4f7
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1ebf4f7 to
e439efc
Compare
This comment has been minimized.
This comment has been minimized.
b35e6b1 to
f1a268d
Compare
This comment has been minimized.
This comment has been minimized.
apurtell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good work and looks good to me. There is a checkstyle nit remaining to fix, please attend to that.
1c0feff to
c28d0fb
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Apache9
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still need lots of polishing, but in general I think the approach is OK, it does not change the normal transition logic, just changes the way on how we show regions as in transition.
| // splittingServersFromWALDir are being actively split -- the directory in the FS ends in | ||
| // '-SPLITTING'. Each splitting server should have a corresponding SCP. Log if not. | ||
| splittingServersFromWALDir.stream().filter(s -> !deadServersFromPE.contains(s)) | ||
| splittingServersFromWALDir.stream().filter(s -> !deadServersFromPE.containsKey(s)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we change the parameter from Set to a Map but we only use its containsKey method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are passing it to ServerManager where we are adding both the values to deadServermap (ref)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then better change the parameter name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Umeshkumar9414 Here, I've replied some of the comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping @Umeshkumar9414
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| * @param liveServersFromWALDir the live region servers from wal directory. | ||
| */ | ||
| void findDeadServersAndProcess(Set<ServerName> deadServersFromPE, | ||
| void findDeadServersAndProcess(Map<ServerName, Long> deadServersFromPE, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why just a parameter type change without chaging any real logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the value is the crash time? We should use it when setting up dead servers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes we should use it. We are using it in line 445 (deadservers::putIfAbsent).
I didn't need to change the code becuase lambda and method overloading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's change the parameter name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @Umeshkumar9414
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java
Outdated
Show resolved
Hide resolved
|
|
||
| private final int forceRegionRetainmentRetries; | ||
|
|
||
| private final RegionInTransitionTracker regionInTransitionTracker; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not put this into RegionStates? In this way we can reduce the code change i think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are couple of reasons for this
- I wanted regionTransitionTracker to follow meta (RegionStateStore in code) and regionStates doesn't do that. Sometime we update region state in memory and update meta in next step of procedure.
- regionInTransitionTracker needed tableStateManager and I doesn't wanted to pass tableStateManager to regionSates. Another option would be create tracker in AssignmentManager itself and then pass that in constructor.
Considering above I feel keeping this in AM is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could keep it here and also pass and store it in RegionStates, if we use it in both places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping @Umeshkumar9414
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not using it in RegionStates as of now.
I do thought of a use in RegionStateNode though, I was thinking to include activeTransitProcedureCount in RegionInTransitionTracker only. But then again in that way I had to pass it till RegionStateNode.
hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionStateNode.java
Outdated
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ServerCrashProcedure.java
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/master/DeadServer.java
Outdated
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java
Outdated
Show resolved
Hide resolved
795802e to
6f89ce9
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
💔 -1 overall
This message was automatically generated. |
|
TestSecureIPC test failed in the build, I was not able to run this test successfully in my machine for master branch as well, becuase of keytab issue. Looks like I might need some changes in my machine to run these tests, that I am not aware of. |
|
@Apache9 can you help me with review ? |
Apache9
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are almost done, thanks for the patience.
Now I'm OK with not putting RegionInTransitionStateTracker in RegionStates. The only left thing is about the isRegionInRegionStates method, do we really use this method in our code base? Can we just remove it?
| // AssignmentManager calls setTableStateManager once hbase:meta is confirmed online, if it is | ||
| // still null it means confirmation is still pending. One should not access TableStateManger | ||
| // till the time. | ||
| if (TableName.isMetaTableName(tableName)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe here we could add an assert TableName.isMetaTableName(tableName);, and then return true?
Anyway, throwing a RuntimeException is also acceptable as in production environment we may disable assertion.
Better add something like "CODE-BUG" to tell developers that you may write code wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somehow I try to avoid if else and liked the idea of assert. Will do that.
hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionStates.java
Outdated
Show resolved
Hide resolved
hbase-server/src/main/resources/hbase-webapps/master/deadRegionServers.jsp
Outdated
Show resolved
Hide resolved
…ion of meta online
c84ed4d to
4af272b
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Apache9
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1.
Thanks @Umeshkumar9414!
|
I'm OK with merging it. I'm in a business trip in Singapore, so let me see if I can get a device to merge and cherry-pick this to all branches... Thanks. |
| ServerName crashedServerName = scp.getServerName(); | ||
| for (RegionInfo regionInfo : regionsOnCrashedServer) { | ||
| RegionStateNode node = regionStates.getOrCreateRegionStateNode(regionInfo); | ||
| if (node.getRegionLocation() == crashedServerName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this always guaranteed to be comparable via ==? Or do we have to, or would it be safer to, use equals?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, it should be equals, and we should log a warn when they do not equal, as this method is called before we assign any regions on the crashed region server, so the regionsOnCrashedServer should all be on the crashed server.
Let me prepare an addendum. Thanks @d-c-manning !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @d-c-manning, @Apache9 let me know if I can help here.
|
Hi @Apache9, @apurtell , @virajjasani while building the phoenix with these changes I got to know that Phoenix uses |
|
@Umeshkumar9414 the usage seems to be in the test code so we can remove it if it is not really necessary, or use JMX metrics to find out RIT count in the test |
|
RegionStates is annotated |
Raised an JIRA https://issues.apache.org/jira/browse/PHOENIX-7741 and solved it in phoenix. |
No description provided.