Pooja Nilangekar 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 6: (11 comments) http://gerrit.cloudera.org:8080/#/c/11052/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11052/5//COMMIT_MSG@16 PS5, Line 16: Testing: Added an end to end test to verify the 'time_since_heartbeat' : metric of a slow subscriber. : > sorry, one more thing I forgot to mention: can we add an automated e2e test Done http://gerrit.cloudera.org:8080/#/c/11052/5/be/src/statestore/statestore.h File be/src/statestore/statestore.h: http://gerrit.cloudera.org:8080/#/c/11052/5/be/src/statestore/statestore.h@444 PS5, Line 444: > nit: technically this should be atomic, right? The "monitor" thread is acce Done http://gerrit.cloudera.org:8080/#/c/11052/5/be/src/statestore/statestore.h@687 PS5, Line 687: rapidjson::Document* document); > the 'MainLoop' call seems to indicate that it would run until some exit fla Done. That comment was left around from quiet some time ago. The Statestore class no longer contains the exit_flag_ nor does it provide any API for graceful decommissioning. I have checked the current codebase and I can confirm that the statestore is never destructed. I have also added the destructor. http://gerrit.cloudera.org:8080/#/c/11052/5/be/src/statestore/statestore.cc File be/src/statestore/statestore.cc: http://gerrit.cloudera.org:8080/#/c/11052/5/be/src/statestore/statestore.cc@78 PS5, Line 78: s from a > nit: typo Done http://gerrit.cloudera.org:8080/#/c/11052/5/be/src/statestore/statestore.cc@315 PS5, Line 315: last_heartbeat_ts_(MonotonicMicros()) { > is there any race here when a subscriber first registers before it sends a Done http://gerrit.cloudera.org:8080/#/c/11052/5/be/src/statestore/statestore.cc@389 PS5, Line 389: DCHECK_GT(timestamp_us, last_heartbeat_ts_.Load()); > if these are wall times, it's possible for them to go backwards if the host Done http://gerrit.cloudera.org:8080/#/c/11052/5/be/src/statestore/statestore.cc@542 PS5, Line 542: > for the purposes of the user understanding this data, I'd think it would be Done http://gerrit.cloudera.org:8080/#/c/11052/5/be/src/statestore/statestore.cc@984 PS5, Line 984: } > You don't gain any real performance by declaring these outside of the loop, Done http://gerrit.cloudera.org:8080/#/c/11052/5/be/src/statestore/statestore.cc@990 PS5, Line 990: int num_subscribers; > maybe it would be clearer to not track this incrementally, but instead, jus Done http://gerrit.cloudera.org:8080/#/c/11052/5/be/src/statestore/statestore.cc@992 PS5, Line 992: AGS_heartbeat_monitoring_ > nit: i think using 'const auto&' is just as good here Done http://gerrit.cloudera.org:8080/#/c/11052/5/be/src/statestore/statestore.cc@998 PS5, Line 998: inactive_subscribers.push_back(subscriber.second->id()); > is it worth escalating this to WARN level if there are any inactive? Done -- 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: 6 Gerrit-Owner: Pooja Nilangekar <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Pooja Nilangekar <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Thu, 16 Aug 2018 22:54:48 +0000 Gerrit-HasComments: Yes
