You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "aswinshakil (via GitHub)" <gi...@apache.org> on 2025/01/22 00:38:27 UTC
[PR] HDDS-11345. Add metrics specific tests for reconciliation tasks. [ozone]
aswinshakil opened a new pull request, #7732:
URL: https://github.com/apache/ozone/pull/7732
## What changes were proposed in this pull request?
#7140 Added a framework to add metrics for any `AbstractReplicationTask`. This adds all the necessary metrics required for container reconciliation task(`ReconcileContainerTask`). This JIRA aims to add tests for these metrics.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-11345
## How was this patch tested?
Added unit tests.
--
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
Re: [PR] HDDS-11345. Add metrics specific tests for reconciliation tasks. [ozone]
Posted by "errose28 (via GitHub)" <gi...@apache.org>.
errose28 commented on PR #7732:
URL: https://github.com/apache/ozone/pull/7732#issuecomment-2643715724
@aswinshakil The [reverse merge](https://github.com/apache/ozone/commit/2cfd22a407c6cd8e5b06c4503390bfbfbea7c702) from master is done with green CI. Please merge this into your PR branch to retrigger CI with the new changes, which should fix the acceptance test issues.
--
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
Re: [PR] HDDS-11345. Add metrics specific tests for reconciliation tasks. [ozone]
Posted by "errose28 (via GitHub)" <gi...@apache.org>.
errose28 commented on code in PR #7732:
URL: https://github.com/apache/ozone/pull/7732#discussion_r1927649216
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/TestReconcileContainerCommandHandler.java:
##########
@@ -141,7 +144,8 @@ public void testReconcileContainerCommandReports(ContainerLayoutVersion layout)
*/
@ContainerLayoutTestInfo.ContainerTest
public void testReconcileContainerCommandMetrics(ContainerLayoutVersion layout) throws Exception {
- init(layout, c -> { });
+ init(layout, c -> {
Review Comment:
nit. we can remove the whitespace change here and on line 95
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/TestReconcileContainerCommandHandler.java:
##########
@@ -150,7 +154,17 @@ public void testReconcileContainerCommandMetrics(ContainerLayoutVersion layout)
ReconcileContainerCommand cmd = new ReconcileContainerCommand(id, Collections.emptySet());
subject.handle(cmd, ozoneContainer, context, null);
Review Comment:
I can't comment on the line directly but 152 has a comment from an older implementation we can remove here. I forgot to remove it in #7076 when we initially moved the timing handling to the supervisor
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/replication/TestReplicationSupervisor.java:
##########
@@ -513,6 +522,54 @@ public void testMultipleReplication(ContainerLayoutVersion layout,
}
}
+ @ContainerLayoutTestInfo.ContainerTest
+ public void testReconciliationTaskMetrics(ContainerLayoutVersion layout) throws IOException {
+ this.layoutVersion = layout;
+ // GIVEN
+ ReplicationSupervisor replicationSupervisor =
+ supervisorWithReplicator(FakeReplicator::new);
+ ReplicationSupervisorMetrics replicationMetrics =
+ ReplicationSupervisorMetrics.create(replicationSupervisor);
+
+ try {
+ //WHEN
+ replicationSupervisor.addTask(createReconciliationTask(1L));
+ replicationSupervisor.addTask(createReconciliationTask(2L));
+
+ ReconcileContainerTask reconciliationTask = createReconciliationTask(6L);
+ clock.fastForward(15000);
+ replicationSupervisor.addTask(reconciliationTask);
+ doThrow(IOException.class).when(mockController).reconcileContainer(any(), anyLong(), any());
+ replicationSupervisor.addTask(createReconciliationTask(7L));
+
+ //THEN
+ assertEquals(2, replicationSupervisor.getReplicationSuccessCount());
+
+ assertEquals(2, replicationSupervisor.getReplicationSuccessCount(
+ reconciliationTask.getMetricName()));
+ assertEquals(1, replicationSupervisor.getReplicationFailureCount());
+ assertEquals(1, replicationSupervisor.getReplicationFailureCount(
+ reconciliationTask.getMetricName()));
+ assertEquals(1, replicationSupervisor.getReplicationTimeoutCount());
+ assertEquals(1, replicationSupervisor.getReplicationTimeoutCount(
+ reconciliationTask.getMetricName()));
+ assertEquals(4, replicationSupervisor.getReplicationRequestCount());
+ assertEquals(4, replicationSupervisor.getReplicationRequestCount(
+ reconciliationTask.getMetricName()));
+
+
+ assertTrue(replicationSupervisor.getReplicationRequestTotalTime(
Review Comment:
Let's check `getReplicationRequestAvgTime` here too
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/ReconcileContainerCommandHandler.java:
##########
@@ -28,28 +28,28 @@
import org.apache.hadoop.ozone.protocol.commands.ReconcileContainerCommand;
import org.apache.hadoop.ozone.protocol.commands.SCMCommand;
-import java.util.concurrent.atomic.AtomicLong;
-
/**
* Handles commands from SCM to reconcile a container replica on this datanode with the replicas on its peers.
*/
public class ReconcileContainerCommandHandler implements CommandHandler {
private final ReplicationSupervisor supervisor;
- private final AtomicLong invocationCount;
private final DNContainerOperationClient dnClient;
+ private String metricsName;
public ReconcileContainerCommandHandler(ReplicationSupervisor supervisor, DNContainerOperationClient dnClient) {
this.supervisor = supervisor;
this.dnClient = dnClient;
- this.invocationCount = new AtomicLong(0);
}
@Override
public void handle(SCMCommand command, OzoneContainer container, StateContext context,
SCMConnectionManager connectionManager) {
- invocationCount.incrementAndGet();
ReconcileContainerCommand reconcileCommand = (ReconcileContainerCommand) command;
- supervisor.addTask(new ReconcileContainerTask(container.getController(), dnClient, reconcileCommand));
+ ReconcileContainerTask task = new ReconcileContainerTask(container.getController(), dnClient, reconcileCommand);
+ if (metricsName == null) {
Review Comment:
Just a note that I filed HDDS-12133 to improve this in general for commands going through the replication supervisor. For now we can leave it as is since it is consistent with other replication type commands.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -1449,6 +1449,10 @@ public void reconcileContainer(DNContainerOperationClient dnClient, Container<?>
Set<DatanodeDetails> peers) throws IOException {
// TODO Just a deterministic placeholder hash for testing until actual implementation is finished.
ContainerData data = container.getContainerData();
+ if (container.getContainerState() == State.DELETED) {
Review Comment:
I think this was intended for #7474
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/ContainerLogger.java:
##########
@@ -146,6 +146,15 @@ public static void logRecovered(ContainerData containerData) {
LOG.info(getMessage(containerData));
}
+ /**
+ * Logged when a container is reconciled.
+ *
+ * @param containerData The container that was reconciled on this datanode.
+ */
+ public static void logContainerReconciled(ContainerData containerData) {
+ LOG.info(getMessage(containerData, "Container reconciled"));
+ }
Review Comment:
In `getMessage`, let's start logging the data checksum for all containers. For the reconcile message, let's note the old and new checksum before and after reconciliation. These can passed as parameters to `logReconciled`
```suggestion
public static void logReconciled(ContainerData containerData, long oldDataChecksum, long newDataChecksum) {
LOG.info(getMessage(containerData, "Container reconciled. Old checksum is " ...)); // Format longs as hex strings and concatenate to the message. We want to make sure this format matches what comes out of `ozone admin container info`
}
```
--
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
Re: [PR] HDDS-11345. Add metrics specific tests for reconciliation tasks. [ozone]
Posted by "errose28 (via GitHub)" <gi...@apache.org>.
errose28 commented on PR #7732:
URL: https://github.com/apache/ozone/pull/7732#issuecomment-2613152649
We need #7699 in our branch to fix acceptance tests
--
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
Re: [PR] HDDS-11345. Add metrics specific tests for reconciliation tasks. [ozone]
Posted by "kerneltime (via GitHub)" <gi...@apache.org>.
kerneltime merged PR #7732:
URL: https://github.com/apache/ozone/pull/7732
--
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