Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/11052 )
Change subject: IMPALA-6644: Add recent heartbeat timestamp into Statestore metric ...................................................................... Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/11052/2/be/src/statestore/statestore.h File be/src/statestore/statestore.h: http://gerrit.cloudera.org:8080/#/c/11052/2/be/src/statestore/statestore.h@442 PS2, Line 442: of a recent heartbeat ... of the most recently logged heartbeat ... http://gerrit.cloudera.org:8080/#/c/11052/2/be/src/statestore/statestore.h@446 PS2, Line 446: recent_heartbeat_timestamp_ This name sounds kind of misleading. To be more accurate, isn't it the " most recently logged heartbeat timestamp" ? Since the actual default heartbeat frequency is much smaller than the default 'statestore_heartbeat_log_frequency_seconds'. I'd suggest changing the name to 'recent_logged_heartbeat_ts_', and update the comments and related functions to reflect that too. http://gerrit.cloudera.org:8080/#/c/11052/2/be/src/statestore/statestore.cc File be/src/statestore/statestore.cc: http://gerrit.cloudera.org:8080/#/c/11052/2/be/src/statestore/statestore.cc@77 PS2, Line 77: statestore_heartbeat_log_frequency_seconds Just wondering if it's necessary to make this configurable. Are there scenarios where users may want to change this, and if so, what's the benefit they get from doing so? If we don't have a strong reason to keep this flag, my vote is to get rid of it and keep 60 seconds (or some reasonable value) as a constant. The more flags we add, the larger our test matrix needs to become, or we result in more untested code paths. So, I'd say we should add it only if it's absolutely necessary. http://gerrit.cloudera.org:8080/#/c/11052/2/be/src/statestore/statestore.cc@389 PS2, Line 389: DCHECK Use DCHECK_GT -- 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: 2 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-Comment-Date: Mon, 30 Jul 2018 18:33:12 +0000 Gerrit-HasComments: Yes
