Copilot commented on code in PR #10100:
URL: https://github.com/apache/ozone/pull/10100#discussion_r3207239578
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/RatisUtil.java:
##########
@@ -231,9 +231,6 @@ private static void setRaftSnapshotProperties(
Snapshot.setAutoTriggerThreshold(properties,
ozoneConf.getLong(ScmConfigKeys.OZONE_SCM_HA_RATIS_SNAPSHOT_THRESHOLD,
ScmConfigKeys.OZONE_SCM_HA_RATIS_SNAPSHOT_THRESHOLD_DEFAULT));
- Snapshot.setCreationGap(properties,
- ozoneConf.getLong(ScmConfigKeys.OZONE_SCM_HA_RATIS_SNAPSHOT_GAP,
- ScmConfigKeys.OZONE_SCM_HA_RATIS_SNAPSHOT_GAP_DEFAULT));
}
Review Comment:
The PR description/title mention reducing the Ratis snapshot creation gap to
1, but this code now *removes* setting the snapshot creation gap entirely (no
call to `Snapshot.setCreationGap(...)`). That means the effective creation-gap
value will fall back to the Apache Ratis default, which may not be 1. Please
either update the PR description to reflect the new approach (decoupling SCM DB
flush from Ratis snapshot creation), or explicitly set the desired creation-gap
value here (or via another supported mechanism) and document the new behavior.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHATransactionBufferMonitorTask.java:
##########
@@ -29,38 +28,26 @@
public class SCMHATransactionBufferMonitorTask implements Runnable {
private static final Logger LOG =
LoggerFactory.getLogger(SCMHATransactionBufferMonitorTask.class);
- private final SCMRatisServer server;
private final SCMHADBTransactionBuffer transactionBuffer;
private final long flushInterval;
/**
* SCMService related variables.
*/
public SCMHATransactionBufferMonitorTask(
- SCMHADBTransactionBuffer transactionBuffer,
- SCMRatisServer server, long flushInterval) {
+ SCMHADBTransactionBuffer transactionBuffer, 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
- final SnapshotInfo lastSnapshot = transactionBuffer
- .getLatestSnapshotRef().getAndSet(null);
try {
- server.triggerSnapshot();
+ transactionBuffer.flush();
} 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
- transactionBuffer.getLatestSnapshotRef().compareAndSet(
- null, lastSnapshot);
+ LOG.error("TransactionFlushTask is failed", e);
Review Comment:
Log message grammar: "TransactionFlushTask is failed" is unidiomatic and
makes grepping/alerting harder. Consider changing it to something like
"TransactionFlushTask failed" (and optionally include context such as the
flushInterval or pending count if available).
--
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]
