d-c-manning commented on a change in pull request #3826:
URL: https://github.com/apache/hbase/pull/3826#discussion_r744990831
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
##########
@@ -938,6 +938,8 @@ private void finishActiveMasterInitialization(MonitoredTask
status)
// Set master as 'initialized'.
setInitialized(true);
+ this.assignmentManager.deleteNonZkBasedQualifiersForZkBasedAssignment();
Review comment:
I'm not intimately familiar with the startup process, but is it going to
be concerning to do this after `setInitialized(true);`? Will there be race
conditions where we can start processing assignment before we have cleaned meta?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
##########
@@ -3343,6 +3346,55 @@ boolean waitUntilNoRegionsInTransition(final long
timeout)
return offlineServers;
}
+ void deleteNonZkBasedQualifiersForZkBasedAssignment() throws IOException {
+ boolean isZKAssignmentInUse =
+ ConfigUtil.useZKForAssignment(server.getConfiguration()) &&
!server.getConfiguration()
+ .getBoolean("hbase.assignment.usezk.migrating", false);
+ if (isZKAssignmentInUse) {
+ List<Result> results =
MetaTableAccessor.fullScanOfMeta(server.getConnection());
+ List<Delete> redundantCQDeletes = new ArrayList<>();
+ for (Result result : results) {
+ RegionLocations rl = MetaTableAccessor.getRegionLocations(result);
+ if (rl == null) {
+ continue;
+ }
+ HRegionLocation[] locations = rl.getRegionLocations();
+ if (locations == null) {
+ continue;
+ }
+ for (HRegionLocation hrl : locations) {
+ if (hrl == null) {
+ continue;
+ }
+ HRegionInfo regionInfo = hrl.getRegionInfo();
+ if (regionInfo == null) {
+ continue;
+ }
+ int replicaId = regionInfo.getReplicaId();
+ Cell cell = result.getColumnLatestCell(HConstants.CATALOG_FAMILY,
+ RegionStateStore.getServerNameColumn(replicaId));
+ if (cell != null && cell.getValueLength() > 0) {
+ Delete delete =
+ new Delete(cell.getRowArray(), cell.getRowOffset(),
cell.getRowLength());
+ delete.addColumns(HConstants.CATALOG_FAMILY,
HConstants.SERVERNAME_QUALIFIER);
Review comment:
is this sufficient for replicas? Doesn't it need to be
```suggestion
delete.addColumns(HConstants.CATALOG_FAMILY,
RegionStateStore.getServerNameColumn(replicaId));
```
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
##########
@@ -3343,6 +3346,55 @@ boolean waitUntilNoRegionsInTransition(final long
timeout)
return offlineServers;
}
+ void deleteNonZkBasedQualifiersForZkBasedAssignment() throws IOException {
+ boolean isZKAssignmentInUse =
+ ConfigUtil.useZKForAssignment(server.getConfiguration()) &&
!server.getConfiguration()
+ .getBoolean("hbase.assignment.usezk.migrating", false);
+ if (isZKAssignmentInUse) {
+ List<Result> results =
MetaTableAccessor.fullScanOfMeta(server.getConnection());
+ List<Delete> redundantCQDeletes = new ArrayList<>();
+ for (Result result : results) {
+ RegionLocations rl = MetaTableAccessor.getRegionLocations(result);
+ if (rl == null) {
+ continue;
+ }
+ HRegionLocation[] locations = rl.getRegionLocations();
+ if (locations == null) {
+ continue;
+ }
+ for (HRegionLocation hrl : locations) {
+ if (hrl == null) {
+ continue;
+ }
+ HRegionInfo regionInfo = hrl.getRegionInfo();
+ if (regionInfo == null) {
+ continue;
+ }
+ int replicaId = regionInfo.getReplicaId();
+ Cell cell = result.getColumnLatestCell(HConstants.CATALOG_FAMILY,
+ RegionStateStore.getServerNameColumn(replicaId));
+ if (cell != null && cell.getValueLength() > 0) {
+ Delete delete =
+ new Delete(cell.getRowArray(), cell.getRowOffset(),
cell.getRowLength());
+ delete.addColumns(HConstants.CATALOG_FAMILY,
HConstants.SERVERNAME_QUALIFIER);
+ redundantCQDeletes.add(delete);
+ }
+ cell = result.getColumnLatestCell(HConstants.CATALOG_FAMILY,
+ RegionStateStore.getStateColumn(replicaId));
+ if (cell != null && cell.getValueLength() > 0) {
+ Delete delete =
+ new Delete(cell.getRowArray(), cell.getRowOffset(),
cell.getRowLength());
+ delete.addColumns(HConstants.CATALOG_FAMILY,
HConstants.STATE_QUALIFIER);
+ redundantCQDeletes.add(delete);
+ }
+ }
+ }
+ if (!redundantCQDeletes.isEmpty()) {
+ MetaTableAccessor.deleteFromMetaTable(server.getConnection(),
redundantCQDeletes);
Review comment:
Can we `LOG.info` something in this method, since it's a pretty
interesting operation that will occur.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
##########
@@ -3343,6 +3346,55 @@ boolean waitUntilNoRegionsInTransition(final long
timeout)
return offlineServers;
}
+ void deleteNonZkBasedQualifiersForZkBasedAssignment() throws IOException {
+ boolean isZKAssignmentInUse =
+ ConfigUtil.useZKForAssignment(server.getConfiguration()) &&
!server.getConfiguration()
+ .getBoolean("hbase.assignment.usezk.migrating", false);
+ if (isZKAssignmentInUse) {
+ List<Result> results =
MetaTableAccessor.fullScanOfMeta(server.getConnection());
+ List<Delete> redundantCQDeletes = new ArrayList<>();
+ for (Result result : results) {
+ RegionLocations rl = MetaTableAccessor.getRegionLocations(result);
+ if (rl == null) {
+ continue;
Review comment:
are these `continue` statements ever expected? Do they at least deserve
a `LOG` statement at `WARN` or at least `INFO`?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/RegionStateStore.java
##########
@@ -73,9 +73,11 @@
* @return A ServerName instance or {@link HRegionInfo#getServerName(Result)}
* if necessary fields not found or empty.
*/
- static ServerName getRegionServer(final Result r, int replicaId) {
+ static ServerName getRegionServer(final Result r, int replicaId,
Configuration config) {
Cell cell = r.getColumnLatestCell(HConstants.CATALOG_FAMILY,
getServerNameColumn(replicaId));
- if (cell == null || cell.getValueLength() == 0) {
+ boolean isZKAssignmentInUse = ConfigUtil.useZKForAssignment(config) &&
!config
+ .getBoolean("hbase.assignment.usezk.migrating", false);
+ if (cell == null || cell.getValueLength() == 0 || isZKAssignmentInUse) {
Review comment:
this is a change in logic from what was there before. Before, we would
use `info:sn` if it was present, even while `usezk.migrating` is `true`. Are we
saying it's better to change the behavior so we don't use it at all until
`usezk` is set to `false`? And the purpose of the `migrating` flag is only to
populate the `info:sn` and `info:state` entries, and not to use them at all? I
just wanted to make sure I understood this change correctly.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
