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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
