Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add last heartbeat timestamp into Statestore metric ...................................................................... Patch Set 8: (10 comments) http://gerrit.cloudera.org:8080/#/c/11052/8/be/src/statestore/statestore.h File be/src/statestore/statestore.h: http://gerrit.cloudera.org:8080/#/c/11052/8/be/src/statestore/statestore.h@420 PS8, Line 420: /// Sets the subscriber's last heartbeat timestamp to the given value. nit: can you clarify what the reference point for this timestamp is? ie is this a wall time or monotonic time? You might consider using the kudu::MonoTime class here to make that clear in lieu of a comment (not sure if you have a similar wrapper, but that should already be imported in kudu/util) http://gerrit.cloudera.org:8080/#/c/11052/8/be/src/statestore/statestore.h@445 PS8, Line 445: microseconds same as above http://gerrit.cloudera.org:8080/#/c/11052/8/be/src/statestore/statestore.h@538 PS8, Line 538: AtomicBool why does this need to be atomic? shouldn't we assume that initialization is single-threaded and the object should not be exposed to other threads until after initialization? http://gerrit.cloudera.org:8080/#/c/11052/8/be/src/statestore/statestore.cc File be/src/statestore/statestore.cc: http://gerrit.cloudera.org:8080/#/c/11052/8/be/src/statestore/statestore.cc@77 PS8, Line 77: DEFINE_int32 perhaps this should be _hidden unless we have some use case in mind where an admin would want to tune it? http://gerrit.cloudera.org:8080/#/c/11052/8/be/src/statestore/statestore.cc@389 PS8, Line 389: DCHECK_GT(timestamp_us, last_heartbeat_ts_.Load()); should this be _GE? it's possible (though unlikely) that we get called in rapid succession. http://gerrit.cloudera.org:8080/#/c/11052/8/be/src/statestore/statestore.cc@445 PS8, Line 445: if (initialized_.Load()) LOG(FATAL) this can just be CHECK(!initialized_.Load()) http://gerrit.cloudera.org:8080/#/c/11052/8/be/src/statestore/statestore.cc@552 PS8, Line 552: time_since_heartbeat can you name this secs_since_heartbeat so the unit is clear? http://gerrit.cloudera.org:8080/#/c/11052/8/be/src/statestore/statestore.cc@916 PS8, Line 916: SetLastHeartbeatTimestamp(MonotonicMicros()); Given the argument to this method is always going to be MonotonicMicros(), what if the method was just called 'RefreshHeartbeatTimestamp()' or something and internally it called MonotonicMicros? Then, instead of the getter, you could have 'double TimeSinceHeartbeaat()' which calculates the subtraction. That would encapsulate the timing mechanism and units internal to the Subscriber class and only expose a minimal interface to callers http://gerrit.cloudera.org:8080/#/c/11052/8/be/src/statestore/statestore.cc@1002 PS8, Line 1002: .size() == 0 empty http://gerrit.cloudera.org:8080/#/c/11052/8/www/statestore_subscribers.tmpl File www/statestore_subscribers.tmpl: http://gerrit.cloudera.org:8080/#/c/11052/8/www/statestore_subscribers.tmpl@31 PS8, Line 31: Time since last heartbeat (seconds) more concise: "Seconds since last heartbeat" -- To view, visit http://gerrit.cloudera.org:8080/11052 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I754adccc4569e8219d5d01500cccdfc8782953f7 Gerrit-Change-Number: 11052 Gerrit-PatchSet: 8 Gerrit-Owner: Pooja Nilangekar <pooja.nilange...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Pooja Nilangekar <pooja.nilange...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Fri, 24 Aug 2018 16:15:13 +0000 Gerrit-HasComments: Yes
