You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "nandakumar131 (via GitHub)" <gi...@apache.org> on 2025/06/16 17:49:16 UTC

[PR] HDDS-13281. Disable Ratis metadata write to Raft Log on OM & SCM. [ozone]

nandakumar131 opened a new pull request, #8637:
URL: https://github.com/apache/ozone/pull/8637

   ## What changes were proposed in this pull request?
   Disable Ratis metadata write to Raft Log on OM & SCM
   
   For OM & SCM we don't have to write the commit index to the Raft Log, this is meant for scenarios where we want to recover commit index on majority failure. This is not required for OM & SCM.
   
   Disabling commit index write to Raft Log will improver the performance of Ratis as we will make one less disk IO for each transaction.
   
   ## What is the link to the Apache JIRA
   HDDS-13281
   
   ## How was this patch tested?
   CI Run: https://github.com/nandakumar131/ozone/actions/runs/15688034541


-- 
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-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-13281. Disable Ratis metadata write to Raft Log on OM & SCM. [ozone]

Posted by "nandakumar131 (via GitHub)" <gi...@apache.org>.
nandakumar131 commented on code in PR #8637:
URL: https://github.com/apache/ozone/pull/8637#discussion_r2176572567


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -905,7 +905,37 @@ public void snapshotLimitCheck() throws IOException, OMException {
   }
 
   public void decrementInFlightSnapshotCount() {
-    inFlightSnapshotCount.decrementAndGet();
+    /*
+     * There is a race condition here because of which
+     * inFlightSnapshotCount could become negative sometimes.
+     *
+     * When we get a call back from Ratis on notifyLeaderReady,
+     * we assume that all the pending transactions (from RaftLog)
+     * are applied to the SateMachine and we go a head and reset the
+     * inFlightSnapshotCount to 0. The expectation here is that after
+     * the leader election we want to start inFlightSnapshotCount from 0.
+     *
+     * The applyTransaction in OzoneManagerStateMachine processes
+     * the calls in an async manner and returns a CompletableFuture,
+     * the transactions are not yet fully processed by the
+     * OzoneManagerStateMachine when Ratis notifies leader ready. (RATIS-2313)
+     *
+     * Because of the async processing in OzoneManagerStateMachine and
+     * Ratis not waiting for the CompletableFuture to complete, we mark the
+     * leader as ready (reset inFlightSnapshotCount to 0) even before we complete
+     * processing all the pending transactions from the old term. If there is a
+     * create snapshot transaction in the Raft Log from old term and it gets
+     * processed after we reset inFlightSnapshotCount, the count would become -1
+     * as we decrement the inFlightSnapshotCount when the OMSnapshotCreateRequest
+     * processing is completed.
+     *
+     * The workaround here is to make sure that we don't make inFlightSnapshotCount
+     * negative. We should be able to remove the workaround after RATIS-2313.
+     */
+    int result = inFlightSnapshotCount.decrementAndGet();
+    if (result < 0) {
+      resetInFlightSnapshotCount();
+    }

Review Comment:
   Created [HDDS-13357](https://issues.apache.org/jira/browse/HDDS-13357) to fix `resetInFlightSnapshotCount` logic. 



-- 
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-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-13281. Disable Ratis metadata write to Raft Log on OM & SCM. [ozone]

Posted by "nandakumar131 (via GitHub)" <gi...@apache.org>.
nandakumar131 commented on PR #8637:
URL: https://github.com/apache/ozone/pull/8637#issuecomment-3022009183

   Thanks @szetszwo for the review.
   Since all the required code changes are already done here, lets keep everything in this PR itself.
   If you have some more comments other than updating the `TODO comment` I will split the change into two PRs.


-- 
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-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-13281. Disable Ratis metadata write to Raft Log on OM & SCM. [ozone]

Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #8637:
URL: https://github.com/apache/ozone/pull/8637#discussion_r2155167076


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/upgrade/OMPrepareRequest.java:
##########
@@ -185,7 +185,6 @@ private static long waitForLogIndex(long minOMDBFlushIndex,
     // If we purge logs without waiting for this index, it may not make it to
     // the RocksDB snapshot, and then the log entry is lost on this OM.
     long minRatisStateMachineIndex = minOMDBFlushIndex + 1; // for the ratis-metadata transaction

Review Comment:
   @nandakumar131 , just found that We should also remove `minRatisStateMachineIndex` and just use `minOMDBFlushIndex`.



-- 
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-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-13281. Disable Ratis metadata write to Raft Log on OM & SCM. [ozone]

Posted by "nandakumar131 (via GitHub)" <gi...@apache.org>.
nandakumar131 commented on PR #8637:
URL: https://github.com/apache/ozone/pull/8637#issuecomment-2987202458

   Thanks for the ping @adoroszlai, I'm looking at the test timeouts.


-- 
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-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-13281. Disable Ratis metadata write to Raft Log on OM & SCM. [ozone]

Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #8637:
URL: https://github.com/apache/ozone/pull/8637#issuecomment-3005593476

   @nandakumar131 , This PR currently is in "draft" state.   Is it ready?  


-- 
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-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-13281. Disable Ratis metadata write to Raft Log on OM & SCM. [ozone]

Posted by "nandakumar131 (via GitHub)" <gi...@apache.org>.
nandakumar131 commented on PR #8637:
URL: https://github.com/apache/ozone/pull/8637#issuecomment-2988957359

   @szetszwo, the SCM State machine was not updating LastAppliedTermIndex due to which the tests were timing out.
   Fixed that in the latest commit.


-- 
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-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-13281. Disable Ratis metadata write to Raft Log on OM & SCM. [ozone]

Posted by "nandakumar131 (via GitHub)" <gi...@apache.org>.
nandakumar131 commented on code in PR #8637:
URL: https://github.com/apache/ozone/pull/8637#discussion_r2155176186


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/upgrade/OMPrepareRequest.java:
##########
@@ -202,11 +201,10 @@ private static long waitForLogIndex(long minOMDBFlushIndex,
           lastOMDBFlushIndex);
 
       // Check ratis state machine.
-      lastRatisCommitIndex = stateMachine.getLastNotifiedTermIndex().getIndex();
-      ratisStateMachineApplied = (lastRatisCommitIndex >=
-          minRatisStateMachineIndex);
+      final long lastRatisAppliedIndex = stateMachine.getLastAppliedTermIndex().getIndex();
+      ratisStateMachineApplied = lastRatisAppliedIndex >= minRatisStateMachineIndex;
       LOG.debug("{} Current Ratis state machine transaction index {}.",
-          om.getOMNodeId(), lastRatisCommitIndex);
+          om.getOMNodeId(), ratisStateMachineApplied);

Review Comment:
   Updated. Also the `lastRatisCommitIndex` was referred in the same method towards the end as well, replaced that with `lastRatisAppliedIndex`.



-- 
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-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-13281. Disable Ratis metadata write to Raft Log on OM & SCM. [ozone]

Posted by "nandakumar131 (via GitHub)" <gi...@apache.org>.
nandakumar131 commented on code in PR #8637:
URL: https://github.com/apache/ozone/pull/8637#discussion_r2155172746


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/upgrade/OMPrepareRequest.java:
##########
@@ -185,7 +185,6 @@ private static long waitForLogIndex(long minOMDBFlushIndex,
     // If we purge logs without waiting for this index, it may not make it to
     // the RocksDB snapshot, and then the log entry is lost on this OM.
     long minRatisStateMachineIndex = minOMDBFlushIndex + 1; // for the ratis-metadata transaction

Review Comment:
   Is it because we are not writing the commit index to Raft Log anymore, so there is no need to add `1` to `minOMDBFlushIndex`?



-- 
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-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-13281. Disable Ratis metadata write to Raft Log on OM & SCM. [ozone]

Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #8637:
URL: https://github.com/apache/ozone/pull/8637#discussion_r2155204542


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/upgrade/OMPrepareRequest.java:
##########
@@ -185,7 +185,6 @@ private static long waitForLogIndex(long minOMDBFlushIndex,
     // If we purge logs without waiting for this index, it may not make it to
     // the RocksDB snapshot, and then the log entry is lost on this OM.
     long minRatisStateMachineIndex = minOMDBFlushIndex + 1; // for the ratis-metadata transaction

Review Comment:
   > ... so there is no need to add 1 to minOMDBFlushIndex?
   
   Yes.
   
   > Can't we just rely on om.getRatisSnapshotIndex() and make sure that the given minOMDBFlushIndex is present in the snapshot?
   
   In `OMPrepareRequest.validateAndUpdateCache(..)`, it waits and then takes snapshot.  So it should wait for `lastRatisAppliedIndex`.
   



-- 
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-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-13281. Disable Ratis metadata write to Raft Log on OM & SCM. [ozone]

Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #8637:
URL: https://github.com/apache/ozone/pull/8637#issuecomment-3021067366

   @nandakumar131 , this change turns out to be not straightforward.  How about we separate the OM and the SCM changes into two JIRAs?  I am fine if we want to keep everything in one JIRA.


-- 
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-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-13281. Disable Ratis metadata write to Raft Log on OM & SCM. [ozone]

Posted by "nandakumar131 (via GitHub)" <gi...@apache.org>.
nandakumar131 commented on PR #8637:
URL: https://github.com/apache/ozone/pull/8637#issuecomment-2985067546

   Thanks @szetszwo for the suggestion, updated the PR accordingly.


-- 
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-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-13281. Disable Ratis metadata write to Raft Log on OM & SCM. [ozone]

Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #8637:
URL: https://github.com/apache/ozone/pull/8637#issuecomment-2986867509

   Some integration tests seem to be timing out:
   
   - TestBlockDeletion
   - TestSCMInstallSnapshotWithHA


-- 
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-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-13281. Disable Ratis metadata write to Raft Log on OM & SCM. [ozone]

Posted by "nandakumar131 (via GitHub)" <gi...@apache.org>.
nandakumar131 commented on PR #8637:
URL: https://github.com/apache/ozone/pull/8637#issuecomment-2979327561

   Thanks @szetszwo and @ivandika3 for the review.
   
   We actually make use of commit index in OzoneManager [here](https://github.com/apache/ozone/blob/48aefeaad09653e286ec4abbc6872e95d9fa1510/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/upgrade/OMPrepareRequest.java#L172). So, we cannot go ahead with this change for now.


-- 
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-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-13281. Disable Ratis metadata write to Raft Log on OM & SCM. [ozone]

Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #8637:
URL: https://github.com/apache/ozone/pull/8637#issuecomment-2981197136

   > We actually make use of commit index in OzoneManager [here](https://github.com/apache/ozone/blob/48aefeaad09653e286ec4abbc6872e95d9fa1510/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/upgrade/OMPrepareRequest.java#L172). ...
   
   @nandakumar131 , Good catch!  We should change it to use `getLastAppliedTermIndex()`
   ```java
   +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/upgrade/OMPrepareRequest.java
   @@ -185,7 +185,6 @@ private static long waitForLogIndex(long minOMDBFlushIndex,
        // If we purge logs without waiting for this index, it may not make it to
        // the RocksDB snapshot, and then the log entry is lost on this OM.
        long minRatisStateMachineIndex = minOMDBFlushIndex + 1; // for the ratis-metadata transaction
   -    long lastRatisCommitIndex = RaftLog.INVALID_LOG_INDEX;
    
        // Wait OM state machine to apply the given index.
        long lastOMDBFlushIndex = RaftLog.INVALID_LOG_INDEX;
   @@ -202,11 +201,10 @@ private static long waitForLogIndex(long minOMDBFlushIndex,
              lastOMDBFlushIndex);
    
          // Check ratis state machine.
   -      lastRatisCommitIndex = stateMachine.getLastNotifiedTermIndex().getIndex();
   -      ratisStateMachineApplied = (lastRatisCommitIndex >=
   -          minRatisStateMachineIndex);
   +      final long lastRatisAppliedIndex = stateMachine.getLastAppliedTermIndex().getIndex();
   +      ratisStateMachineApplied = lastRatisAppliedIndex >= minRatisStateMachineIndex;
          LOG.debug("{} Current Ratis state machine transaction index {}.",
   -          om.getOMNodeId(), lastRatisCommitIndex);
   +          om.getOMNodeId(), lastRatisAppliedIndex);
    
          if (!(omDBFlushed && ratisStateMachineApplied)) {
            Thread.sleep(flushCheckInterval.toMillis());
   ```


-- 
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-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-13281. Disable Ratis metadata write to Raft Log on OM & SCM. [ozone]

Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #8637:
URL: https://github.com/apache/ozone/pull/8637#discussion_r2155164041


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/upgrade/OMPrepareRequest.java:
##########
@@ -202,11 +201,10 @@ private static long waitForLogIndex(long minOMDBFlushIndex,
           lastOMDBFlushIndex);
 
       // Check ratis state machine.
-      lastRatisCommitIndex = stateMachine.getLastNotifiedTermIndex().getIndex();
-      ratisStateMachineApplied = (lastRatisCommitIndex >=
-          minRatisStateMachineIndex);
+      final long lastRatisAppliedIndex = stateMachine.getLastAppliedTermIndex().getIndex();
+      ratisStateMachineApplied = lastRatisAppliedIndex >= minRatisStateMachineIndex;
       LOG.debug("{} Current Ratis state machine transaction index {}.",
-          om.getOMNodeId(), lastRatisCommitIndex);
+          om.getOMNodeId(), ratisStateMachineApplied);

Review Comment:
   @nandakumar131 , It should print `lastRatisAppliedIndex`.



-- 
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-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-13281. Disable Ratis metadata write to Raft Log on OM & SCM. [ozone]

Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #8637:
URL: https://github.com/apache/ozone/pull/8637#discussion_r2176032836


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -905,7 +905,37 @@ public void snapshotLimitCheck() throws IOException, OMException {
   }
 
   public void decrementInFlightSnapshotCount() {
-    inFlightSnapshotCount.decrementAndGet();
+    /*
+     * There is a race condition here because of which
+     * inFlightSnapshotCount could become negative sometimes.
+     *
+     * When we get a call back from Ratis on notifyLeaderReady,
+     * we assume that all the pending transactions (from RaftLog)
+     * are applied to the SateMachine and we go a head and reset the

Review Comment:
   This assumption is incorrect. -- LeaderReady means that the leader is able to commit a new transaction.  Ratis should not care about how the StateMachine applies the transactions.



##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -905,7 +905,37 @@ public void snapshotLimitCheck() throws IOException, OMException {
   }
 
   public void decrementInFlightSnapshotCount() {
-    inFlightSnapshotCount.decrementAndGet();
+    /*
+     * There is a race condition here because of which
+     * inFlightSnapshotCount could become negative sometimes.
+     *
+     * When we get a call back from Ratis on notifyLeaderReady,
+     * we assume that all the pending transactions (from RaftLog)
+     * are applied to the SateMachine and we go a head and reset the
+     * inFlightSnapshotCount to 0. The expectation here is that after
+     * the leader election we want to start inFlightSnapshotCount from 0.
+     *
+     * The applyTransaction in OzoneManagerStateMachine processes
+     * the calls in an async manner and returns a CompletableFuture,
+     * the transactions are not yet fully processed by the
+     * OzoneManagerStateMachine when Ratis notifies leader ready. (RATIS-2313)
+     *
+     * Because of the async processing in OzoneManagerStateMachine and
+     * Ratis not waiting for the CompletableFuture to complete, we mark the
+     * leader as ready (reset inFlightSnapshotCount to 0) even before we complete
+     * processing all the pending transactions from the old term. If there is a
+     * create snapshot transaction in the Raft Log from old term and it gets
+     * processed after we reset inFlightSnapshotCount, the count would become -1
+     * as we decrement the inFlightSnapshotCount when the OMSnapshotCreateRequest
+     * processing is completed.
+     *
+     * The workaround here is to make sure that we don't make inFlightSnapshotCount
+     * negative. We should be able to remove the workaround after RATIS-2313.

Review Comment:
   As mentioned in [this comment](https://issues.apache.org/jira/browse/RATIS-2313?focusedCommentId=17987031&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17987031), we should not change Ratis for fixing the `inFlightSnapshotCount` bug.
   
   BTW, I can see that `inFlightSnapshotCount` may have other bugs such as the request could be disallowed by PrepareState:
   https://github.com/apache/ozone/blob/18845e0954e2b7c237d28bd65ed394f11f434fe2/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerRatisServer.java#L260
   It seems that the count won't  be decremented in that case.  The problem is the account logic of `inFlightSnapshotCount` has flaw.



##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -905,7 +905,37 @@ public void snapshotLimitCheck() throws IOException, OMException {
   }
 
   public void decrementInFlightSnapshotCount() {
-    inFlightSnapshotCount.decrementAndGet();
+    /*
+     * There is a race condition here because of which
+     * inFlightSnapshotCount could become negative sometimes.
+     *
+     * When we get a call back from Ratis on notifyLeaderReady,
+     * we assume that all the pending transactions (from RaftLog)
+     * are applied to the SateMachine and we go a head and reset the
+     * inFlightSnapshotCount to 0. The expectation here is that after

Review Comment:
   We should fix the accounting logic of `inFlightSnapshotCount`.
   - When a request is submitted, increment the count;
   - When the same request is completed/falied/cancelled, decrement the count.
   
   It should not rely on an external condition to reset the count.
   



##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -905,7 +905,37 @@ public void snapshotLimitCheck() throws IOException, OMException {
   }
 
   public void decrementInFlightSnapshotCount() {
-    inFlightSnapshotCount.decrementAndGet();
+    /*
+     * There is a race condition here because of which
+     * inFlightSnapshotCount could become negative sometimes.
+     *
+     * When we get a call back from Ratis on notifyLeaderReady,
+     * we assume that all the pending transactions (from RaftLog)
+     * are applied to the SateMachine and we go a head and reset the
+     * inFlightSnapshotCount to 0. The expectation here is that after
+     * the leader election we want to start inFlightSnapshotCount from 0.
+     *
+     * The applyTransaction in OzoneManagerStateMachine processes
+     * the calls in an async manner and returns a CompletableFuture,
+     * the transactions are not yet fully processed by the
+     * OzoneManagerStateMachine when Ratis notifies leader ready. (RATIS-2313)
+     *
+     * Because of the async processing in OzoneManagerStateMachine and
+     * Ratis not waiting for the CompletableFuture to complete, we mark the
+     * leader as ready (reset inFlightSnapshotCount to 0) even before we complete
+     * processing all the pending transactions from the old term. If there is a
+     * create snapshot transaction in the Raft Log from old term and it gets
+     * processed after we reset inFlightSnapshotCount, the count would become -1
+     * as we decrement the inFlightSnapshotCount when the OMSnapshotCreateRequest
+     * processing is completed.
+     *
+     * The workaround here is to make sure that we don't make inFlightSnapshotCount
+     * negative. We should be able to remove the workaround after RATIS-2313.
+     */
+    int result = inFlightSnapshotCount.decrementAndGet();
+    if (result < 0) {
+      resetInFlightSnapshotCount();
+    }

Review Comment:
   The workaround is fine.  How about updating the comment to below?
   ```java
   // TODO this is a work around for the accounting logic of `inFlightSnapshotCount`.
   //    - It incorrectly assumes that LeaderReady means that there are no inflight snapshot requests.
   //      We may consider fixing it by waiting all the pending requests in notifyLeaderReady().
   //    - Also, it seems to have another bug that the PrepareState could disallow snapshot requests.
   //      In such case, `inFlightSnapshotCount` won't be decremented. 
   ```
   
   We should file a separate Ozone JIRA for fixing 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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-13281. Disable Ratis metadata write to Raft Log on OM & SCM. [ozone]

Posted by "nandakumar131 (via GitHub)" <gi...@apache.org>.
nandakumar131 commented on PR #8637:
URL: https://github.com/apache/ozone/pull/8637#issuecomment-3027420774

   Thanks @szetszwo & @ivandika3 for the review!


-- 
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-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-13281. Disable Ratis metadata write to Raft Log on OM & SCM. [ozone]

Posted by "nandakumar131 (via GitHub)" <gi...@apache.org>.
nandakumar131 merged PR #8637:
URL: https://github.com/apache/ozone/pull/8637


-- 
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-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-13281. Disable Ratis metadata write to Raft Log on OM & SCM. [ozone]

Posted by "nandakumar131 (via GitHub)" <gi...@apache.org>.
nandakumar131 commented on code in PR #8637:
URL: https://github.com/apache/ozone/pull/8637#discussion_r2155230368


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/upgrade/OMPrepareRequest.java:
##########
@@ -185,7 +185,6 @@ private static long waitForLogIndex(long minOMDBFlushIndex,
     // If we purge logs without waiting for this index, it may not make it to
     // the RocksDB snapshot, and then the log entry is lost on this OM.
     long minRatisStateMachineIndex = minOMDBFlushIndex + 1; // for the ratis-metadata transaction

Review Comment:
   Thanks for the explanation. Updated the patch accordingly.



-- 
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-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-13281. Disable Ratis metadata write to Raft Log on OM & SCM. [ozone]

Posted by "nandakumar131 (via GitHub)" <gi...@apache.org>.
nandakumar131 commented on code in PR #8637:
URL: https://github.com/apache/ozone/pull/8637#discussion_r2155193872


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/upgrade/OMPrepareRequest.java:
##########
@@ -185,7 +185,6 @@ private static long waitForLogIndex(long minOMDBFlushIndex,
     // If we purge logs without waiting for this index, it may not make it to
     // the RocksDB snapshot, and then the log entry is lost on this OM.
     long minRatisStateMachineIndex = minOMDBFlushIndex + 1; // for the ratis-metadata transaction

Review Comment:
   Do we really need the `lastRatisAppliedIndex` check?
   Can't we just rely on `om.getRatisSnapshotIndex()` and make sure that the given `minOMDBFlushIndex` is present in the snapshot? If `minOMDBFlushIndex` is present in the snapshot, then it will definitely be applied in the Ratis' state machine.



-- 
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-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-13281. Disable Ratis metadata write to Raft Log on OM & SCM. [ozone]

Posted by "nandakumar131 (via GitHub)" <gi...@apache.org>.
nandakumar131 commented on PR #8637:
URL: https://github.com/apache/ozone/pull/8637#issuecomment-3006891492

   @szetszwo, there are test failures in the CI run. Trying to check if they are related to this change, will make the PR ready once the test failures are addressed.


-- 
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-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org