errose28 commented on code in PR #4138:
URL: https://github.com/apache/ozone/pull/4138#discussion_r1066307476
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -459,6 +459,7 @@ private OzoneManager(OzoneConfiguration conf, StartupOption
startupOption)
this.omNodeDetails = omhaNodeDetails.getLocalNodeDetails();
omStorage = new OMStorage(conf);
+ omStorage.validateOrPersistOmNodeId(omNodeDetails.getNodeId());
Review Comment:
I can see 3 possible options:
1. Write the value in the constructor if it is not present, or validate the
value if it is already there.
- Simple approach
- All installs get the value without a re-init.
- May lead to code fragmentation between omInit and the constructor for
all new fields added to the version file.
- omInit must write all the values, but the constructor must also
write any new values. Previously the constructor did not write to the version
file.
2. Add a layout feature and upgrade action to write the new value to the
version file. omInit writes the value to the version file, and OM constructor
only validates the value.
- More involved approach
- All installs get the value without a re-init.
- omInit and the constructor maintain their original jobs with respect
to the version file, but some upgrade pieces must be added.
3. Write the value in omInit only. Existing clusters that do not re-init
will not have the value persisted.
- Simple approach
- Only new installs or re-inited existing installs get it.
- omInit and constructor maintain their original jobs with respect to
the version file.
2 seems excessive IMO since there are no downgrade implications to the
change. 3 may be annoying to debug in the future. Someone may change the node
ID and this change will not catch it even if it is present. So after writing
out the options like this, I think 1 is the best choice, which is what the PR
is already doing.
After another look the javadoc on the method should be sufficient. If anyone
is wondering why the constructor may need to modify the version file on startup
they would probably go from the constructor to the method declaration and see
the doc.
--
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]
