Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/12407 )
Change subject: IMPALA-5043: diagnostics for topic staleness in AC ...................................................................... Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/12407/3/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/12407/3/be/src/scheduling/admission-controller.cc@49 PS3, Line 49: stale nit: repeated http://gerrit.cloudera.org:8080/#/c/12407/3/be/src/scheduling/admission-controller.cc@292 PS3, Line 292: } else { : last_topic_update_time_ms_ = MonotonicMillis(); this might be a bit misleading since this does not signify that the topic update was received. I think your intention was to keep the monotonicTime() - last_topic_update_time_ms_ be consistent, but how about we handle last_topic_update_time_ms_ == 0 as a separate case in GetStalenessDetailLocked() and mention that an update was never recieved http://gerrit.cloudera.org:8080/#/c/12407/3/be/src/scheduling/admission-controller.cc@1127 PS3, Line 1127: // Include a space before the sentence because we're appending to an existing : // sentence. nit: 'stale' comment? :P http://gerrit.cloudera.org:8080/#/c/12407/3/tests/custom_cluster/test_admission_controller.py File tests/custom_cluster/test_admission_controller.py: http://gerrit.cloudera.org:8080/#/c/12407/3/tests/custom_cluster/test_admission_controller.py@861 PS3, Line 861: STALE_TOPIC_THRESHOLD_MS nit: maybe move this to the top with other constants? (feel free to ignore this comment) http://gerrit.cloudera.org:8080/#/c/12407/3/tests/custom_cluster/test_admission_controller.py@919 PS3, Line 919: assert num_admitted_immediately == 1 : assert num_rejected == 1 : assert num_queued == NUM_QUERIES - 2 based on the AC config: num_queued == 3 num_rejected == NUM_QUERIES - 4 because if NUM_QUERIES increases, the the queries rejected would increase. -- To view, visit http://gerrit.cloudera.org:8080/12407 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib9e26adb6419589ccf7625e423356df45bee4ac9 Gerrit-Change-Number: 12407 Gerrit-PatchSet: 3 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Fri, 08 Feb 2019 19:49:35 +0000 Gerrit-HasComments: Yes
