Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/16318 )
Change subject: WIP: IMPALA-9867: Add Support for Spilling to S3: Milestone 1 ...................................................................... Patch Set 10: (10 comments) http://gerrit.cloudera.org:8080/#/c/16318/10//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16318/10//COMMIT_MSG@43 PS10, Line 43: A local buffer file can be evicted if it is in status REMOTE or it > typo REMOTE -> UPLOADED Done http://gerrit.cloudera.org:8080/#/c/16318/10/be/src/runtime/hdfs-fs-cache.h File be/src/runtime/hdfs-fs-cache.h: http://gerrit.cloudera.org:8080/#/c/16318/10/be/src/runtime/hdfs-fs-cache.h@57 PS10, Line 57: > Maybe use a typedef for the vector<std::pair<string, string>> to improve th Done http://gerrit.cloudera.org:8080/#/c/16318/10/be/src/runtime/io/disk-io-mgr.cc File be/src/runtime/io/disk-io-mgr.cc: http://gerrit.cloudera.org:8080/#/c/16318/10/be/src/runtime/io/disk-io-mgr.cc@110 PS10, Line 110: DEFINE_int32(num_remote_hdfs_file_oper_io_threads, 2, > Add a comment for the new startup flag. Done. Modified the default value for operation io threads either. http://gerrit.cloudera.org:8080/#/c/16318/10/be/src/runtime/io/disk-io-mgr.cc@240 PS10, Line 240: ScopedHistogramTimer > Should the write timer also involve the lock acquisition delays? Done. Yes, it is better to include the lock delays. http://gerrit.cloudera.org:8080/#/c/16318/10/be/src/runtime/io/disk-io-mgr.cc@285 PS10, Line 285: offset_ = file_offset; : disk_id_ = disk_id; : tmp_file_ = tmp_file; : io_mgr_ = io_mgr; > We could move all these to the initializer's list in the constructor. Done http://gerrit.cloudera.org:8080/#/c/16318/10/be/src/runtime/io/disk-io-mgr.cc@294 PS10, Line 294: disk_id_ = disk_id; : file_path_ = file_path; : io_mgr_ = io_mgr; > Same here, better to move to initializer's list. Done http://gerrit.cloudera.org:8080/#/c/16318/10/be/src/runtime/io/request-context.h File be/src/runtime/io/request-context.h: http://gerrit.cloudera.org:8080/#/c/16318/10/be/src/runtime/io/request-context.h@296 PS10, Line 296: void RemoteOperDone(RemoteOperRange* oper_range, const Status& write_status); > Would be good to add comments here. Done http://gerrit.cloudera.org:8080/#/c/16318/10/be/src/runtime/io/request-ranges.h File be/src/runtime/io/request-ranges.h: http://gerrit.cloudera.org:8080/#/c/16318/10/be/src/runtime/io/request-ranges.h@372 PS10, Line 372: bool > Adding comments here for the new functions would be good. Done http://gerrit.cloudera.org:8080/#/c/16318/10/be/src/runtime/tmp-file-mgr.cc File be/src/runtime/tmp-file-mgr.cc: http://gerrit.cloudera.org:8080/#/c/16318/10/be/src/runtime/tmp-file-mgr.cc@152 PS10, Line 152: > Don't think we're using this variable? Done http://gerrit.cloudera.org:8080/#/c/16318/10/be/src/runtime/tmp-file-mgr.cc@391 PS10, Line 391: s3a_options_ > Should we set s3a_options_ only for S3 and not for hdfs? Done. Move to the S3 path logic. -- To view, visit http://gerrit.cloudera.org:8080/16318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I419b1d5dbbfe35334d9f964c4b65e553579fdc89 Gerrit-Change-Number: 16318 Gerrit-PatchSet: 10 Gerrit-Owner: Yida Wu <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Wed, 16 Sep 2020 14:33:32 +0000 Gerrit-HasComments: Yes
