aeron icon indicating copy to clipboard operation
aeron copied to clipboard

[Java] Modified ClusterClock and ConsensusModuleAgent to better support time travel tests.

Open kwbc opened this issue 2 years ago • 5 comments

ConsensusModuleAgent generally uses ClusterClock.time() for log timestamps and ClusterClock.timeNanos() for internal time-related tasks. During time travel tests we want adjust/offset the log timestamps only. Modified ConsensusModuleAgent to use ClusterClock.convertToNanos instead of timeUnit.convert to allow the implementations of ClusterClock to differentiate between timestamps to be used in the log versus internal time-keeping. Also modified Election to take both the log timestamp and nano time in several methods to avoid converting back from nanoseconds to log time.

Final note: debated about what to call the log-timestamp parameter. Went with 'timestamp', but perhaps 'now' or 'nowLog' would be better?

kwbc avatar Jan 13 '22 23:01 kwbc

Overall I'm happy with the change (some small comments mentioned).

One question the addition of ClusterClock::convertToNanos. I'm not sure I completely understand the necessity of it, would it be possible to show with a simple test/use case it's value over calling clusterClock.timeUnit().toNanos() directly?

Final note: debated about what to call the log-timestamp parameter. Went with 'timestamp', but perhaps 'now' or 'nowLog' would be better?

I think timestamp is a bit generic. Not sure what the best name should be (nowClusterUnits is a bit wordy, logTimestamp is another option).

mikeb01 avatar Jan 16 '22 21:01 mikeb01

One question the addition of ClusterClock::convertToNanos. I'm not sure I completely understand the necessity of it, would it be possible to show with a simple test/use case it's value over calling clusterClock.timeUnit().toNanos() directly?

In our time travel implementation of ClusterClock, time() returns epochNanos() + timeTravelOffset, and convertToNanos(time) returns time - timeTravelOffset thus converting the adjusted log timestamp back to real wall-clock time for internal time keeping purposes within the consensus module. Does that make sense?

Would be simpler/clearer if consensus module used ClusterClock for log timestamps (and timers) only, and some other EpochClock for everything else. But that would impose a performance penalty having to call ClusterClock.time and EpochClock.time in the same duty-cycle.

kwbc avatar Jan 17 '22 04:01 kwbc

I think timestamp is a bit generic. Not sure what the best name should be (nowClusterUnits is a bit wordy, logTimestamp is another option).

I like logTimestamp. logClusterUnit is a bit wordy, but more consistent with nowNs and only slightly longer. Anyone else have an opinion?

kwbc avatar Jan 17 '22 04:01 kwbc

Thanks for the code review comments. I've renamed timestamp parameters to logTimestamp in ConsensusModuleAgent and Election -- like how it is consistent with logPosition, etc. And other requested parameter renames.

kwbc avatar Jan 19 '22 04:01 kwbc

I'll review this Friday or over the weekend.

mjpt777 avatar Jan 19 '22 14:01 mjpt777