errose28 commented on code in PR #11800:
URL: https://github.com/apache/ozone/pull/11800#discussion_r4473912162
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/ozone/upgrade/RatisBasedVersionManager.java:
##########
@@ -46,14 +46,32 @@ public void validateDBVersion(Table<String, String>
finalizationStore) throws IO
}
}
- public void finalizeFromSnapshotIfRequired(Table<String, String>
finalizationStore) throws IOException {
+ public void finalizeFromDBIfRequired(Table<String, String> finalizationStore)
Review Comment:
I took a lot more time workshop this method and went through a few
iterations with claude. I'm reasonably confident about this implementation but
please review it. Key points:
- Every potential range the DB version can land in is now enumerated and
given at least a comment indicating what is expected.
- Ratis log replay happens async on startup, so we cannot guarantee that it
has finished when this method runs. This means apparent version < DB version is
valid. The new implementation treats this as an expected case instead of
logging or blocking until log replay finishes.
- The only unexpected case we truly can't handle and need to crash on is if
the version in the DB is larger than our known software version.
- For the other unexpected case where the DB version is between our
apparent and software versions, the safest thing to do is partially finalize to
that version.
- Choosing either our apparent or software versions instead risks
running mixed apparent versions in the quorum.
- Throwing in this case would lead to an unrecoverable error if a bug
ever allows this to happen.
Test cases will need to be updated to test a DB version in every possible
range relative to apparent and software versions.
```diff
diff --git
a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/ozone/upgrade/ComponentVersionManager.java
b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/ozone/upgrade/ComponentVersionManager.java
index 5aaa48aec1..125dc51474 100644
---
a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/ozone/upgrade/ComponentVersionManager.java
+++
b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/ozone/upgrade/ComponentVersionManager.java
@@ -90,8 +90,22 @@ public boolean needsFinalization() {
public abstract int getPersistedApparentVersion();
public void finalizeUpgrade() throws UpgradeException {
+ finalizeUpgrade(softwareVersion);
+ }
+
+ /**
+ * Finalizes each version after the current apparent version, up to and
including {@code targetVersion}. Does nothing
+ * if {@code targetVersion} is already finalized.
+ *
+ * @param targetVersion The last version to finalize. Must be supported
by the software version.
+ */
+ public void finalizeUpgrade(ComponentVersion targetVersion) throws
UpgradeException {
+ if (!targetVersion.isSupportedBy(softwareVersion)) {
+ throw new IllegalArgumentException("Cannot finalize to version " +
targetVersion
+ + " which is newer than software version " + softwareVersion +
".");
+ }
ComponentVersion prevVersion = apparentVersion;
- for (ComponentVersion newVersion : getUnfinalizedVersions()) {
+ for (ComponentVersion newVersion :
getUnfinalizedVersions(targetVersion)) {
validateForFinalization(newVersion);
runUpgradeAction(newVersion);
persistApparentVersion(newVersion, prevVersion);
@@ -110,16 +124,18 @@ public void finalizeUpgrade() throws UpgradeException {
public abstract ComponentVersion getVersionForClient();
/**
- * @return An Iterable of all versions after the current apparent version
which still need to be finalized. If this
- * component is already finalized, the Iterable will be empty.
+ * @return An Iterable of all versions after the current apparent
version, up to and including
+ * {@code targetVersion}, which still need to be finalized. If {@code
targetVersion} is already finalized, the
+ * Iterable will be empty.
*/
- private Iterable<ComponentVersion> getUnfinalizedVersions() {
+ private Iterable<ComponentVersion>
getUnfinalizedVersions(ComponentVersion targetVersion) {
return () -> new Iterator<ComponentVersion>() {
private ComponentVersion currentVersion = apparentVersion;
@Override
public boolean hasNext() {
- return currentVersion.nextVersion() != null;
+ ComponentVersion nextVersion = currentVersion.nextVersion();
+ return nextVersion != null &&
nextVersion.isSupportedBy(targetVersion);
}
@Override
diff --git
a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/ozone/upgrade/RatisBasedVersionManager.java
b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/ozone/upgrade/RatisBasedVersionManager.java
index a1dd5baa56..573b371993 100644
---
a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/ozone/upgrade/RatisBasedVersionManager.java
+++
b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/ozone/upgrade/RatisBasedVersionManager.java
@@ -46,33 +46,44 @@ public void validateDBVersion(Table<String, String>
finalizationStore) throws IO
}
}
- public void finalizeFromDBIfRequired(Table<String, String>
finalizationStore)
- throws IOException {
- ComponentVersion apparentVersionInNewDB =
getApparentVersionInDB(finalizationStore);
- if (apparentVersionInNewDB == null) {
- LOG.info("Component has apparent version {} but no apparent version
was found in the DB. That is expected if " +
+ public void finalizeFromDBIfRequired(Table<String, String>
finalizationStore) throws IOException {
+ // This call is expected to throw and crash the process if the apparent
version in the DB is larger than our
+ // software version, which we cannot support.
+ ComponentVersion apparentVersionInDB =
getApparentVersionInDB(finalizationStore);
+ if (apparentVersionInDB == null) {
+ LOG.debug("Component has apparent version {} but no apparent version
was found in the DB. That is expected if " +
"this component has never been finalized to a newer version.",
getApparentVersion());
- } else if (!isAllowed(apparentVersionInNewDB)) {
- // The DB is ahead of the VERSION file. This can happen if the leader
finalized and this follower received the
- // update through a snapshot. It can also happen if the component was
finalizing from a snapshot, failed
- // partway through, and restarted.
- if (apparentVersionInNewDB != getSoftwareVersion()) {
- LOG.warn("Received apparent version {} in DB which does not match
the current apparent version {} or " +
- "software version {}. User may have incorrectly run
finalization before all components were " +
- "upgraded. Finalizing this component to its software
version.",
- apparentVersionInNewDB, getApparentVersion(),
getSoftwareVersion());
- } else if (getApparentVersion().serialize() <
apparentVersionInNewDB.serialize()) {
- LOG.info("Version file has lower apparent version ({}) than DB
({}). Finalizing to the DB version.",
- getApparentVersion(), apparentVersionInNewDB);
+ } else if (!apparentVersionInDB.equals(getApparentVersion())) {
+ // We only need to take potential action if DB version != apparent
version.
+ if (isAllowed(apparentVersionInDB)) {
+ // DB version < apparent version.
+ LOG.debug("Received apparent version {} in DB which is less than
the current apparent version {}. " +
+ "Component likely previously crashed during finalization. Ratis
log replay should correct this issue " +
+ "after startup.",
+ apparentVersionInDB, getApparentVersion());
+ } else {
+ // DB version > apparent version
+ // We must finalize to this version.
+ if (apparentVersionInDB.isSupportedBy(getSoftwareVersion()) &&
+ !apparentVersionInDB.equals(getSoftwareVersion())) {
+ // DB version < software version.
+ // This should never happen.
+ // When restarting after failed finalization, the DB should not
have been updated until all versions up to
+ // the software version were finalized.
+ // On the snapshot installation path all components should be
running the same software version when
+ // finalization is run.
+ LOG.warn("Received apparent version {} in DB which is between the
current apparent version {} " +
+ "and the software version {}. User may have incorrectly run
finalization in a mixed version " +
+ "cluster.", apparentVersionInDB, getApparentVersion(),
getSoftwareVersion());
+ } else {
+ // DB version == software version
+ // This is expected when we receive finalization from a Ratis
snapshot.
+ // The previous attempt to finalize from a snapshot may have
failed, so we will resume it here.
+ LOG.info("Version file has lower apparent version ({}) than DB
({}). Finalizing to the DB version.",
+ getApparentVersion(), apparentVersionInDB);
+ }
+ finalizeUpgrade(apparentVersionInDB);
}
- finalizeUpgrade();
- } else if (!apparentVersionInNewDB.equals(getApparentVersion())) {
- // The DB is behind the VERSION file. This should not happen:
finalization writes the VERSION file inside
- // applyTransaction, so the Ratis log entry was committed and should
be replayed on restart, which would
- // update the DB. This is a defensive branch to catch any potential
bugs.
- LOG.warn("DB has lower apparent version ({}) than VERSION file ({}).
Updating DB to match VERSION file.",
- apparentVersionInNewDB, getApparentVersion());
- finalizationStore.put(APPARENT_VERSION_KEY,
String.valueOf(getApparentVersion().serialize()));
}
}
```
--
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]
