You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "sumitagrawl (via GitHub)" <gi...@apache.org> on 2023/05/09 07:20:14 UTC
[GitHub] [ozone] sumitagrawl opened a new pull request, #4683: HDDS-8508. SCMHATransactionBuffer flush based on time
sumitagrawl opened a new pull request, #4683:
URL: https://github.com/apache/ozone/pull/4683
## 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.
--
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
[GitHub] [ozone] sumitagrawl commented on a diff in pull request #4683: HDDS-8508. SCMHATransactionBuffer flush based on time
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #4683:
URL: https://github.com/apache/ozone/pull/4683#discussion_r1204368263
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/ScmConfigKeys.java:
##########
@@ -562,7 +562,7 @@ public final class ScmConfigKeys {
1000L;
public static final String OZONE_SCM_HA_RATIS_SNAPSHOT_GAP
- = "raft.server.snapshot.creation.gap";
+ = "ozone.scm.ha.ratis.server.snapshot.creation.gap";
Review Comment:
@Xushaohong Updated with 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-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
[GitHub] [ozone] szetszwo commented on a diff in pull request #4683: HDDS-8508. SCMHATransactionBuffer flush based on time
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #4683:
URL: https://github.com/apache/ozone/pull/4683#discussion_r1205020883
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHADBTransactionBufferImpl.java:
##########
@@ -17,6 +17,7 @@
package org.apache.hadoop.hdds.scm.ha;
import com.google.common.base.Preconditions;
+import java.util.concurrent.atomic.AtomicLong;
Review Comment:
Could you move it right above to `import java.util.concurrent.locks.ReentrantReadWriteLock;`?
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/ScmConfigKeys.java:
##########
@@ -556,11 +556,20 @@ public final class ScmConfigKeys {
"ozone.scm.ha.ratis.log.purge.gap";
public static final int OZONE_SCM_HA_RAFT_LOG_PURGE_GAP_DEFAULT = 1000000;
+ // the config will transfer value to ratis config
+ // raft.server.snapshot.auto.trigger.threshold
Review Comment:
Use javadoc comments.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHATransactionBufferMonitorTask.java:
##########
@@ -0,0 +1,71 @@
+/*
+ * 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.
+ */
+package org.apache.hadoop.hdds.scm.ha;
+
+import java.io.IOException;
+import org.apache.ratis.statemachine.SnapshotInfo;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A background service running in SCM to check and flush the HA Transaction
+ * buffer.
+ */
+public class SCMHATransactionBufferMonitorTask implements Runnable {
+ public static final Logger LOG =
+ LoggerFactory.getLogger(SCMHATransactionBufferMonitorTask.class);
+ private final SCMRatisServer server;
+ private final SCMHADBTransactionBuffer transactionBuffer;
+ private final long flushInterval;
+
+ /**
+ * SCMService related variables.
+ */
+
+ @SuppressWarnings("parameternumber")
Review Comment:
Remove `@SuppressWarnings`
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHAManagerImpl.java:
##########
@@ -67,6 +71,7 @@ public class SCMHAManagerImpl implements SCMHAManager {
// this should ideally be started only in a ratis leader
private final InterSCMGrpcProtocolService grpcServer;
+ private BackgroundSCMService trxBufferMonitorService = null;
Review Comment:
I see. We need to have some code refactoring. Let's do it later.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHATransactionBufferMonitorTask.java:
##########
@@ -0,0 +1,69 @@
+/**
+ * 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.
+ */
+package org.apache.hadoop.hdds.scm.ha;
+
+import java.io.IOException;
+import org.apache.ratis.statemachine.SnapshotInfo;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A background service running in SCM to check and flush the HA Transaction
+ * buffer.
+ */
+public class SCMHATransactionBufferMonitorTask implements Runnable {
+ public static final Logger LOG =
+ LoggerFactory.getLogger(SCMHATransactionBufferMonitorTask.class);
+ private SCMRatisServer server;
+ private SCMHADBTransactionBuffer transactionBuffer;
+ private long flushInterval = 0;
+
+ /**
+ * SCMService related variables.
+ */
+
+ @SuppressWarnings("parameternumber")
+ public SCMHATransactionBufferMonitorTask(
+ SCMHADBTransactionBuffer transactionBuffer,
+ SCMRatisServer server, long flushInterval) {
+ this.flushInterval = flushInterval;
+ this.transactionBuffer = transactionBuffer;
+ this.server = server;
+ }
+
+ @Override
+ public void run() {
+ if (transactionBuffer.shouldFlush(flushInterval)) {
+ 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();
+ transactionBuffer.setLatestSnapshot(null);
+ try {
+ server.doSnapshotRequest();
+ } catch (IOException e) {
+ LOG.error("Snapshot request is failed", e);
+ } finally {
+ // under failure case, if unable to take snapshot, its value
+ // is reset to previous known value
+ if (null == transactionBuffer.getLatestSnapshot()) {
+ transactionBuffer.setLatestSnapshot(lastSnapshot);
+ }
Review Comment:
It is still not working get and set are two different calls. We need to use `AtomicReference.compareAndSet`.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHAManagerImpl.java:
##########
@@ -92,6 +97,25 @@ public SCMHAManagerImpl(final ConfigurationSource conf,
}
}
+
+ private void createStartTransactionBufferMonitor() {
+ long interval = conf.getTimeDuration(
+ OZONE_SCM_HA_DBTRANSACTIONBUFFER_FLUSH_INTERVAL,
+ OZONE_SCM_HA_DBTRANSACTIONBUFFER_FLUSH_INTERVAL_DEFAULT,
+ TimeUnit.MILLISECONDS);
+ SCMHATransactionBufferMonitorTask monitorTask
+ = new SCMHATransactionBufferMonitorTask(
+ (SCMHADBTransactionBuffer) transactionBuffer, ratisServer, interval);
Review Comment:
I see. Could you move the method to right below the `start()` method?
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHADBTransactionBufferImpl.java:
##########
@@ -44,6 +45,8 @@ public class SCMHADBTransactionBufferImpl implements SCMHADBTransactionBuffer {
private BatchOperation currentBatchOperation;
private TransactionInfo latestTrxInfo;
private SnapshotInfo latestSnapshot;
+ private AtomicLong txFlushPending = new AtomicLong(0);
Review Comment:
Add `final`.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHATransactionBufferMonitorTask.java:
##########
@@ -0,0 +1,71 @@
+/*
+ * 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.
+ */
+package org.apache.hadoop.hdds.scm.ha;
+
+import java.io.IOException;
+import org.apache.ratis.statemachine.SnapshotInfo;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A background service running in SCM to check and flush the HA Transaction
+ * buffer.
+ */
+public class SCMHATransactionBufferMonitorTask implements Runnable {
+ public static final Logger LOG =
+ LoggerFactory.getLogger(SCMHATransactionBufferMonitorTask.class);
+ private final SCMRatisServer server;
+ private final SCMHADBTransactionBuffer transactionBuffer;
+ private final long flushInterval;
+
+ /**
+ * SCMService related variables.
+ */
+
+ @SuppressWarnings("parameternumber")
+ public SCMHATransactionBufferMonitorTask(
+ SCMHADBTransactionBuffer transactionBuffer,
+ SCMRatisServer server, long flushInterval) {
+ this.flushInterval = flushInterval;
+ this.transactionBuffer = transactionBuffer;
+ this.server = server;
+ }
+
+ @Override
+ public void run() {
+ if (transactionBuffer.shouldFlush(flushInterval)) {
+ 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();
Review Comment:
We should use `AtomicReference.getAndSet(null)`.
--
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
[GitHub] [ozone] sumitagrawl commented on pull request #4683: HDDS-8508. SCMHATransactionBuffer flush based on time
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on PR #4683:
URL: https://github.com/apache/ozone/pull/4683#issuecomment-1562800939
@szetszwo Thanks for suggestion for fixing latestSnapshot, I have fixed all comments. Plz 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
[GitHub] [ozone] Xushaohong commented on a diff in pull request #4683: HDDS-8508. SCMHATransactionBuffer flush based on time
Posted by "Xushaohong (via GitHub)" <gi...@apache.org>.
Xushaohong commented on code in PR #4683:
URL: https://github.com/apache/ozone/pull/4683#discussion_r1195139213
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/ScmConfigKeys.java:
##########
@@ -561,6 +561,10 @@ public final class ScmConfigKeys {
public static final long OZONE_SCM_HA_RATIS_SNAPSHOT_THRESHOLD_DEFAULT =
1000L;
+ public static final String OZONE_SCM_HA_RATIS_SNAPSHOT_GAP
+ = "raft.server.snapshot.creation.gap";
Review Comment:
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 "raft.server.snapshot.manual.creation.gap"?
--
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
[GitHub] [ozone] Xushaohong commented on a diff in pull request #4683: HDDS-8508. SCMHATransactionBuffer flush based on time
Posted by "Xushaohong (via GitHub)" <gi...@apache.org>.
Xushaohong commented on code in PR #4683:
URL: https://github.com/apache/ozone/pull/4683#discussion_r1195123425
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHATransactionBufferMonitorTask.java:
##########
@@ -0,0 +1,61 @@
+/**
+ * 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.
+ */
+package org.apache.hadoop.hdds.scm.ha;
+
+import java.io.IOException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A background service running in SCM to check and flush the HA Transaction
+ * buffer.
+ */
+public class SCMHATransactionBufferMonitorTask implements Runnable {
+ public static final Logger LOG =
+ LoggerFactory.getLogger(SCMHATransactionBufferMonitorTask.class);
+ private SCMRatisServer server;
+ private SCMHADBTransactionBuffer transactionBuffer;
+ private long flushInterval = 0;
+
+ /**
+ * SCMService related variables.
+ */
+
+ @SuppressWarnings("parameternumber")
+ public SCMHATransactionBufferMonitorTask(
+ SCMHADBTransactionBuffer transactionBuffer,
+ SCMRatisServer server, long flushInterval) {
+ this.flushInterval = flushInterval;
+ this.transactionBuffer = transactionBuffer;
+ this.server = server;
+ }
+
+ @Override
+ public void run() {
+ if (transactionBuffer.shouldFlush(flushInterval)) {
+ 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);
Review Comment:
I see. In Ratis, the gap needs this to bypass the limitation. Thx for the explanation.
--
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
[GitHub] [ozone] sumitagrawl commented on a diff in pull request #4683: HDDS-8508. SCMHATransactionBuffer flush based on time
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #4683:
URL: https://github.com/apache/ozone/pull/4683#discussion_r1204281536
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHAManagerImpl.java:
##########
@@ -67,6 +71,7 @@ public class SCMHAManagerImpl implements SCMHAManager {
// this should ideally be started only in a ratis leader
private final InterSCMGrpcProtocolService grpcServer;
+ private BackgroundSCMService trxBufferMonitorService = null;
Review Comment:
@szetszwo
SCMHAManagerImpl is initialized before SCMContext is created in scm object. So BackgroundSCMService will have null scmContext.
To avoid this, its initialized during start of the service. Due to this reason, it can not be final and need create during start() of the SCMHAManager
--
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
[GitHub] [ozone] Xushaohong commented on a diff in pull request #4683: HDDS-8508. SCMHATransactionBuffer flush based on time
Posted by "Xushaohong (via GitHub)" <gi...@apache.org>.
Xushaohong commented on code in PR #4683:
URL: https://github.com/apache/ozone/pull/4683#discussion_r1188397861
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHATransactionBufferMonitorTask.java:
##########
@@ -0,0 +1,61 @@
+/**
+ * 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.
+ */
+package org.apache.hadoop.hdds.scm.ha;
+
+import java.io.IOException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A background service running in SCM to check and flush the HA Transaction
+ * buffer.
+ */
+public class SCMHATransactionBufferMonitorTask implements Runnable {
+ public static final Logger LOG =
+ LoggerFactory.getLogger(SCMHATransactionBufferMonitorTask.class);
+ private SCMRatisServer server;
+ private SCMHADBTransactionBuffer transactionBuffer;
+ private long flushInterval = 0;
+
+ /**
+ * SCMService related variables.
+ */
+
+ @SuppressWarnings("parameternumber")
+ public SCMHATransactionBufferMonitorTask(
+ SCMHADBTransactionBuffer transactionBuffer,
+ SCMRatisServer server, long flushInterval) {
+ this.flushInterval = flushInterval;
+ this.transactionBuffer = transactionBuffer;
+ this.server = server;
+ }
+
+ @Override
+ public void run() {
+ if (transactionBuffer.shouldFlush(flushInterval)) {
+ 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);
Review Comment:
Is this necessary? What will happen if we do not set null
--
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
[GitHub] [ozone] Xushaohong commented on a diff in pull request #4683: HDDS-8508. SCMHATransactionBuffer flush based on time
Posted by "Xushaohong (via GitHub)" <gi...@apache.org>.
Xushaohong commented on code in PR #4683:
URL: https://github.com/apache/ozone/pull/4683#discussion_r1188388580
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHAManagerImpl.java:
##########
@@ -92,6 +97,27 @@ public SCMHAManagerImpl(final ConfigurationSource conf,
}
}
+
+ private void createStartTransactionBufferMonitor() {
+ long interval = conf.getTimeDuration(
+ OZONE_SCM_HA_DBTRANSACTIONBUFFER_FLUSH_INTERVAL,
+ OZONE_SCM_HA_DBTRANSACTIONBUFFER_FLUSH_INTERVAL_DEFAULT,
+ TimeUnit.MILLISECONDS);
+ SCMHATransactionBufferMonitorTask monitorTask
+ = new SCMHATransactionBufferMonitorTask(
+ (SCMHADBTransactionBuffer) transactionBuffer, ratisServer, interval);
+ trxBufferMonitorService =
+ new BackgroundSCMService.Builder().setClock(scm.getSystemClock())
+ .setScmContext(scm.getScmContext())
+ .setServiceName("SCMHATransactionMonitor")
+ .setIntervalInMillis(interval)
+ .setWaitTimeInMillis(interval)
+ .setPeriodicalTask(() -> {
+ monitorTask.run();
+ }).build();
Review Comment:
```suggestion
.setPeriodicalTask(monitorTask)
.build();
```
Could be simpler
--
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
[GitHub] [ozone] Xushaohong commented on a diff in pull request #4683: HDDS-8508. SCMHATransactionBuffer flush based on time
Posted by "Xushaohong (via GitHub)" <gi...@apache.org>.
Xushaohong commented on code in PR #4683:
URL: https://github.com/apache/ozone/pull/4683#discussion_r1196019799
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/ScmConfigKeys.java:
##########
@@ -562,7 +562,7 @@ public final class ScmConfigKeys {
1000L;
public static final String OZONE_SCM_HA_RATIS_SNAPSHOT_GAP
- = "raft.server.snapshot.creation.gap";
+ = "ozone.scm.ha.ratis.server.snapshot.creation.gap";
Review Comment:
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.
--
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
[GitHub] [ozone] sumitagrawl commented on pull request #4683: HDDS-8508. SCMHATransactionBuffer flush based on time
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on PR #4683:
URL: https://github.com/apache/ozone/pull/4683#issuecomment-1542309955
> 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
--
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
[GitHub] [ozone] Xushaohong commented on a diff in pull request #4683: HDDS-8508. SCMHATransactionBuffer flush based on time
Posted by "Xushaohong (via GitHub)" <gi...@apache.org>.
Xushaohong commented on code in PR #4683:
URL: https://github.com/apache/ozone/pull/4683#discussion_r1195162045
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/ScmConfigKeys.java:
##########
@@ -562,7 +562,7 @@ public final class ScmConfigKeys {
1000L;
public static final String OZONE_SCM_HA_RATIS_SNAPSHOT_GAP
- = "raft.server.snapshot.creation.gap";
+ = "ozone.scm.ha.ratis.server.snapshot.creation.gap";
Review Comment:
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?
--
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
[GitHub] [ozone] szetszwo merged pull request #4683: HDDS-8508. SCMHATransactionBuffer flush based on time
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo merged PR #4683:
URL: https://github.com/apache/ozone/pull/4683
--
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
[GitHub] [ozone] sumitagrawl commented on a diff in pull request #4683: HDDS-8508. SCMHATransactionBuffer flush based on time
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #4683:
URL: https://github.com/apache/ozone/pull/4683#discussion_r1204273502
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHAManagerImpl.java:
##########
@@ -92,6 +97,25 @@ public SCMHAManagerImpl(final ConfigurationSource conf,
}
}
+
+ private void createStartTransactionBufferMonitor() {
+ long interval = conf.getTimeDuration(
+ OZONE_SCM_HA_DBTRANSACTIONBUFFER_FLUSH_INTERVAL,
+ OZONE_SCM_HA_DBTRANSACTIONBUFFER_FLUSH_INTERVAL_DEFAULT,
+ TimeUnit.MILLISECONDS);
+ SCMHATransactionBufferMonitorTask monitorTask
+ = new SCMHATransactionBufferMonitorTask(
+ (SCMHADBTransactionBuffer) transactionBuffer, ratisServer, interval);
Review Comment:
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.
--
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
[GitHub] [ozone] Xushaohong commented on a diff in pull request #4683: HDDS-8508. SCMHATransactionBuffer flush based on time
Posted by "Xushaohong (via GitHub)" <gi...@apache.org>.
Xushaohong commented on code in PR #4683:
URL: https://github.com/apache/ozone/pull/4683#discussion_r1195123425
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHATransactionBufferMonitorTask.java:
##########
@@ -0,0 +1,61 @@
+/**
+ * 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.
+ */
+package org.apache.hadoop.hdds.scm.ha;
+
+import java.io.IOException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A background service running in SCM to check and flush the HA Transaction
+ * buffer.
+ */
+public class SCMHATransactionBufferMonitorTask implements Runnable {
+ public static final Logger LOG =
+ LoggerFactory.getLogger(SCMHATransactionBufferMonitorTask.class);
+ private SCMRatisServer server;
+ private SCMHADBTransactionBuffer transactionBuffer;
+ private long flushInterval = 0;
+
+ /**
+ * SCMService related variables.
+ */
+
+ @SuppressWarnings("parameternumber")
+ public SCMHATransactionBufferMonitorTask(
+ SCMHADBTransactionBuffer transactionBuffer,
+ SCMRatisServer server, long flushInterval) {
+ this.flushInterval = flushInterval;
+ this.transactionBuffer = transactionBuffer;
+ this.server = server;
+ }
+
+ @Override
+ public void run() {
+ if (transactionBuffer.shouldFlush(flushInterval)) {
+ 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);
Review Comment:
I see. In Ratis, the gap needs this to bypass the limitation. Thx for the explaination.
--
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
[GitHub] [ozone] sumitagrawl commented on a diff in pull request #4683: HDDS-8508. SCMHATransactionBuffer flush based on time
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #4683:
URL: https://github.com/apache/ozone/pull/4683#discussion_r1188682494
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHATransactionBufferMonitorTask.java:
##########
@@ -0,0 +1,61 @@
+/**
+ * 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.
+ */
+package org.apache.hadoop.hdds.scm.ha;
+
+import java.io.IOException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A background service running in SCM to check and flush the HA Transaction
+ * buffer.
+ */
+public class SCMHATransactionBufferMonitorTask implements Runnable {
+ public static final Logger LOG =
+ LoggerFactory.getLogger(SCMHATransactionBufferMonitorTask.class);
+ private SCMRatisServer server;
+ private SCMHADBTransactionBuffer transactionBuffer;
+ private long flushInterval = 0;
+
+ /**
+ * SCMService related variables.
+ */
+
+ @SuppressWarnings("parameternumber")
+ public SCMHATransactionBufferMonitorTask(
+ SCMHADBTransactionBuffer transactionBuffer,
+ SCMRatisServer server, long flushInterval) {
+ this.flushInterval = flushInterval;
+ this.transactionBuffer = transactionBuffer;
+ this.server = server;
+ }
+
+ @Override
+ public void run() {
+ if (transactionBuffer.shouldFlush(flushInterval)) {
+ 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);
Review Comment:
@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.
--
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
[GitHub] [ozone] Xushaohong commented on a diff in pull request #4683: HDDS-8508. SCMHATransactionBuffer flush based on time
Posted by "Xushaohong (via GitHub)" <gi...@apache.org>.
Xushaohong commented on code in PR #4683:
URL: https://github.com/apache/ozone/pull/4683#discussion_r1195139213
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/ScmConfigKeys.java:
##########
@@ -561,6 +561,10 @@ public final class ScmConfigKeys {
public static final long OZONE_SCM_HA_RATIS_SNAPSHOT_THRESHOLD_DEFAULT =
1000L;
+ public static final String OZONE_SCM_HA_RATIS_SNAPSHOT_GAP
+ = "raft.server.snapshot.creation.gap";
Review Comment:
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 "raft.server.snapshot.manual.creation.gap"?
--
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
[GitHub] [ozone] sumitagrawl commented on a diff in pull request #4683: HDDS-8508. SCMHATransactionBuffer flush based on time
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #4683:
URL: https://github.com/apache/ozone/pull/4683#discussion_r1195948340
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/ScmConfigKeys.java:
##########
@@ -562,7 +562,7 @@ public final class ScmConfigKeys {
1000L;
public static final String OZONE_SCM_HA_RATIS_SNAPSHOT_GAP
- = "raft.server.snapshot.creation.gap";
+ = "ozone.scm.ha.ratis.server.snapshot.creation.gap";
Review Comment:
@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.
--
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
[GitHub] [ozone] sumitagrawl commented on a diff in pull request #4683: HDDS-8508. SCMHATransactionBuffer flush based on time
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #4683:
URL: https://github.com/apache/ozone/pull/4683#discussion_r1204371528
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHATransactionBufferMonitorTask.java:
##########
@@ -0,0 +1,69 @@
+/**
+ * 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.
+ */
+package org.apache.hadoop.hdds.scm.ha;
+
+import java.io.IOException;
+import org.apache.ratis.statemachine.SnapshotInfo;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A background service running in SCM to check and flush the HA Transaction
+ * buffer.
+ */
+public class SCMHATransactionBufferMonitorTask implements Runnable {
+ public static final Logger LOG =
+ LoggerFactory.getLogger(SCMHATransactionBufferMonitorTask.class);
+ private SCMRatisServer server;
+ private SCMHADBTransactionBuffer transactionBuffer;
+ private long flushInterval = 0;
+
+ /**
+ * SCMService related variables.
+ */
+
+ @SuppressWarnings("parameternumber")
+ public SCMHATransactionBufferMonitorTask(
+ SCMHADBTransactionBuffer transactionBuffer,
+ SCMRatisServer server, long flushInterval) {
+ this.flushInterval = flushInterval;
+ this.transactionBuffer = transactionBuffer;
+ this.server = server;
+ }
+
+ @Override
+ public void run() {
+ if (transactionBuffer.shouldFlush(flushInterval)) {
+ 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();
+ transactionBuffer.setLatestSnapshot(null);
+ try {
+ server.doSnapshotRequest();
+ } catch (IOException e) {
+ LOG.error("Snapshot request is failed", e);
+ } finally {
+ // under failure case, if unable to take snapshot, its value
+ // is reset to previous known value
+ if (null == transactionBuffer.getLatestSnapshot()) {
+ transactionBuffer.setLatestSnapshot(lastSnapshot);
+ }
Review Comment:
Updated ... added lock while update or get snapshot
--
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
[GitHub] [ozone] szetszwo commented on a diff in pull request #4683: HDDS-8508. SCMHATransactionBuffer flush based on time
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #4683:
URL: https://github.com/apache/ozone/pull/4683#discussion_r1202648273
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHADBTransactionBufferImpl.java:
##########
@@ -61,6 +63,7 @@ public <KEY, VALUE> void addToBuffer(
Table<KEY, VALUE> table, KEY key, VALUE value) throws IOException {
rwLock.readLock().lock();
try {
+ txFlushPending++;
Review Comment:
It need AtomicLong or writeLock.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMRatisServerImpl.java:
##########
@@ -229,6 +230,25 @@ public SCMRatisResponse submitRequest(SCMRatisRequest request)
return SCMRatisResponse.decode(raftClientReply);
}
+ @Override
+ public boolean doSnapshotRequest() throws IOException {
Review Comment:
Let's call it `triggerSnapshot`.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHATransactionBufferMonitorTask.java:
##########
@@ -0,0 +1,69 @@
+/**
+ * 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.
+ */
+package org.apache.hadoop.hdds.scm.ha;
+
+import java.io.IOException;
+import org.apache.ratis.statemachine.SnapshotInfo;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A background service running in SCM to check and flush the HA Transaction
+ * buffer.
+ */
+public class SCMHATransactionBufferMonitorTask implements Runnable {
+ public static final Logger LOG =
+ LoggerFactory.getLogger(SCMHATransactionBufferMonitorTask.class);
+ private SCMRatisServer server;
+ private SCMHADBTransactionBuffer transactionBuffer;
+ private long flushInterval = 0;
+
+ /**
+ * SCMService related variables.
+ */
+
+ @SuppressWarnings("parameternumber")
+ public SCMHATransactionBufferMonitorTask(
+ SCMHADBTransactionBuffer transactionBuffer,
+ SCMRatisServer server, long flushInterval) {
+ this.flushInterval = flushInterval;
+ this.transactionBuffer = transactionBuffer;
+ this.server = server;
+ }
+
+ @Override
+ public void run() {
+ if (transactionBuffer.shouldFlush(flushInterval)) {
+ 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();
+ transactionBuffer.setLatestSnapshot(null);
+ try {
+ server.doSnapshotRequest();
+ } catch (IOException e) {
+ LOG.error("Snapshot request is failed", e);
+ } finally {
+ // under failure case, if unable to take snapshot, its value
+ // is reset to previous known value
+ if (null == transactionBuffer.getLatestSnapshot()) {
+ transactionBuffer.setLatestSnapshot(lastSnapshot);
+ }
Review Comment:
`getLatestSnapshot` and `setLatestSnapshot` are not thread safe.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHATransactionBufferMonitorTask.java:
##########
@@ -0,0 +1,69 @@
+/**
Review Comment:
Please don't use javadoc for license.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHATransactionBufferMonitorTask.java:
##########
@@ -0,0 +1,69 @@
+/**
+ * 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.
Review Comment:
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/SCMRatisServerImpl.java:
##########
@@ -229,6 +230,25 @@ public SCMRatisResponse submitRequest(SCMRatisRequest request)
return SCMRatisResponse.decode(raftClientReply);
}
+ @Override
+ public boolean doSnapshotRequest() throws IOException {
+ 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.");
Review Comment:
Declare `requestTimeout` as a field and move the check to the constructor.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHAManagerImpl.java:
##########
@@ -67,6 +71,7 @@ public class SCMHAManagerImpl implements SCMHAManager {
// this should ideally be started only in a ratis leader
private final InterSCMGrpcProtocolService grpcServer;
+ private BackgroundSCMService trxBufferMonitorService = null;
Review Comment:
Add `final`. Create it in the constructor.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMRatisServerImpl.java:
##########
@@ -229,6 +230,25 @@ public SCMRatisResponse submitRequest(SCMRatisRequest request)
return SCMRatisResponse.decode(raftClientReply);
}
+ @Override
+ public boolean doSnapshotRequest() throws IOException {
+ 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.");
+
+ final SnapshotManagementRequest req = SnapshotManagementRequest.newCreate(
+ clientId, getDivision().getId(), getDivision().getGroup().getGroupId(),
+ nextCallId(), requestTimeout);
+ final RaftClientReply raftClientReply = server.snapshotManagement(req);
+ if (!raftClientReply.isSuccess()) {
+ LOG.info("Snapshot request failed", raftClientReply.getException());
Review Comment:
Use `LOG.warn`.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHAManagerImpl.java:
##########
@@ -92,6 +97,25 @@ public SCMHAManagerImpl(final ConfigurationSource conf,
}
}
+
+ private void createStartTransactionBufferMonitor() {
+ long interval = conf.getTimeDuration(
+ OZONE_SCM_HA_DBTRANSACTIONBUFFER_FLUSH_INTERVAL,
+ OZONE_SCM_HA_DBTRANSACTIONBUFFER_FLUSH_INTERVAL_DEFAULT,
+ TimeUnit.MILLISECONDS);
+ SCMHATransactionBufferMonitorTask monitorTask
+ = new SCMHATransactionBufferMonitorTask(
+ (SCMHADBTransactionBuffer) transactionBuffer, ratisServer, interval);
Review Comment:
When HA is disabled, the cast will fail.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHATransactionBufferMonitorTask.java:
##########
@@ -0,0 +1,69 @@
+/**
+ * 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.
+ */
+package org.apache.hadoop.hdds.scm.ha;
+
+import java.io.IOException;
+import org.apache.ratis.statemachine.SnapshotInfo;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A background service running in SCM to check and flush the HA Transaction
+ * buffer.
+ */
+public class SCMHATransactionBufferMonitorTask implements Runnable {
+ public static final Logger LOG =
+ LoggerFactory.getLogger(SCMHATransactionBufferMonitorTask.class);
+ private SCMRatisServer server;
+ private SCMHADBTransactionBuffer transactionBuffer;
+ private long flushInterval = 0;
Review Comment:
Add `final`.
--
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