HBASE-30142 Resolve NPE while running recoverUnknown command because null RegionLocation#8192
Open
Umeshkumar9414 wants to merge 3 commits into
Open
HBASE-30142 Resolve NPE while running recoverUnknown command because null RegionLocation#8192Umeshkumar9414 wants to merge 3 commits into
Umeshkumar9414 wants to merge 3 commits into
Conversation
wchevreuil
reviewed
May 5, 2026
Contributor
wchevreuil
left a comment
wchevreuil
left a comment
There was a problem hiding this comment.
Makes sense. Should we add UT for this?
Umeshkumar9414
force-pushed
the
HBASE-30142
branch
from
6e21a3a to
314f55e
Compare
Contributor
Author
@wchevreuil Added an test. Let me know if it looks good. |
mnpoonia
approved these changes
May 12, 2026
There was a problem hiding this comment.
Pull request overview
Fixes HBASE-30142 by ensuring recoverUnknown/scheduleSCPsForUnknownServers does not treat regions with a null ServerName (i.e., null region location during transitions/failed-open handling) as “unknown servers,” avoiding spurious SCP scheduling and potential NPEs.
Changes:
- Update
ServerManager#isServerUnknownto returnfalsefornullserver names. - Add a new master-level mini-cluster test that drives a region into
ABNORMALLY_CLOSEDwith anulllocation and verifiesscheduleSCPsForUnknownServersschedules no SCPs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
hbase-server/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java |
Adjusts “unknown server” classification to ignore null server names and updates Javadoc. |
hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestRecoverUnknownWithNullRegionLocation.java |
Adds regression coverage for recoverUnknown when a region’s location is null but state is non-OFFLINE. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+983
to
+986
| * any more, for example, a very old previous instance). A null serverName should not be | ||
| * considered unknown. We set regionLocation null before finding the assign candidate (in-between | ||
| * region transition) or while marking it OFFLINE/FAILED_OPEN For more info regarding null-check | ||
| * in this refer HBASE-30142 |
Comment on lines
+101
to
+107
| assertTrue(node.isInState(RegionState.State.ABNORMALLY_CLOSED), | ||
| "regionClosedAbnormally must move state to ABNORMALLY_CLOSED"); | ||
| assertNull(node.getRegionLocation(), "regionClosedAbnormally must null out the location"); | ||
|
|
||
| MasterProtos.ScheduleSCPsForUnknownServersResponse response = | ||
| master.getMasterRpcServices().scheduleSCPsForUnknownServers(null, | ||
| MasterProtos.ScheduleSCPsForUnknownServersRequest.newBuilder().build()); |
Contributor
There was a problem hiding this comment.
This is a good suggestion.
Umeshkumar9414
force-pushed
the
HBASE-30142
branch
from
314f55e to
4488d62
Compare
apurtell
requested changes
May 29, 2026
Contributor
apurtell
left a comment
apurtell
left a comment
There was a problem hiding this comment.
Test classification needs a change, otherwise seems ok
| * {@code shouldSubmitSCP(null)} then dereferenced the null and crashed. | ||
| */ | ||
| @Tag(MasterTests.TAG) | ||
| @Tag(SmallTests.TAG) |
Contributor
There was a problem hiding this comment.
I think this needs to be @Tag(MediumTests.TAG) because it runs a minicluster.
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
MasterRpcServices.scheduleSCPsForUnknownServers() has no null guard before calling isServerUnknown(). With the old code, regions with null serverName (OFFLINE/FAILED_OPEN in-between state) would spuriously trigger a ServerCrashProcedure. The fix prevents that.