HDDS-11866. Remove code paths for non-Ratis OM #7778
Conversation
adoroszlai
added
the
code-cleanup
| @@ -260,9 +254,6 @@ public OMPrefixAclOpResult addAcl(OzoneObj ozoneObj, OzoneAcl ozoneAcl, | |||
| // update the in-memory prefix tree | |||
| prefixTree.insert(ozoneObj.getPath(), prefixInfo); | |||
|
|
|||
| if (!isRatisEnabled) { | |||
There was a problem hiding this comment.
I'm not sure about this one. This code path (isRatisEnabled=false) is used by OmSnapshotManager. Are we sure it doesn't break the Snapshot feature?
There was a problem hiding this comment.
@hemantk-12 or @smengcl do you know why Snapshot creates PrefixManagerImpl assuming ratis not enabled?
| @@ -4289,8 +4249,9 @@ public void checkLeaderStatus() throws OMNotLeaderException, | |||
| /** | |||
| * Return if Ratis is enabled or not. | |||
| */ | |||
| // FIXME remove | |||
There was a problem hiding this comment.
This is for HDDS-12161, added issue number in the comment.
| @@ -384,8 +384,7 @@ public OmSnapshot load(@Nonnull UUID snapshotId) throws IOException { | |||
| try { | |||
| // create the other manager instances based on snapshot | |||
| // metadataManager | |||
| PrefixManagerImpl pm = new PrefixManagerImpl(ozoneManager, snapshotMetadataManager, | |||
| false); | |||
There was a problem hiding this comment.
Thanks, restored.
|
Would an upgrade from non-HA OM to HA OM work properly? Not sure if the upgrade path is tested properly |
|
Thanks @jojochuang for the review.
I thought the idea is to only support upgrade with OM Ratis already enabled (i.e. have to enabled it before upgrade). |
| * Only used to handle write requests when ratis is disabled. | ||
| * When ratis is enabled, write requests are handled by the state machine. | ||
| */ |
There was a problem hiding this comment.
Remove this comment
| * Only used to handle write requests when ratis is disabled. | |
| * When ratis is enabled, write requests are handled by the state machine. | |
| */ |
There was a problem hiding this comment.
Thanks for spotting it, the variable is also leftover.
| public PrefixManagerImpl(OzoneManager ozoneManager, OMMetadataManager metadataManager, | ||
| boolean isRatisEnabled) { | ||
| this.isRatisEnabled = isRatisEnabled; | ||
| public PrefixManagerImpl(OzoneManager ozoneManager, OMMetadataManager metadataManager) { |
There was a problem hiding this comment.
OzoneManager instantiates a PrefixManagerImpl with isRatisEnabled = true and
OmSnapshotManager instantiates a PrefixManagerImpl with isRatisEnabled = false.
I would suggest to restore PrefixManagerImpl, and rename isRatisEnabled to persistentToRocksDB, because it looks like OmSnapshotManager repurposed it and which is unrelated to Ratis.
There was a problem hiding this comment.
Restored, now the tests are unchanged compared to master. However, I kept the name isRatisEnabled, someone more familiar with snapshots can rename it separately.
|
Merged. Thanks @adoroszlai |
|
Thanks @jojochuang for reviewing and merging this. |
What changes were proposed in this pull request?
Remove production code for non-Ratis case in Ozone Manager, except request/response classes (to be done in HDDS-12161 to keep patch size smaller).
Real change is easier to see with
--ignore-space-change:https://issues.apache.org/jira/browse/HDDS-11866
How was this patch tested?
CI:
https://github.com/adoroszlai/ozone/actions/runs/13038347046