Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/10977 )
Change subject: IMPALA-7296: bytes limit for row batch queue ...................................................................... Patch Set 5: (5 comments) http://gerrit.cloudera.org:8080/#/c/10977/5/be/src/util/blocking-queue-test.cc File be/src/util/blocking-queue-test.cc: http://gerrit.cloudera.org:8080/#/c/10977/5/be/src/util/blocking-queue-test.cc@97 PS5, Line 97: LONG_TIMEOUT_MICROS); Does it make the test timing sensitive ? Should we use a larger timeout ? http://gerrit.cloudera.org:8080/#/c/10977/5/be/src/util/blocking-queue.h File be/src/util/blocking-queue.h: http://gerrit.cloudera.org:8080/#/c/10977/5/be/src/util/blocking-queue.h@117 PS5, Line 117: ElemBytesFn()(*out); It's a bit unfortunate that each element goes through two indirect function calls (one in BlockingPut() and one in BlockingGet()) before it's consumed. Is it a reasonable alternative to modify BlockingPut() to take an extra parameter for the element size (with default value set to 0 for callers which don't care about size limit of the queue) and store the size for each entry as part of the queue ? I think it's reasonable to cap each entry to no more than 2GB so we can represent the size as int32_t. Would that be too much space overhead in the queue ? http://gerrit.cloudera.org:8080/#/c/10977/5/be/src/util/blocking-queue.h@152 PS5, Line 152: if (val_bytes > 0) Not sure why the need for if() here. I can only assume this is to avoid accessing put_bytes_enqueued_. Given the access to total_put_wait_time_ above, put_bytes_enqueued_ is most likely in the cache already. This complication is most likely not warranted given the speculative execution of modern CPU. If anything, this just adds unnecessary pressure to the branch predicator. http://gerrit.cloudera.org:8080/#/c/10977/5/be/src/util/blocking-queue.h@185 PS5, Line 185: if (val_bytes > 0) See comments above. http://gerrit.cloudera.org:8080/#/c/10977/5/be/src/util/blocking-queue.h@237 PS5, Line 237: bool HasCapacityInternal(const boost::unique_lock<boost::mutex>& lock, int64_t val_bytes) { nit: long line -- To view, visit http://gerrit.cloudera.org:8080/10977 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaa06d1d8da2a6d101efda08f620c0bf84a71e681 Gerrit-Change-Number: 10977 Gerrit-PatchSet: 5 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 25 Jul 2018 01:41:40 +0000 Gerrit-HasComments: Yes
