sodonnel commented on code in PR #9955:
URL: https://github.com/apache/ozone/pull/9955#discussion_r2989830961
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/upgrade/FinalizationStateManagerImpl.java:
##########
@@ -59,152 +57,51 @@ protected FinalizationStateManagerImpl(Builder builder)
throws IOException {
this.upgradeFinalizer = builder.upgradeFinalizer;
this.versionManager = this.upgradeFinalizer.getVersionManager();
this.checkpointLock = new ReentrantReadWriteLock();
- initialize();
- }
-
- private void initialize() throws IOException {
- this.hasFinalizingMark =
- finalizationStore.isExist(OzoneConsts.FINALIZING_KEY);
- }
-
- private void publishCheckpoint(FinalizationCheckpoint checkpoint) {
- // Move the upgrade status according to this checkpoint. This is sent
- // back to the client if they query for the current upgrade status.
- versionManager.setUpgradeState(checkpoint.getStatus());
- // Set the checkpoint in the SCM context so other components can read it.
- upgradeContext.getSCMContext().setFinalizationCheckpoint(checkpoint);
}
@Override
public void setUpgradeContext(SCMUpgradeFinalizationContext context) {
this.upgradeContext = context;
- FinalizationCheckpoint checkpoint = getFinalizationCheckpoint();
- upgradeContext.getSCMContext().setFinalizationCheckpoint(checkpoint);
- // Set the version manager's upgrade status (sent back to the client to
- // identify upgrade progress) based on the current checkpoint.
- versionManager.setUpgradeState(checkpoint.getStatus());
- }
-
- @Override
- public void addFinalizingMark() throws IOException {
- checkpointLock.writeLock().lock();
- try {
- hasFinalizingMark = true;
- } finally {
- checkpointLock.writeLock().unlock();
- }
- transactionBuffer.addToBuffer(finalizationStore,
- OzoneConsts.FINALIZING_KEY, "");
- publishCheckpoint(FinalizationCheckpoint.FINALIZATION_STARTED);
}
@Override
public void finalizeLayoutFeatures(Integer toVersion) throws IOException {
- int startLayoutVersion = versionManager.getMetadataLayoutVersion() + 1;
- for (int version = startLayoutVersion; version <= toVersion; version++) {
- finalizeLayoutFeatureLocal(version);
+ for (LayoutFeature feature : versionManager.unfinalizedFeatures()) {
+ finalizeLayoutFeatureLocal((HDDSLayoutFeature) feature);
}
}
/**
* A version of finalizeLayoutFeature without the {@link Replicate}
* annotation that can be called by followers to finalize from a snapshot.
*/
- private void finalizeLayoutFeatureLocal(Integer layoutVersion)
+ private void finalizeLayoutFeatureLocal(HDDSLayoutFeature layoutFeature)
throws IOException {
checkpointLock.writeLock().lock();
try {
// The VERSION file is the source of truth for the current layout
// version. This is updated in the replicated finalization steps.
// Layout version will be written to the DB as well so followers can
// finalize from a snapshot.
- if (versionManager.getMetadataLayoutVersion() >= layoutVersion) {
+ if (versionManager.getMetadataLayoutVersion() >=
layoutFeature.layoutVersion()) {
LOG.warn("Attempting to finalize layout feature for layout version {},
but " +
"current metadata layout version is {}. Skipping finalization for
this layout version.",
- layoutVersion, versionManager.getMetadataLayoutVersion());
+ layoutFeature.layoutVersion(),
versionManager.getMetadataLayoutVersion());
} else {
- HDDSLayoutFeature feature =
- (HDDSLayoutFeature) versionManager.getFeature(layoutVersion);
- upgradeFinalizer.replicatedFinalizationSteps(feature, upgradeContext);
+ upgradeFinalizer.replicatedFinalizationSteps(layoutFeature,
upgradeContext);
}
} finally {
checkpointLock.writeLock().unlock();
}
- if (!versionManager.needsFinalization()) {
- publishCheckpoint(FinalizationCheckpoint.MLV_EQUALS_SLV);
+ if (!versionManager.needsFinalization() &&
!upgradeContext.getSCMContext().isLeader()) {
+ // Only the followers complete finalize here, the leader must wait until
the DNs
+ // have finalized before making finalization done, otherwise a polling
client could
+ // be told it is complete too early.
Review Comment:
Adding this was actually suggested by co-pilot after I asked it to check for
more dead code. But I think you are correct - I can remove it.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
