wchevreuil commented on code in PR #6448:
URL: https://github.com/apache/hbase/pull/6448#discussion_r2095882466
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ModifyTableProcedure.java:
##########
@@ -506,14 +506,17 @@ private void
removeReplicaColumnsIfNeeded(MasterProcedureEnv env) throws IOExcep
private void assignNewReplicasIfNeeded(MasterProcedureEnv env) throws
IOException {
final int oldReplicaCount =
unmodifiedTableDescriptor.getRegionReplication();
final int newReplicaCount = modifiedTableDescriptor.getRegionReplication();
- if (newReplicaCount <= oldReplicaCount) {
+ int existingReplicasCount = env.getAssignmentManager().getRegionStates()
+ .getRegionsOfTable(modifiedTableDescriptor.getTableName()).size();
+ if (newReplicaCount <= Math.min(oldReplicaCount, existingReplicasCount)) {
Review Comment:
Why do we need to consider the ZK info here? Shouldn't the desc diff be
enough?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java:
##########
@@ -1171,30 +1171,30 @@ private void finishActiveMasterInitialization() throws
IOException, InterruptedE
int replicasNumInConf =
conf.getInt(HConstants.META_REPLICAS_NUM,
HConstants.DEFAULT_META_REPLICA_NUM);
TableDescriptor metaDesc =
tableDescriptors.get(TableName.META_TABLE_NAME);
+ int existingReplicasCount =
+
assignmentManager.getRegionStates().getRegionsOfTable(TableName.META_TABLE_NAME).size();
+
if (metaDesc.getRegionReplication() != replicasNumInConf) {
// it is possible that we already have some replicas before upgrading,
so we must set the
// region replication number in meta TableDescriptor directly first,
without creating a
// ModifyTableProcedure, otherwise it may cause a double assign for
the meta replicas.
- int existingReplicasCount =
-
assignmentManager.getRegionStates().getRegionsOfTable(TableName.META_TABLE_NAME).size();
- if (existingReplicasCount > metaDesc.getRegionReplication()) {
- LOG.info("Update replica count of hbase:meta from {}(in
TableDescriptor)"
- + " to {}(existing ZNodes)", metaDesc.getRegionReplication(),
existingReplicasCount);
- metaDesc = TableDescriptorBuilder.newBuilder(metaDesc)
+ LOG.info("Update replica count of hbase:meta from {}(in
TableDescriptor)"
+ + " to {}(existing ZNodes)", metaDesc.getRegionReplication(),
existingReplicasCount);
+ metaDesc = TableDescriptorBuilder.newBuilder(metaDesc)
.setRegionReplication(existingReplicasCount).build();
- tableDescriptors.update(metaDesc);
- }
- // check again, and issue a ModifyTableProcedure if needed
- if (metaDesc.getRegionReplication() != replicasNumInConf) {
- LOG.info(
- "The {} config is {} while the replica count in TableDescriptor is
{}"
- + " for hbase:meta, altering...",
- HConstants.META_REPLICAS_NUM, replicasNumInConf,
metaDesc.getRegionReplication());
- procedureExecutor.submitProcedure(new ModifyTableProcedure(
- procedureExecutor.getEnvironment(),
TableDescriptorBuilder.newBuilder(metaDesc)
- .setRegionReplication(replicasNumInConf).build(),
- null, metaDesc, false, true));
- }
+ tableDescriptors.update(metaDesc);
+ }
+ // check again, and issue a ModifyTableProcedure if needed
+ if (metaDesc.getRegionReplication() != replicasNumInConf
+ || existingReplicasCount != metaDesc.getRegionReplication()) {
+ LOG.info(
+ "The {} config is {} while the replica count in TableDescriptor is
{}"
+ + " for hbase:meta, altering...",
+ HConstants.META_REPLICAS_NUM, replicasNumInConf,
metaDesc.getRegionReplication());
Review Comment:
Let's also log the number of replicas seen on ZK (existingReplicasCount).
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ModifyTableProcedure.java:
##########
@@ -506,14 +506,17 @@ private void
removeReplicaColumnsIfNeeded(MasterProcedureEnv env) throws IOExcep
private void assignNewReplicasIfNeeded(MasterProcedureEnv env) throws
IOException {
final int oldReplicaCount =
unmodifiedTableDescriptor.getRegionReplication();
final int newReplicaCount = modifiedTableDescriptor.getRegionReplication();
- if (newReplicaCount <= oldReplicaCount) {
+ int existingReplicasCount = env.getAssignmentManager().getRegionStates()
+ .getRegionsOfTable(modifiedTableDescriptor.getTableName()).size();
+ if (newReplicaCount <= Math.min(oldReplicaCount, existingReplicasCount)) {
return;
}
if (isTableEnabled(env)) {
List<RegionInfo> newReplicas =
env.getAssignmentManager().getRegionStates()
.getRegionsOfTable(getTableName()).stream().filter(RegionReplicaUtil::isDefaultReplica)
- .flatMap(primaryRegion -> IntStream.range(oldReplicaCount,
newReplicaCount).mapToObj(
- replicaId ->
RegionReplicaUtil.getRegionInfoForReplica(primaryRegion, replicaId)))
+ .flatMap(primaryRegion -> IntStream.range(Math.min(oldReplicaCount,
existingReplicasCount),
Review Comment:
Same as above.
--
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]
