ndimiduk commented on a change in pull request #917: HBASE-23383 [hbck2]
`fixHoles` should queue assignment procedures for any regions its fixing
URL: https://github.com/apache/hbase/pull/917#discussion_r356330879
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java
##########
@@ -724,24 +731,57 @@ static int compare(TransitRegionStateProcedure left,
TransitRegionStateProcedure
return RegionInfo.COMPARATOR.compare(left.getRegion(), right.getRegion());
}
- private TransitRegionStateProcedure createAssignProcedure(RegionStateNode
regionNode,
- ServerName targetServer, boolean override) {
- TransitRegionStateProcedure proc;
+ private TransitRegionStateProcedure createAssignProcedure(final RegionInfo
regionInfo)
+ throws HBaseIOException {
+ return createAssignProcedure(regionInfo, null);
+ }
+
+ private TransitRegionStateProcedure createAssignProcedure(final RegionInfo
regionInfo,
+ final ServerName targetServer) throws HBaseIOException {
+ // TODO: should we use getRegionStateNode?
+ final RegionStateNode regionNode =
regionStates.getOrCreateRegionStateNode(regionInfo);
+ return createAssignProcedure(regionNode, targetServer, false);
+ }
+
+ /**
+ * Queue a new {@link TransitRegionStateProcedure} representing this
assignment request.
+ * @param regionNode the region to assign
+ * @param targetServer the target server for this assignment (optional)
+ * @param override when {@code true}, remove any existing procedure found on
{@code regionNode}
+ */
+ private TransitRegionStateProcedure createAssignProcedure(final
RegionStateNode regionNode,
+ final ServerName targetServer, final boolean override) throws
HBaseIOException {
+ final TransitRegionStateProcedure proc;
regionNode.lock();
try {
- if(override && regionNode.getProcedure() != null) {
+ if (override && regionNode.getProcedure() != null) {
regionNode.unsetProcedure(regionNode.getProcedure());
}
assert regionNode.getProcedure() == null;
- proc = TransitRegionStateProcedure.assign(getProcedureEnvironment(),
- regionNode.getRegionInfo(), targetServer);
+ preTransitCheck(regionNode, STATES_EXPECTED_ON_ASSIGN);
Review comment:
So this is a great question. Looking a (what should be javadoc) on this
symbol, it says
> This is for manually scheduled region assign, can add other states later
if we find out other usages
Here I've promoted it to a general case. That's not the right thing to do
without adding more states. Sadly, I'm not seeing obvious additions based on
the errors. I think the only way to test for them right now is to tease them
out via a CM test. I'll need to study the existing code to figure out what the
actual state machine is, and thus what states should be listed here.
----------------------------------------------------------------
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:
[email protected]
With regards,
Apache Git Services
