Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14332 )
Change subject: IMPALA-8962: FETCH_ROWS_TIMEOUT_MS should apply before rows are available ...................................................................... Patch Set 4: (2 comments) http://gerrit.cloudera.org:8080/#/c/14332/4/be/src/exec/blocking-plan-root-sink.h File be/src/exec/blocking-plan-root-sink.h: http://gerrit.cloudera.org:8080/#/c/14332/4/be/src/exec/blocking-plan-root-sink.h@72 PS4, Line 72: bool* eos, const uint64_t timeout_us) override; I'd prefer non-const for arguments that are passed by value, it just adds noise. http://gerrit.cloudera.org:8080/#/c/14332/4/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/14332/4/be/src/runtime/coordinator.cc@694 PS4, Line 694: max(static_cast<uint64_t>(1), I think there's still a possibility of underflow cause it's unsigned. I think this would all be simpler if we used int64_t for the timeouts throughout, since we don't need the extra range of the unsigned type. https://google.github.io/styleguide/cppguide.html#Integer_Types addresses this issue and says to use signed types in this situation. We could address the underflow locally, but the choice of signed vs unsigned is kinda viral because anywhere you convert to signed you have to deal with overflow. -- To view, visit http://gerrit.cloudera.org:8080/14332 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2cba6bf062dcc1af19471d21857caa797c1ea4a4 Gerrit-Change-Number: 14332 Gerrit-PatchSet: 4 Gerrit-Owner: Sahil Takiar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Thu, 03 Oct 2019 17:27:34 +0000 Gerrit-HasComments: Yes
