Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/12401 )
Change subject: IMPALA-8064: Improve observability of wait times for runtime filters ...................................................................... Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/12401/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12401/1//COMMIT_MSG@10 PS1, Line 10: Previously, the wait time was calculated with : respect to the ScanNode::Init() function while duration was logged : with respect to the ScanNode::WaitForRuntimeFilters() function. The filter wait time counts against the elapsed time since the filter's registration in ScanNode::Init() while duration logged in ScanNode::WaitForRuntimeFilters() is the time spent in the function waiting for all filters to arrive. This could be misleading as it doesn't account for the elapsed time spent between ScanNode::Init() and ScanNode::WaitForRuntimeFilters(). http://gerrit.cloudera.org:8080/#/c/12401/1//COMMIT_MSG@18 PS1, Line 18: to http://gerrit.cloudera.org:8080/#/c/12401/1/be/src/exec/scan-node.cc File be/src/exec/scan-node.cc: http://gerrit.cloudera.org:8080/#/c/12401/1/be/src/exec/scan-node.cc@179 PS1, Line 179: max_wait_time = max(max_wait_time, ctx.filter->time_elapsed()); If I understand it correctly, the point of these changes is to reflect the fact that "FLAGS_runtime_filter_wait_time_ms" is essentially the maximum tolerable elapsed time between the filter's registration and arrival time, which potentially is longer than the elapsed time spent in this function. While I agree it's useful to show the maximum elapsed time between registration and arrival for all filters, it seems that the original wait time here is also useful for helping us understand the actual wall clock time spent in this function waiting for filters to arrive. The time spent in this function directly adds to the delay on issuing the initial scan ranges and thus how fast we return rows. So, what do you think about keeping the original wait time while calling the max elapsed time between registration and arrival "max arrival delay". http://gerrit.cloudera.org:8080/#/c/12401/1/be/src/runtime/runtime-filter.h File be/src/runtime/runtime-filter.h: http://gerrit.cloudera.org:8080/#/c/12401/1/be/src/runtime/runtime-filter.h@79 PS1, Line 79: /// Returns the amount of time waited since registration for the filter to , in ms, http://gerrit.cloudera.org:8080/#/c/12401/1/be/src/runtime/runtime-filter.h@81 PS1, Line 81: int32_t Either set this to int64_t or convert time_elapsed() below to return int32_t. I suppose int32_t can tolerate delays up to 24 days. http://gerrit.cloudera.org:8080/#/c/12401/1/be/src/runtime/runtime-filter.h@86 PS1, Line 86: time in millisecond -- To view, visit http://gerrit.cloudera.org:8080/12401 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I28fd45e75c773bc01d424f5a179ae186ee9b7469 Gerrit-Change-Number: 12401 Gerrit-PatchSet: 1 Gerrit-Owner: Pooja Nilangekar <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Pooja Nilangekar <[email protected]> Gerrit-Comment-Date: Fri, 08 Feb 2019 07:37:34 +0000 Gerrit-HasComments: Yes
