Abhishek Rawat has posted comments on this change. ( http://gerrit.cloudera.org:8080/16318 )
Change subject: IMPALA-9867: Add Support for Spilling to S3: Milestone 1 ...................................................................... Patch Set 17: (17 comments) Some basic comments... http://gerrit.cloudera.org:8080/#/c/16318/13//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16318/13//COMMIT_MSG@18 PS13, Line 18: * TmpFile is separated into two types of implementation, TmpFileLocal nit: A new http://gerrit.cloudera.org:8080/#/c/16318/13//COMMIT_MSG@32 PS13, Line 32: DiskFile status of the buffer file would become INWRITING, and then nit: at the same time http://gerrit.cloudera.org:8080/#/c/16318/13//COMMIT_MSG@34 PS13, Line 34: nit: space after comma http://gerrit.cloudera.org:8080/#/c/16318/13//COMMIT_MSG@43 PS13, Line 43: : to specify the imple Maybe better to say: "all its pages have been pinned" http://gerrit.cloudera.org:8080/#/c/16318/13//COMMIT_MSG@54 PS13, Line 54: ease the efficiency of the file op Better to say "be returned during initialization" http://gerrit.cloudera.org:8080/#/c/16318/13//COMMIT_MSG@65 PS13, Line 65: of "Unit" http://gerrit.cloudera.org:8080/#/c/16318/17/be/src/runtime/io/disk-file.h File be/src/runtime/io/disk-file.h: http://gerrit.cloudera.org:8080/#/c/16318/17/be/src/runtime/io/disk-file.h@69 PS17, Line 69: /// or rewriting after deleted. "rewriting after deleted" is not very clear. I think you meant, writing to local buffer from remote? http://gerrit.cloudera.org:8080/#/c/16318/17/be/src/runtime/io/disk-file.h@133 PS17, Line 133: // Every time to query or modify the status, or need to gurrantee working under typo: guarantee http://gerrit.cloudera.org:8080/#/c/16318/17/be/src/runtime/io/disk-file.cc File be/src/runtime/io/disk-file.cc: http://gerrit.cloudera.org:8080/#/c/16318/17/be/src/runtime/io/disk-file.cc@24 PS17, Line 24: void DiskFile::SetPersisted() { I think these set function definitions could also be moved inside class body (default inline). http://gerrit.cloudera.org:8080/#/c/16318/13/be/src/runtime/io/disk-io-mgr.cc File be/src/runtime/io/disk-io-mgr.cc: http://gerrit.cloudera.org:8080/#/c/16318/13/be/src/runtime/io/disk-io-mgr.cc@109 PS13, Line 109: // The maximum number of HDFS I/O threads for doing file operations, such a file typos: 'a' -> 'as' http://gerrit.cloudera.org:8080/#/c/16318/13/be/src/runtime/io/disk-io-mgr.cc@120 PS13, Line 120: // The maximum number of S3 I/O threads for doing file operations, such a file typo: 'a' -> 'as' http://gerrit.cloudera.org:8080/#/c/16318/17/be/src/runtime/io/disk-io-mgr.cc File be/src/runtime/io/disk-io-mgr.cc: http://gerrit.cloudera.org:8080/#/c/16318/17/be/src/runtime/io/disk-io-mgr.cc@109 PS17, Line 109: // The maximum number of HDFS I/O threads for doing file operations, such a file typo "such a file" -> "such as file" http://gerrit.cloudera.org:8080/#/c/16318/17/be/src/runtime/io/disk-io-mgr.cc@110 PS17, Line 110: // uploading. The default value of 8 was chosen emperically to maximize HDFS throughput. typo: "emperically" -> "empirically" http://gerrit.cloudera.org:8080/#/c/16318/17/be/src/runtime/io/disk-io-mgr.cc@120 PS17, Line 120: // The maximum number of S3 I/O threads for doing file operations, such a file typo: "such a file" -> "such as file" http://gerrit.cloudera.org:8080/#/c/16318/17/be/src/runtime/io/disk-io-mgr.cc@121 PS17, Line 121: // uploading. The default value of 16 was chosen emperically to maximize S3 throughput. typo: "emperically" -> "empirically" http://gerrit.cloudera.org:8080/#/c/16318/17/be/src/runtime/io/disk-io-mgr.cc@233 PS17, Line 233: // DoWrite is using for Spilling. For spilling to local, we will typo: using -> used http://gerrit.cloudera.org:8080/#/c/16318/17/be/src/runtime/io/disk-io-mgr.cc@246 PS17, Line 246: ret_status = file_writer->Write(this, &is_ready); I think "is_full" might make more sense here rather than "is_ready". -- 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: 17 Gerrit-Owner: Yida Wu <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Mon, 19 Oct 2020 20:04:55 +0000 Gerrit-HasComments: Yes
