HDDS-5456. Inject a Clock into Replication Manager to allow timeouts to be tested #2425
Conversation
adoroszlai
left a comment
adoroszlai
left a comment
There was a problem hiding this comment.
Thanks @sodonnel for the improvement, LGTM, except minor (theoretical) comment on equals.
| serviceManager, | ||
| scmNodeManager); | ||
| scmNodeManager, | ||
| new MonotonicClock(ZoneId.of("UTC"))); |
There was a problem hiding this comment.
Is there any reason not to use ZoneOffset.UTC?
There was a problem hiding this comment.
No reason, except I didn't know about it :-) I will change it.
| if (obj instanceof MonotonicClock) { | ||
| return zoneId.equals(((MonotonicClock) obj).zoneId); |
There was a problem hiding this comment.
MonotonicClock should be final or equals should check class equality instead of instanceof. Otherwise equality of instances of a subclass and MonotonicClock would not be symmetric.
There was a problem hiding this comment.
Thanks for pointing this out. I will make it final to avoid this problem. I basically copied this code from the Java SystemClock class, and it was a final class, so that explains why this code is that way.
adoroszlai
left a comment
adoroszlai
left a comment
There was a problem hiding this comment.
Thanks @sodonnel for updating the patch.
|
Thanks for the review @adoroszlai. I have merged the change to master. |
2 participants
Replication Manager currently uses Time.monotonicNow() in a few places to check if a pending command should be timed out.
This is difficult to test without adding sleeps in the tests.
We should inject a Clock object to use rather than making calls to SystemTime directly to allow tests more flexibility.
What changes were proposed in this pull request?
(Please fill in changes proposed in this fix)
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-5456
How was this patch tested?
New Unit test