Re: [PR] HDDS-10026. Remove applyTransactionMap and ratisTransactionMap from OzoneManagerStateMachine. [ozone]

["sumitagrawl (via GitHub)" <gi...@apache.org>]

sumitagrawl commented on code in PR #5891:
URL: https://github.com/apache/ozone/pull/5891#discussion_r1440136117


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerStateMachine.java:
##########
@@ -493,15 +489,28 @@ public OzoneManagerDoubleBuffer buildDoubleBufferForRatis() {
    */
   @Override
   public long takeSnapshot() throws IOException {
-    LOG.info("Current Snapshot Index {}", getLastAppliedTermIndex());
-    TermIndex lastTermIndex = getLastAppliedTermIndex();
-    final TransactionInfo build = TransactionInfo.valueOf(lastTermIndex);
-    ozoneManager.setTransactionInfo(build);
-    Table<String, TransactionInfo> txnInfoTable =
-        ozoneManager.getMetadataManager().getTransactionInfoTable();
-    txnInfoTable.put(TRANSACTION_INFO_KEY, build);
+    try {
+      ozoneManagerDoubleBuffer.awaitFlush();
+    } catch (InterruptedException e) {
+      throw IOUtils.toInterruptedIOException("Interrupted ozoneManagerDoubleBuffer.awaitFlush", e);
+    }
+
+    return takeSnapshotImpl();
+  }
+
+  private synchronized long takeSnapshotImpl() throws IOException {
+    final TermIndex applied = getLastAppliedTermIndex();
+    final TermIndex notified = getLastNotifiedTermIndex();
+    final TermIndex snapshot = applied.compareTo(notified) > 0 ? applied : notified;

Review Comment:
   updating DB with notified TermIndex will be incorrect, as notified will be greater than applied, as there can be many transaction pending in executers and may take time. So this update can give wrong snapshot info.
   
   Further if there is a restart, then raft log not yet executed will never be executed as transaction info in DB is updated to the latest.



##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerStateMachine.java:
##########
@@ -186,19 +177,26 @@ public void notifyLeaderChanged(RaftGroupMemberId groupMemberId,
    * @param index index which is being updated
    */
   @Override
-  public void notifyTermIndexUpdated(long currentTerm, long index) {
-    // SnapshotInfo should be updated when the term changes.
-    // The index here refers to the log entry index and the index in
-    // SnapshotInfo represents the snapshotIndex i.e. the index of the last
-    // transaction included in the snapshot. Hence, snaphsotInfo#index is not
-    // updated here.
+  public synchronized void notifyTermIndexUpdated(long currentTerm, long index) {
+    final TermIndex newTermIndex = TermIndex.valueOf(currentTerm, index);
+    lastNotifiedTermIndex = assertUpdateIncreasingly("lastNotified", lastNotifiedTermIndex, newTermIndex);

Review Comment:
   I have seen issues while handling with earlier PR, that, leader election depends on  notifiedTermIndex (as updated to lastAppliedTermIndex), but here we are not updating to lastAppliedTermIndex.
   
   Check if this can have impact to ratis, as this information is not known to ratis internally till snapshot is taken.



-- 
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