-
Notifications
You must be signed in to change notification settings - Fork 3.3k
HBASE-29291: Add a command to refresh/sync hbase:meta table #7058
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
base: HBASE-29081
Are you sure you want to change the base?
Conversation
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
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.
LGTM and have few minor comments, let me try to use the copilot to have another round of verification
@@ -742,7 +742,7 @@ private static void deleteFromMetaTable(final Connection connection, final Delet | |||
* @param connection connection we're using | |||
* @param deletes Deletes to add to hbase:meta This list should support #remove. | |||
*/ | |||
private static void deleteFromMetaTable(final Connection connection, final List<Delete> deletes) | |||
public static void deleteFromMetaTable(final Connection connection, final List<Delete> deletes) |
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 change for testing purpose?
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.
Not really, I added in case we need to delete regions from the meta table during refresh.
private boolean isSystemDirectory(String dirName) { | ||
return dirName.startsWith(".") || dirName.startsWith("-") || dirName.startsWith("_"); | ||
} |
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.
nit: do we still have system directory start with _
? is it _logs
?
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.
Well, I wasn't sure about that. So I just added it just in case.
Set<RegionInfo> latestSet = new HashSet<>(latest); | ||
|
||
// Check for size or set differences | ||
if (currentSet.size() != latestSet.size() || !currentSet.equals(latestSet)) { |
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.
nit: Set
's equals should have compare the size before calling the containsAll
if (currentSet.size() != latestSet.size() || !currentSet.equals(latestSet)) { | |
if (!currentSet.equals(latestSet)) { |
// Check for boundary changes | ||
for (RegionInfo cr : current) { | ||
RegionInfo lr = latest.stream() | ||
.filter(r -> r.getRegionNameAsString().equals(cr.getRegionNameAsString())) | ||
.findFirst() | ||
.orElse(null); | ||
|
||
if (lr == null || hasBoundaryChanged(cr, lr)) { | ||
LOG.info("Region mismatch or boundary changed for {}", cr.getRegionNameAsString()); | ||
return true; | ||
} | ||
} | ||
return false; | ||
} |
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.
nit: just in case of large amount of regions need to be synced, let's optimize this code for better looping from worst case of O(N^2) to O(N).
// Check for boundary changes | |
for (RegionInfo cr : current) { | |
RegionInfo lr = latest.stream() | |
.filter(r -> r.getRegionNameAsString().equals(cr.getRegionNameAsString())) | |
.findFirst() | |
.orElse(null); | |
if (lr == null || hasBoundaryChanged(cr, lr)) { | |
LOG.info("Region mismatch or boundary changed for {}", cr.getRegionNameAsString()); | |
return true; | |
} | |
} | |
return false; | |
} | |
// Check for boundary changes | |
Map<String, RegionInfo> latestMap = latest.stream() | |
.collect(Collectors.toMap(RegionInfo::getRegionNameAsString, Function.identity())); | |
for (RegionInfo cr : current) { | |
RegionInfo lr = latestMap.get(cr.getRegionNameAsString()); | |
if (lr == null || hasBoundaryChanged(cr, lr)) { | |
LOG.info("Region mismatch or boundary changed for {}", cr.getRegionNameAsString()); | |
return true; | |
} | |
} | |
return false; | |
} |
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.
Fair point. And afaik there will not be duplicate regions. I will make this change.
if (c.getEnvironment().getRegionInfo().getTable().isSystemTable()) { | ||
return; | ||
} |
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.
should this and below changes in ReadOnlyController be out of this PR? or is this blocking the refreshMeta ?
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 blocking the refresh_meta. We need to be able to put and delete in the system table (hbase:meta). So I made these changes in the ReadOnlyController.
currentRegions = getCurrentRegions(env.getMasterServices().getConnection()); | ||
LOG.info("Found {} current regions in meta table", | ||
currentRegions != null ? currentRegions.size() : 0); |
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.
nit: getCurrentRegions by
calling MetaTableAccessor.getAllRegions
will return empty set instead of null, is this null check required?
currentRegions = getCurrentRegions(env.getMasterServices().getConnection()); | |
LOG.info("Found {} current regions in meta table", | |
currentRegions != null ? currentRegions.size() : 0); | |
currentRegions = getCurrentRegions(env.getMasterServices().getConnection()); | |
LOG.info("Found {} current regions in meta table", currentRegions.size()); |
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.
Pull Request Overview
This pull request implements a new command to refresh/sync the hbase:meta table, addressing Jira ticket HBASE-29291. The changes include adding a refreshMeta method across various Admin interfaces and RPC endpoints, integration and unit tests for the new procedure, and corresponding protocol buffer definitions and updates.
- Added refreshMeta methods in HBase admin and client classes.
- Introduced new tests and integration procedures for the meta table refresh workflow.
- Updated RPC and protobuf definitions to support the refreshMeta command.
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
hbase-thrift/src/main/java/org/apache/hadoop/hbase/thrift2/client/ThriftAdmin.java | Added a refreshMeta stub throwing NotImplementedException |
hbase-server/src/test/java/org/apache/hadoop/hbase/rsgroup/VerifyingRSGroupAdmin.java | Delegated refreshMeta call to underlying admin |
hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestRefreshMetaProcedureIntegration.java | Integrated end-to-end test for meta refresh behavior |
hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestRefreshMetaProcedure.java | Added various unit tests for RefreshMetaProcedure functionality |
hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/ReadOnlyController.java | Allowed deletes on system tables during read-only mode |
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java | Implemented RPC endpoint for refreshMeta |
hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java | Added refreshMeta implementation using RefreshMetaProcedure |
hbase-server/src/main/java/org/apache/hadoop/hbase/MetaTableAccessor.java | Changed deleteFromMetaTable method visibility to public |
hbase-protocol-shaded/src/main/protobuf/server/master/MasterProcedure.proto | Added RefreshMeta state definitions |
hbase-protocol-shaded/src/main/protobuf/server/master/Master.proto | Added RefreshMeta request/response messages |
hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java | Added async refreshMeta call |
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncHBaseAdmin.java | Exposed async refreshMeta in client API |
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncAdmin.java | Added refreshMeta to AsyncAdmin interface |
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java | Exposed refreshMeta over AsyncAdmin |
hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java | Added refreshMeta method in the Admin API |
Comments suppressed due to low confidence (2)
hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestRefreshMetaProcedureIntegration.java:168
- [nitpick] The fixed wait time of 3000 ms might be insufficient on slower environments, potentially leading to flaky tests. Consider increasing the timeout or making it configurable.
TEST_UTIL.waitFor(3000, () -> {
hbase-server/src/main/java/org/apache/hadoop/hbase/MetaTableAccessor.java:745
- The visibility of deleteFromMetaTable has been changed from private to public. Please confirm that exposing this method aligns with the overall API design and does not introduce unintended access.
public static void deleteFromMetaTable(final Connection connection, final List<Delete> deletes)
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.
nit: one more thought, we don't need to address in this PR, but assuming one cluster with 100k regions, would this refresh_meta for all tables still work?
@taklwu - Thanks for the review!
Well, it becomes a heavy operation, and that's something we should test for. I'm thinking this refresh_meta implementation will be a fallback operation, in case we need to refresh_meta manually without depending on the active cluster persisting updates. |
Jira: https://issues.apache.org/jira/browse/HBASE-29291