Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15215 )
Change subject: [clock] update on Clock interface ...................................................................... Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/15215/1/src/kudu/clock/logical_clock.h File src/kudu/clock/logical_clock.h: http://gerrit.cloudera.org:8080/#/c/15215/1/src/kudu/clock/logical_clock.h@51 PS1, Line 51: const scoped_refptr<MetricEntity>& metric_entity = {}); Why is it optional in the LogicalClock but required in the HybridClock? Certainly in production we'll always have the server entity; are there many more LogicalClock tests that don't set up an entity? FWIW, I think it'd be clearer if both clock types behaved the same way: either both required an entity, or it was optional for both. As is, it'll be hard to remember that there are different semantics for different clock types. -- To view, visit http://gerrit.cloudera.org:8080/15215 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic4c1944d54bf50e54c06c12e2fb9e57fc452b877 Gerrit-Change-Number: 15215 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Volodymyr Verovkin <[email protected]> Gerrit-Comment-Date: Fri, 14 Feb 2020 01:49:08 +0000 Gerrit-HasComments: Yes
