Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files ...................................................................... Patch Set 20: (9 comments) Looks great! http://gerrit.cloudera.org:8080/#/c/17979/20//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17979/20//COMMIT_MSG@32 PS20, Line 32: If the system decides to fetch a block, the block will be : stored in the memory until all of the pages in the block are read : or the query ends. nit. suggest to present the positive case first. That is, moving it before "if not, we will go" at line 29. http://gerrit.cloudera.org:8080/#/c/17979/20//COMMIT_MSG@45 PS20, Line 45: the read buffer will be disabled. nit. May mention that the memory borrowed from the reserved memory will be returned immediately after use. http://gerrit.cloudera.org:8080/#/c/17979/20/be/src/runtime/tmp-file-mgr-test.cc File be/src/runtime/tmp-file-mgr-test.cc: http://gerrit.cloudera.org:8080/#/c/17979/20/be/src/runtime/tmp-file-mgr-test.cc@2183 PS20, Line 2183: // Check the metrics that we use the read buffer for reading. nit. 'we use' -> 'we did use'. http://gerrit.cloudera.org:8080/#/c/17979/20/be/src/runtime/tmp-file-mgr-test.cc@2200 PS20, Line 2200: f (buffer_size == small_size) To improve the readability, it looks like various IF tests inside this block can be avoided if a helper function testCapacityForReadBuffer(int64_t buffer_size) can be written that tests one buffer size only. http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/tmp-file-mgr.cc File be/src/runtime/tmp-file-mgr.cc: http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/tmp-file-mgr.cc@1070 PS13, Line 1070: nit exists http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/tmp-file-mgr.cc@1078 PS13, Line 1078: [read_buffer_block, tmp_file = this](const Status& fetch_status) { nit. Can this line be moved above 1074? That is, if remote batch reading is disabled, return null immediately. No need to do the work from 1074 to 1077. http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/tmp-file-mgr.cc@1104 PS13, Line 1104: ile* TmpFileRemote::GetR Looks like this function does more than what the function name implies. You may need to add a comment at least. Is there a strong need to do this check? http://gerrit.cloudera.org:8080/#/c/17979/20/be/src/util/mem-info.cc File be/src/util/mem-info.cc: http://gerrit.cloudera.org:8080/#/c/17979/20/be/src/util/mem-info.cc@265 PS20, Line 265: (process_avail_mem != nullptr) may just say if (process_avail_mem) *process_avail_mem = avail_mem; http://gerrit.cloudera.org:8080/#/c/17979/20/be/src/util/metrics.h File be/src/util/metrics.h: http://gerrit.cloudera.org:8080/#/c/17979/20/be/src/util/metrics.h@272 PS20, Line 272: /// The hwm value is also updated atomically. May add "Also return the updated current value." here. -- To view, visit http://gerrit.cloudera.org:8080/17979 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b Gerrit-Change-Number: 17979 Gerrit-PatchSet: 20 Gerrit-Owner: Yida Wu <wydbaggio...@gmail.com> Gerrit-Reviewer: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com> Gerrit-Reviewer: Yida Wu <wydbaggio...@gmail.com> Gerrit-Comment-Date: Tue, 13 Sep 2022 14:21:23 +0000 Gerrit-HasComments: Yes
