Re: [PR] HDDS-11866. Remove code paths for non-Ratis OM [ozone]
adoroszlai commented on PR #7778: URL: https://github.com/apache/ozone/pull/7778#issuecomment-2642096133 Thanks @jojochuang for reviewing and merging this. -- 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: issues-unsubscr...@ozone.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@ozone.apache.org For additional commands, e-mail: issues-h...@ozone.apache.org
Re: [PR] HDDS-11866. Remove code paths for non-Ratis OM [ozone]
jojochuang commented on PR #7778: URL: https://github.com/apache/ozone/pull/7778#issuecomment-2641002182 Merged. Thanks @adoroszlai -- 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: issues-unsubscr...@ozone.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@ozone.apache.org For additional commands, e-mail: issues-h...@ozone.apache.org
Re: [PR] HDDS-11866. Remove code paths for non-Ratis OM [ozone]
jojochuang merged PR #7778: URL: https://github.com/apache/ozone/pull/7778 -- 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: issues-unsubscr...@ozone.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@ozone.apache.org For additional commands, e-mail: issues-h...@ozone.apache.org
Re: [PR] HDDS-11866. Remove code paths for non-Ratis OM [ozone]
adoroszlai commented on code in PR #7778: URL: https://github.com/apache/ozone/pull/7778#discussion_r1943583949 ## hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OzoneManagerProtocolServerSideTranslatorPB.java: ## @@ -79,7 +71,6 @@ public class OzoneManagerProtocolServerSideTranslatorPB implements OzoneManagerP * Only used to handle write requests when ratis is disabled. * When ratis is enabled, write requests are handled by the state machine. */ Review Comment: Thanks for spotting it, the variable is also leftover. -- 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: issues-unsubscr...@ozone.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@ozone.apache.org For additional commands, e-mail: issues-h...@ozone.apache.org
Re: [PR] HDDS-11866. Remove code paths for non-Ratis OM [ozone]
adoroszlai commented on code in PR #7778:
URL: https://github.com/apache/ozone/pull/7778#discussion_r1943586069
##
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/PrefixManagerImpl.java:
##
@@ -64,13 +64,7 @@ public class PrefixManagerImpl implements PrefixManager {
// In-memory prefix tree to optimize ACL evaluation
private RadixTree prefixTree;
- // TODO: This isRatisEnabled check will be removed as part of HDDS-1909,
- // where we integrate both HA and Non-HA code.
- private boolean isRatisEnabled;
-
- public PrefixManagerImpl(OzoneManager ozoneManager, OMMetadataManager
metadataManager,
- boolean isRatisEnabled) {
-this.isRatisEnabled = isRatisEnabled;
+ public PrefixManagerImpl(OzoneManager ozoneManager, OMMetadataManager
metadataManager) {
Review 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.
--
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: issues-unsubscr...@ozone.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@ozone.apache.org
For additional commands, e-mail: issues-h...@ozone.apache.org
Re: [PR] HDDS-11866. Remove code paths for non-Ratis OM [ozone]
jojochuang commented on code in PR #7778:
URL: https://github.com/apache/ozone/pull/7778#discussion_r1943486067
##
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OzoneManagerProtocolServerSideTranslatorPB.java:
##
@@ -79,7 +71,6 @@ public class OzoneManagerProtocolServerSideTranslatorPB
implements OzoneManagerP
* Only used to handle write requests when ratis is disabled.
* When ratis is enabled, write requests are handled by the state machine.
*/
Review Comment:
Remove this comment
```suggestion
```
##
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/PrefixManagerImpl.java:
##
@@ -64,13 +64,7 @@ public class PrefixManagerImpl implements PrefixManager {
// In-memory prefix tree to optimize ACL evaluation
private RadixTree prefixTree;
- // TODO: This isRatisEnabled check will be removed as part of HDDS-1909,
- // where we integrate both HA and Non-HA code.
- private boolean isRatisEnabled;
-
- public PrefixManagerImpl(OzoneManager ozoneManager, OMMetadataManager
metadataManager,
- boolean isRatisEnabled) {
-this.isRatisEnabled = isRatisEnabled;
+ public PrefixManagerImpl(OzoneManager ozoneManager, OMMetadataManager
metadataManager) {
Review 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.
--
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: issues-unsubscr...@ozone.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@ozone.apache.org
For additional commands, e-mail: issues-h...@ozone.apache.org
Re: [PR] HDDS-11866. Remove code paths for non-Ratis OM [ozone]
jojochuang commented on code in PR #7778:
URL: https://github.com/apache/ozone/pull/7778#discussion_r1943347548
##
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/PrefixManagerImpl.java:
##
@@ -260,9 +254,6 @@ public OMPrefixAclOpResult addAcl(OzoneObj ozoneObj,
OzoneAcl ozoneAcl,
// update the in-memory prefix tree
prefixTree.insert(ozoneObj.getPath(), prefixInfo);
-if (!isRatisEnabled) {
Review Comment:
@hemantk-12 or @smengcl do you know why Snapshot creates PrefixManagerImpl
assuming ratis not enabled?
--
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: issues-unsubscr...@ozone.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@ozone.apache.org
For additional commands, e-mail: issues-h...@ozone.apache.org
Re: [PR] HDDS-11866. Remove code paths for non-Ratis OM [ozone]
adoroszlai commented on code in PR #7778:
URL: https://github.com/apache/ozone/pull/7778#discussion_r1942694547
##
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##
@@ -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);
Review Comment:
Thanks, restored.
--
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: issues-unsubscr...@ozone.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@ozone.apache.org
For additional commands, e-mail: issues-h...@ozone.apache.org
Re: [PR] HDDS-11866. Remove code paths for non-Ratis OM [ozone]
adoroszlai commented on code in PR #7778: URL: https://github.com/apache/ozone/pull/7778#discussion_r1942694199 ## hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java: ## @@ -4289,8 +4249,9 @@ public void checkLeaderStatus() throws OMNotLeaderException, /** * Return if Ratis is enabled or not. */ + // FIXME remove Review Comment: This is for HDDS-12161, added issue number in the comment. -- 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: issues-unsubscr...@ozone.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@ozone.apache.org For additional commands, e-mail: issues-h...@ozone.apache.org
Re: [PR] HDDS-11866. Remove code paths for non-Ratis OM [ozone]
adoroszlai commented on code in PR #7778:
URL: https://github.com/apache/ozone/pull/7778#discussion_r1942693802
##
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/PrefixManagerImpl.java:
##
@@ -260,9 +254,6 @@ public OMPrefixAclOpResult addAcl(OzoneObj ozoneObj,
OzoneAcl ozoneAcl,
// update the in-memory prefix tree
prefixTree.insert(ozoneObj.getPath(), prefixInfo);
-if (!isRatisEnabled) {
Review Comment:
Restored.
--
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: issues-unsubscr...@ozone.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@ozone.apache.org
For additional commands, e-mail: issues-h...@ozone.apache.org
Re: [PR] HDDS-11866. Remove code paths for non-Ratis OM [ozone]
adoroszlai commented on PR #7778: URL: https://github.com/apache/ozone/pull/7778#issuecomment-2636458498 Thanks @jojochuang for the review. > Would an upgrade from non-HA OM to HA OM work properly? Not sure if the upgrade path is tested properly I thought the idea is to only support upgrade with OM Ratis already enabled (i.e. have to enabled it before upgrade). -- 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: issues-unsubscr...@ozone.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@ozone.apache.org For additional commands, e-mail: issues-h...@ozone.apache.org
Re: [PR] HDDS-11866. Remove code paths for non-Ratis OM [ozone]
jojochuang commented on PR #7778: URL: https://github.com/apache/ozone/pull/7778#issuecomment-2635349114 Would an upgrade from non-HA OM to HA OM? Not sure if the upgrade path is tested properly -- 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: issues-unsubscr...@ozone.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@ozone.apache.org For additional commands, e-mail: issues-h...@ozone.apache.org
Re: [PR] HDDS-11866. Remove code paths for non-Ratis OM [ozone]
jojochuang commented on code in PR #7778:
URL: https://github.com/apache/ozone/pull/7778#discussion_r1942052873
##
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##
@@ -4289,8 +4249,9 @@ public void checkLeaderStatus() throws
OMNotLeaderException,
/**
* Return if Ratis is enabled or not.
*/
+ // FIXME remove
Review Comment:
TODO
##
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/PrefixManagerImpl.java:
##
@@ -260,9 +254,6 @@ public OMPrefixAclOpResult addAcl(OzoneObj ozoneObj,
OzoneAcl ozoneAcl,
// update the in-memory prefix tree
prefixTree.insert(ozoneObj.getPath(), prefixInfo);
-if (!isRatisEnabled) {
Review 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?
##
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##
@@ -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);
Review Comment:
isRatisEnabled = false
--
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: issues-unsubscr...@ozone.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@ozone.apache.org
For additional commands, e-mail: issues-h...@ozone.apache.org
