You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org> on 2018/02/06 08:40:51 UTC
[Impala-ASF-CR] IMPALA-4835: Part 3: switch I/O buffers to buffer pool
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/8966 )
Change subject: IMPALA-4835: Part 3: switch I/O buffers to buffer pool
......................................................................
Patch Set 12:
(4 comments)
some early comments: overall looks pretty solid. will do another pass
http://gerrit.cloudera.org:8080/#/c/8966/12/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
http://gerrit.cloudera.org:8080/#/c/8966/12/be/src/exec/hdfs-parquet-scanner.cc@1707
PS12, Line 1707: 3
would it make sense to replace this with a named constant? something that appropriately ties together the use of 3 buffers and optimum reservation size.
http://gerrit.cloudera.org:8080/#/c/8966/12/be/src/exec/hdfs-parquet-scanner.cc@1724
PS12, Line 1724: bytes_to_add = 0;
would it be useful to squeeze max amount of buffers if we do this here:
if(reservation_to_distribute >= min_buffer_size)
bytes_to_add = BitUtil::RoundDownToPowerOfTwo(reservation_to_distribute)
http://gerrit.cloudera.org:8080/#/c/8966/12/be/src/exec/hdfs-scan-node-base.cc
File be/src/exec/hdfs-scan-node-base.cc:
http://gerrit.cloudera.org:8080/#/c/8966/12/be/src/exec/hdfs-scan-node-base.cc@347
PS12, Line 347: // We got the minimum reservation. Now try to get ideal reservation.
: if (resource_profile_.min_reservation != ideal_scan_range_reservation_) {
: ignore_result(buffer_pool_client_.IncreaseReservation(
: ideal_scan_range_reservation_ - resource_profile_.min_reservation));
: }
should we briefly document how ideal_scan_range_reservation_ is used as compared to min_scan_range_reservation_ in HdfsScanNode.java's class level comments?
http://gerrit.cloudera.org:8080/#/c/8966/12/be/src/exec/scanner-context.cc
File be/src/exec/scanner-context.cc:
http://gerrit.cloudera.org:8080/#/c/8966/12/be/src/exec/scanner-context.cc@161
PS12, Line 161: parent_->bp_client_,
is this the same clientHandle object used across different threads?
--
To view, visit http://gerrit.cloudera.org:8080/8966
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic09c6196b31e55b301df45cc56d0b72cfece6786
Gerrit-Change-Number: 8966
Gerrit-PatchSet: 12
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Feb 2018 08:40:51 +0000
Gerrit-HasComments: Yes