HDDS-8158. Replication Manager: Make all handlers send commands immediately instead of returning commands #4399
Conversation
…commands and fix up all related classes and tests
There was a problem hiding this comment.
@sodonnel Nicely done change. Mostly looks good, just a few minor comments.
| excess--; | ||
| } | ||
| } | ||
| return commands; | ||
| return commandsSent; | ||
| } | ||
|
|
||
| private DeleteContainerCommand createDeleteCommand(ContainerInfo container) { |
There was a problem hiding this comment.
We can remove this method since it's unused now.
There was a problem hiding this comment.
I missed that, thanks. Removed it now.
| overReplicatedProcessor.processAll(); | ||
|
|
||
| Mockito.verify(replicationManager).sendDatanodeCommand(any(), any(), any()); |
There was a problem hiding this comment.
Why remove these verifications in this test and the next one?
There was a problem hiding this 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.
| @@ -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)) | |||
There was a problem hiding this comment.
Same question as above
There was a problem hiding this comment.
Same answer as above.
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.