Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/16083 )
Change subject: IMPALA-9829: Add Write Metrics for Spilling ...................................................................... Patch Set 10: (13 comments) Good change! I like the detailed unit tests. I think a few cosmetic changes are all that is needed. This may seem like a picky nit but in Impala code, comments start with a capital letter and end with a period. This may seem weirdly prescriptive but it does enhance readability. http://gerrit.cloudera.org:8080/#/c/16083/10//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16083/10//COMMIT_MSG@9 PS10, Line 9: Three types of metrics are added in disk-io-mgr: Nit: in the commit message it can be best to be more high level, and describe what is added in more descriptive terms. http://gerrit.cloudera.org:8080/#/c/16083/10/be/src/runtime/io/disk-io-mgr-test.cc File be/src/runtime/io/disk-io-mgr-test.cc: http://gerrit.cloudera.org:8080/#/c/16083/10/be/src/runtime/io/disk-io-mgr-test.cc@1648 PS10, Line 1648: // the write operations Finish comment with a period. http://gerrit.cloudera.org:8080/#/c/16083/10/be/src/runtime/io/disk-io-mgr-test.cc@1664 PS10, Line 1664: // Reset the Metric if it exists Add period. http://gerrit.cloudera.org:8080/#/c/16083/10/be/src/runtime/io/disk-io-mgr-test.cc@1686 PS10, Line 1686: // Issue a number of writes to the disks Add period. http://gerrit.cloudera.org:8080/#/c/16083/10/be/src/runtime/io/disk-io-mgr-test.cc@1706 PS10, Line 1706: // Check the count and max/min of the histogram metric Add period. http://gerrit.cloudera.org:8080/#/c/16083/10/be/src/runtime/io/disk-io-mgr-test.cc@1712 PS10, Line 1712: // The count should be added by num_ranges/num_disks per disk Add period. http://gerrit.cloudera.org:8080/#/c/16083/10/be/src/runtime/io/disk-io-mgr-test.cc@1714 PS10, Line 1714: // Check if the min and max of write size are the same as the written len Add period. http://gerrit.cloudera.org:8080/#/c/16083/10/be/src/runtime/io/disk-io-mgr-test.cc@1732 PS10, Line 1732: // Issue a writing operation to a non-existent tmp file path Add periods http://gerrit.cloudera.org:8080/#/c/16083/10/be/src/runtime/io/disk-io-mgr-test.cc@1737 PS10, Line 1737: string tmp_file = "/tmp/disk_io_mgr_test/MetricsOfWriteIoError"; Another test uses "/non-existent/file.txt" to indicate a non-existing file, this makes it clearer what is happening http://gerrit.cloudera.org:8080/#/c/16083/10/be/src/runtime/io/disk-io-mgr-test.cc@1739 PS10, Line 1739: // Reset the Metric if it exists Add period. http://gerrit.cloudera.org:8080/#/c/16083/10/be/src/runtime/io/disk-io-mgr-test.cc@1746 PS10, Line 1746: // Remove the path in case it exists Add period. http://gerrit.cloudera.org:8080/#/c/16083/10/be/src/runtime/io/disk-io-mgr-test.cc@1767 PS10, Line 1767: // One IO Error should be added to the metrics counter Add period. http://gerrit.cloudera.org:8080/#/c/16083/10/common/thrift/metrics.json File common/thrift/metrics.json: http://gerrit.cloudera.org:8080/#/c/16083/10/common/thrift/metrics.json@663 PS10, Line 663: "description": "The number of write io error on disk.", Should be "errors". -- To view, visit http://gerrit.cloudera.org:8080/16083 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I152b9c5339cedabe33f8873a2bbf651aa5dbb914 Gerrit-Change-Number: 16083 Gerrit-PatchSet: 10 Gerrit-Owner: Yida Wu <wydbaggio...@gmail.com> Gerrit-Reviewer: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Yida Wu <wydbaggio...@gmail.com> Gerrit-Comment-Date: Thu, 25 Jun 2020 17:36:10 +0000 Gerrit-HasComments: Yes
