You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "sodonnel (via GitHub)" <gi...@apache.org> on 2023/03/14 17:30:10 UTC
[GitHub] [ozone] sodonnel opened a new pull request, #4399: HDDS-8158. Replication Manager: Make all handlers send commands immediately instead of returning commands
sodonnel opened a new pull request, #4399:
URL: https://github.com/apache/ozone/pull/4399
## What changes were proposed in this pull request?
To allow better throttling control all the unhealthy handlers should send the command directly using the RM API, rather than gathering up a list of commands and returning them.
This change involves a change to the UnhealthyReplicationHandler interface, which previously returned the commands, but now returns just the count of commands sent instead.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-8158
## How was this patch tested?
Various existing tests modified.
--
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] sodonnel commented on a diff in pull request #4399: HDDS-8158. Replication Manager: Make all handlers send commands immediately instead of returning commands
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on code in PR #4399:
URL: https://github.com/apache/ozone/pull/4399#discussion_r1138337363
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/RatisOverReplicationHandler.java:
##########
@@ -277,17 +282,18 @@ private Set<Pair<DatanodeDetails, SCMCommand<?>>> createCommands(
// iterate through replicas in deterministic order
for (ContainerReplica replica : replicas) {
if (excess == 0) {
- return commands;
+ return commandsSent;
}
if (super.isPlacementStatusActuallyEqualAfterRemove(replicaSet, replica,
containerInfo.getReplicationFactor().getNumber())) {
- commands.add(Pair.of(replica.getDatanodeDetails(),
- createDeleteCommand(containerInfo)));
+ replicationManager.sendDeleteCommand(containerInfo,
+ replica.getReplicaIndex(), replica.getDatanodeDetails(), true);
+ commandsSent++;
excess--;
}
}
- return commands;
+ return commandsSent;
}
private DeleteContainerCommand createDeleteCommand(ContainerInfo container) {
Review Comment:
I missed that, thanks. Removed it now.
--
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] siddhantsangwan commented on a diff in pull request #4399: HDDS-8158. Replication Manager: Make all handlers send commands immediately instead of returning commands
Posted by "siddhantsangwan (via GitHub)" <gi...@apache.org>.
siddhantsangwan commented on code in PR #4399:
URL: https://github.com/apache/ozone/pull/4399#discussion_r1138173257
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/TestOverReplicatedProcessor.java:
##########
@@ -78,23 +71,17 @@ public void setup() {
}
@Test
- public void testDeleteContainerCommand() throws IOException {
+ public void testSuccessfulRun() throws IOException {
ContainerInfo container = ReplicationTestUtil
.createContainer(HddsProtos.LifeCycleState.CLOSED, repConfig);
queue.enqueue(new OverReplicatedHealthResult(
container, 3, false));
- Set<Pair<DatanodeDetails, SCMCommand<?>>> commands = new HashSet<>();
- DeleteContainerCommand cmd =
- new DeleteContainerCommand(container.getContainerID());
- cmd.setReplicaIndex(5);
- commands.add(Pair.of(MockDatanodeDetails.randomDatanodeDetails(), cmd));
- Mockito
- .when(replicationManager.processOverReplicatedContainer(any()))
- .thenReturn(commands);
+ Mockito.when(replicationManager.processOverReplicatedContainer(any()))
+ .thenReturn(1);
overReplicatedProcessor.processAll();
-
- Mockito.verify(replicationManager).sendDatanodeCommand(any(), any(), any());
Review Comment:
Why remove these verifications in this test and the next one?
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/TestUnderReplicatedProcessor.java:
##########
@@ -156,8 +100,6 @@ public void testMessageRequeuedOnException() throws IOException {
.thenThrow(new AssertionError("Should process only one item"));
underReplicatedProcessor.processAll();
- Mockito.verify(replicationManager, Mockito.times(0))
Review Comment:
Same question as above
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/RatisOverReplicationHandler.java:
##########
@@ -277,17 +282,18 @@ private Set<Pair<DatanodeDetails, SCMCommand<?>>> createCommands(
// iterate through replicas in deterministic order
for (ContainerReplica replica : replicas) {
if (excess == 0) {
- return commands;
+ return commandsSent;
}
if (super.isPlacementStatusActuallyEqualAfterRemove(replicaSet, replica,
containerInfo.getReplicationFactor().getNumber())) {
- commands.add(Pair.of(replica.getDatanodeDetails(),
- createDeleteCommand(containerInfo)));
+ replicationManager.sendDeleteCommand(containerInfo,
+ replica.getReplicaIndex(), replica.getDatanodeDetails(), true);
+ commandsSent++;
excess--;
}
}
- return commands;
+ return commandsSent;
}
private DeleteContainerCommand createDeleteCommand(ContainerInfo container) {
Review Comment:
We can remove this method since it's unused now.
--
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] sodonnel commented on a diff in pull request #4399: HDDS-8158. Replication Manager: Make all handlers send commands immediately instead of returning commands
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on code in PR #4399:
URL: https://github.com/apache/ozone/pull/4399#discussion_r1138343866
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/TestUnderReplicatedProcessor.java:
##########
@@ -156,8 +100,6 @@ public void testMessageRequeuedOnException() throws IOException {
.thenThrow(new AssertionError("Should process only one item"));
underReplicatedProcessor.processAll();
- Mockito.verify(replicationManager, Mockito.times(0))
Review Comment:
Same answer as above.
--
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] sodonnel merged pull request #4399: HDDS-8158. Replication Manager: Make all handlers send commands immediately instead of returning commands
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel merged PR #4399:
URL: https://github.com/apache/ozone/pull/4399
--
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] sodonnel commented on a diff in pull request #4399: HDDS-8158. Replication Manager: Make all handlers send commands immediately instead of returning commands
Posted by "sodonnel (via GitHub)" <gi...@apache.org>.
sodonnel commented on code in PR #4399:
URL: https://github.com/apache/ozone/pull/4399#discussion_r1138343205
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/TestOverReplicatedProcessor.java:
##########
@@ -78,23 +71,17 @@ public void setup() {
}
@Test
- public void testDeleteContainerCommand() throws IOException {
+ public void testSuccessfulRun() throws IOException {
ContainerInfo container = ReplicationTestUtil
.createContainer(HddsProtos.LifeCycleState.CLOSED, repConfig);
queue.enqueue(new OverReplicatedHealthResult(
container, 3, false));
- Set<Pair<DatanodeDetails, SCMCommand<?>>> commands = new HashSet<>();
- DeleteContainerCommand cmd =
- new DeleteContainerCommand(container.getContainerID());
- cmd.setReplicaIndex(5);
- commands.add(Pair.of(MockDatanodeDetails.randomDatanodeDetails(), cmd));
- Mockito
- .when(replicationManager.processOverReplicatedContainer(any()))
- .thenReturn(commands);
+ Mockito.when(replicationManager.processOverReplicatedContainer(any()))
+ .thenReturn(1);
overReplicatedProcessor.processAll();
-
- Mockito.verify(replicationManager).sendDatanodeCommand(any(), any(), any());
Review Comment:
I decided the tests here didn't need to validate the functionality of replicationManager actually sending commands. All the tests here need to do, is validate a successful run does not requeue the command, while a failed run does requeue, as that is basically the only thing the processor does now.
Sending the commands is internal to RM now (before RM returned the commands and then this processor sent them, so it was valid to test it does the sending), which we cannot really verify with a mocked RM anyway.
--
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