Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/18126 )
Change subject: IMPALA-11068: Add query option to reduce scanner thread launch. ...................................................................... Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/18126/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18126/2//COMMIT_MSG@27 PS2, Line 27: intent Nit: could be "is intended to offer" http://gerrit.cloudera.org:8080/#/c/18126/2//COMMIT_MSG@30 PS2, Line 30: is not set Mention that we also fall back to the flag if the query option is set no zero or a negative value. http://gerrit.cloudera.org:8080/#/c/18126/2/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/18126/2/be/src/exec/hdfs-scan-node.cc@220 PS2, Line 220: from : // either of hdfs_scanner_thread_max_estimated_bytes flag Nit: it would be better like this: "from either the hdfs_scanner_thread_max_estimated_bytes flag ..." http://gerrit.cloudera.org:8080/#/c/18126/2/be/src/exec/hdfs-scan-node.cc@221 PS2, Line 221: or : // hdfs_scanner_non_reserved_bytes option Nit: it would be better like this: "or the HDFS_SCANNER_NON_RESERVED_BYTES query option." We could mention that the query option takes precedence over the flag if it is set. http://gerrit.cloudera.org:8080/#/c/18126/2/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/18126/2/common/thrift/ImpalaService.thrift@847 PS2, Line 847: by planner Nit: by the planner. http://gerrit.cloudera.org:8080/#/c/18126/2/common/thrift/ImpalaService.thrift@849 PS2, Line 849: thread Nit: threads. http://gerrit.cloudera.org:8080/#/c/18126/2/common/thrift/Query.thrift File common/thrift/Query.thrift: http://gerrit.cloudera.org:8080/#/c/18126/2/common/thrift/Query.thrift@665 PS2, Line 665: 166: optional i64 hdfs_scanner_non_reserved_bytes = 33554432 // 32MB Shouldn't the default be unset, i.e. for example -1? That way the original behaviour would be conserved, i.e. the 'hdfs_scanner_thread_max_estimated_bytes' flag would be effective unless this option is set. If the default value stays this, we should mention it also in the commit message. My reason for having this option unset by default is that a user may set the 'hdfs_scanner_thread_max_estimated_bytes' flag but it won't have any effect and it could be difficult for the user to find out why. -- To view, visit http://gerrit.cloudera.org:8080/18126 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I03cadf1230eed00d69f2890c82476c6861e37466 Gerrit-Change-Number: 18126 Gerrit-PatchSet: 2 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Tue, 12 Sep 2023 09:03:48 +0000 Gerrit-HasComments: Yes
