HDDS-8508. SCMHATransactionBuffer flush based on time #4683

sumitagrawl · 2023-05-09T07:20:12Z

What changes were proposed in this pull request?

Timer based flush is added using ratis SnapshotManagement

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-8508

How was this patch tested?

UT cases and integration test case.

Xushaohong · 2023-05-09T09:37:18Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHAManagerImpl.java

+            .setPeriodicalTask(() -> {
+              monitorTask.run();
+            }).build();


Suggested change

.setPeriodicalTask(() -> {

monitorTask.run();

}).build();

.setPeriodicalTask(monitorTask)

.build();

Could be simpler

Xushaohong · 2023-05-09T09:45:04Z

...erver-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHATransactionBufferMonitorTask.java

+      LOG.debug("Running TransactionFlushTask");
+      // set latest snapshot to null for force snapshot
+      // the value will be reset again when snapshot is taken
+      transactionBuffer.setLatestSnapshot(null);


Is this necessary? What will happen if we do not set null

@Xushaohong Thanks for review, above is done for full snapshot as,

Ratis will try to get diff of current applied Index and latest snapshot Index, with minGapValue (default value is 1024), if its less than do not take snapshot

But since this is force snapshot, and setting LatestSnapshot to null, its value is treated as "0", so diff after sometime crosses 1024 value and snapshot is taken.

Further reset is not required with old value as when taking snapshot, it will set with latest applied index. (I will add if value is not set during snapshot due to any failure for this case).

Note: At least 1024 transaction is required in this case also, as this limitation should be ok after discussion with Nicholas,

@szetszwo Please share if this logic is fine as discussed.

I see. In Ratis, the gap needs this to bypass the limitation. Thx for the explanation.

Xushaohong

Thx @sumitagrawl for the work. The idea of adding a timer to timely trigger the Ratis snapshot mostly looks fine to me. Left a few comments.

Besides, we need to reset txFlushPending in SCMHADBTransactionBufferImpl#init. When a snapshot is loaded and reinited, the txFlushPending should be reset also.

sumitagrawl · 2023-05-10T14:27:21Z

Thx @sumitagrawl for the work. The idea of adding a timer to timely trigger the Ratis snapshot mostly looks fine to me. Left a few comments.

Besides, we need to reset txFlushPending in SCMHADBTransactionBufferImpl#init. When a snapshot is loaded and reinited, the txFlushPending should be reset also.

@Xushaohong Its updated

Xushaohong

Sorry for the late reply.
The change looks good to me! Thx @sumitagrawl.
See the latest suggestion, could be fine without that.

Hi @szetszwo, could you help take a look at this PR?

Xushaohong · 2023-05-16T13:24:32Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/ScmConfigKeys.java


  public static final String OZONE_SCM_HA_RATIS_SNAPSHOT_GAP
-      = "raft.server.snapshot.creation.gap";
+      = "ozone.scm.ha.ratis.server.snapshot.creation.gap";


Could you refine the name and add some comments to help with the comprehension for the other reader? It might be confusing with the AutoTriggerThreshold as it is only used when RATIS takeSnapshotAsync by manual.
How about ozone.scm.ha.ratis.snapshot.creation.auto.threshold and ozone.scm.ha.ratis.snapshot.creation.manual.gap respectively?

@Xushaohong Thanks for review,
"raft.server.snapshot.creation.gap" -- This is configuration provided by ratis to trigger / take snapshot.
"ozone.scm.ha.ratis.server.snapshot.creation.gap" -- This configuration is provided at ozone mapping to ratis, which can be updated. And is applicable for both manual snapshot and raits automatic snapshot.

So adding manual is not required.

Hi @sumitagrawl
Actually, I mean these two: ScmConfigKeys.OZONE_SCM_HA_RATIS_SNAPSHOT_THRESHOLD and OZONE_SCM_HA_RATIS_SNAPSHOT_GAP.
They are both SCM side conf, but their similar names could be confusing for those not too familiar.
It might be better to add some comments to tell the difference or rename them.

@Xushaohong Updated with comment

szetszwo

@sumitagrawl , thanks a lot for working on this! Please see the comments inlined.

szetszwo · 2023-05-23T16:18:14Z

...dds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHADBTransactionBufferImpl.java

      Table<KEY, VALUE> table, KEY key, VALUE value) throws IOException {
    rwLock.readLock().lock();
    try {
+      txFlushPending++;


It need AtomicLong or writeLock.

szetszwo · 2023-05-23T16:25:57Z

...erver-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHATransactionBufferMonitorTask.java

+        if (null == transactionBuffer.getLatestSnapshot()) {
+          transactionBuffer.setLatestSnapshot(lastSnapshot);
+        }


getLatestSnapshot and setLatestSnapshot are not thread safe.

Updated ... added lock while update or get snapshot

It is still not working get and set are two different calls. We need to use AtomicReference.compareAndSet.

szetszwo · 2023-05-23T16:29:47Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMRatisServerImpl.java

+    final long requestTimeout = ozoneConf.getTimeDuration(
+        ScmConfigKeys.OZONE_SCM_HA_RATIS_REQUEST_TIMEOUT,
+        ScmConfigKeys.OZONE_SCM_HA_RATIS_REQUEST_TIMEOUT_DEFAULT,
+        TimeUnit.MILLISECONDS);
+    Preconditions.checkArgument(requestTimeout > 1000L,
+        "Ratis request timeout cannot be less than 1000ms.");


Declare requestTimeout as a field and move the check to the constructor.

szetszwo · 2023-05-23T16:35:05Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMRatisServerImpl.java

+        nextCallId(), requestTimeout);
+    final RaftClientReply raftClientReply = server.snapshotManagement(req);
+    if (!raftClientReply.isSuccess()) {
+      LOG.info("Snapshot request failed", raftClientReply.getException());


Use LOG.warn.

szetszwo · 2023-05-23T16:36:24Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMRatisServerImpl.java

  }

+  @Override
+  public boolean doSnapshotRequest() throws IOException {


Let's call it triggerSnapshot.

szetszwo · 2023-05-23T16:37:05Z

...erver-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHATransactionBufferMonitorTask.java

@@ -0,0 +1,69 @@
+/**


Please don't use javadoc for license.

szetszwo · 2023-05-23T16:37:50Z

...erver-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHATransactionBufferMonitorTask.java

+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with this
+ * work for additional information regarding copyright ownership.  The ASF
+ * licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.


Please copy a well formatted license header; see https://www.apache.org/legal/src-headers.html

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHAManagerImpl.java

szetszwo · 2023-05-23T16:40:51Z

...erver-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHATransactionBufferMonitorTask.java

+  private SCMRatisServer server;
+  private SCMHADBTransactionBuffer transactionBuffer;
+  private long flushInterval = 0;


Add final.

szetszwo · 2023-05-23T16:45:18Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHAManagerImpl.java

+        TimeUnit.MILLISECONDS);
+    SCMHATransactionBufferMonitorTask monitorTask
+        = new SCMHATransactionBufferMonitorTask(
+        (SCMHADBTransactionBuffer) transactionBuffer, ratisServer, interval);


When HA is disabled, the cast will fail.

There is a check before calling this method in start(), if ratisServer is null, return. So this flow will not hit when HA is disabled.

I see. Could you move the method to right below the start() method?

szetszwo

@sumitagrawl , thanks for the update!

We need to declare latestSnapshot as AtomicReference<SnapshotInfo>; see https://issues.apache.org/jira/secure/attachment/13058517/4683_review.patch

szetszwo · 2023-05-25T05:22:37Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/ScmConfigKeys.java

+  // the config will transfer value to ratis config
+  // raft.server.snapshot.auto.trigger.threshold


Use javadoc comments.

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHAManagerImpl.java

szetszwo · 2023-05-25T05:38:34Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHAManagerImpl.java

+        TimeUnit.MILLISECONDS);
+    SCMHATransactionBufferMonitorTask monitorTask
+        = new SCMHATransactionBufferMonitorTask(
+        (SCMHADBTransactionBuffer) transactionBuffer, ratisServer, interval);


I see. Could you move the method to right below the start() method?

szetszwo · 2023-05-25T05:44:22Z

...dds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHADBTransactionBufferImpl.java

 package org.apache.hadoop.hdds.scm.ha;

 import com.google.common.base.Preconditions;
+import java.util.concurrent.atomic.AtomicLong;


Could you move it right above to import java.util.concurrent.locks.ReentrantReadWriteLock;?

szetszwo · 2023-05-25T05:44:44Z

...dds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHADBTransactionBufferImpl.java

  private BatchOperation currentBatchOperation;
  private TransactionInfo latestTrxInfo;
  private SnapshotInfo latestSnapshot;
+  private AtomicLong txFlushPending = new AtomicLong(0);


Add final.

szetszwo · 2023-05-25T05:49:48Z

...erver-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHATransactionBufferMonitorTask.java

+   * SCMService related variables.
+   */
+
+  @SuppressWarnings("parameternumber")


Remove @SuppressWarnings

szetszwo · 2023-05-25T06:07:33Z

...erver-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHATransactionBufferMonitorTask.java

+        if (null == transactionBuffer.getLatestSnapshot()) {
+          transactionBuffer.setLatestSnapshot(lastSnapshot);
+        }


It is still not working get and set are two different calls. We need to use AtomicReference.compareAndSet.

szetszwo · 2023-05-25T06:08:21Z

...erver-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHATransactionBufferMonitorTask.java

+      LOG.debug("Running TransactionFlushTask");
+      // set latest snapshot to null for force snapshot
+      // the value will be reset again when snapshot is taken
+      SnapshotInfo lastSnapshot = transactionBuffer.getLatestSnapshot();


We should use AtomicReference.getAndSet(null).

szetszwo

@sumitagrawl , thanks for the update!

We need to declare latestSnapshot as AtomicReference<SnapshotInfo>; see https://issues.apache.org/jira/secure/attachment/13058517/4683_review.patch

szetszwo

@sumitagrawl , thanks for the update!

We need to declare latestSnapshot as AtomicReference<SnapshotInfo>; see https://issues.apache.org/jira/secure/attachment/13058517/4683_review.patch

sumitagrawl · 2023-05-25T12:13:21Z

@szetszwo Thanks for suggestion for fixing latestSnapshot, I have fixed all comments. Plz review.

szetszwo

+1 the change looks good.

sumitagrawl added 5 commits May 5, 2023 05:46

HDDS-8508. SCMHATransactionBuffer flush based on time

5da8899

HDDS-8508. SCMHATransactionBuffer flush based on time

2d9d720

Merge branch 'apache:master' into HDDS-8508

66c6303

HDDS-8508. SCMHATransactionBuffer flush based on time

16e3fa8

HDDS-8508. SCMHATransactionBuffer flush based on time

6fc55c2

Xushaohong reviewed May 9, 2023

View reviewed changes

sumitagrawl added 2 commits May 9, 2023 19:54

HDDS-8508. SCMHATransactionBuffer flush based on time

247087e

HDDS-8508. SCMHATransactionBuffer flush based on time

9216ed7

sumitagrawl requested a review from Xushaohong May 10, 2023 14:26

sumitagrawl added 3 commits May 11, 2023 07:51

Merge branch 'apache:master' into HDDS-8508

f77d5b6

HDDS-8508. SCMHATransactionBuffer flush based on time

6927410

Merge branch 'HDDS-8508' of github.com:sumitagrawl/ozone into HDDS-8508

03bbe7d

Xushaohong approved these changes May 16, 2023

View reviewed changes

Xushaohong reviewed May 16, 2023

View reviewed changes

szetszwo reviewed May 23, 2023

View reviewed changes

sumitagrawl added 2 commits May 24, 2023 20:27

HDDS-8508. SCMHATransactionBuffer flush based on time

638e663

HDDS-8508. SCMHATransactionBuffer flush based on time

5a01e57

sumitagrawl requested a review from szetszwo May 24, 2023 15:19

szetszwo mentioned this pull request May 25, 2023

HDDS-7621. Update SCM term in datanode from heartbeat without any commands #4101

Merged

szetszwo reviewed May 25, 2023

View reviewed changes

HDDS-8508. SCMHATransactionBuffer flush based on time

115b109

sumitagrawl requested a review from szetszwo May 25, 2023 12:12

szetszwo approved these changes May 25, 2023

View reviewed changes

szetszwo merged commit 72bce3d into apache:master May 25, 2023

		// the config will transfer value to ratis config
		// raft.server.snapshot.auto.trigger.threshold

HDDS-8508. SCMHATransactionBuffer flush based on time #4683

HDDS-8508. SCMHATransactionBuffer flush based on time #4683

Uh oh!

Conversation

sumitagrawl commented May 9, 2023

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Xushaohong May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Xushaohong left a comment

Choose a reason for hiding this comment

Uh oh!

sumitagrawl commented May 10, 2023

Uh oh!

Xushaohong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Xushaohong May 16, 2023 •

edited

Loading