Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/22070 )
Change subject: IMPALA-13448: Logged cause when fails to flush lineage events, audit events or profiles ...................................................................... Patch Set 2: (10 comments) Thanks JiangWei for your contribution! I've left a few comments. http://gerrit.cloudera.org:8080/#/c/22070/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22070/2//COMMIT_MSG@7 PS2, Line 7: Logged cause when fails This would be better: "Log cause when failing to flush..." http://gerrit.cloudera.org:8080/#/c/22070/2//COMMIT_MSG@9 PS2, Line 9: When impala fails to flush lineage events, audit events or profiles, only the log file name is logged: "Could not open log file: filename". In the commit message, we have a line width limit of 72 characters (except for the title). http://gerrit.cloudera.org:8080/#/c/22070/2//COMMIT_MSG@10 PS2, Line 10: Now will "Now we will ..." would be better. http://gerrit.cloudera.org:8080/#/c/22070/2//COMMIT_MSG@11 PS2, Line 11: In commit messages we usually include a section describing the tests. This could be for example: Testing: - added custom cluster tests in test_logging.py http://gerrit.cloudera.org:8080/#/c/22070/2/be/src/util/simple-logger.h File be/src/util/simple-logger.h: http://gerrit.cloudera.org:8080/#/c/22070/2/be/src/util/simple-logger.h@22 PS2, Line 22: #include <cerrno> This could be included in the .cc file instead, reducing recompilation dependencies. http://gerrit.cloudera.org:8080/#/c/22070/2/be/src/util/simple-logger.cc File be/src/util/simple-logger.cc: http://gerrit.cloudera.org:8080/#/c/22070/2/be/src/util/simple-logger.cc@43 PS2, Line 43: using std::cerr; We're not using it, no need to declare "using". http://gerrit.cloudera.org:8080/#/c/22070/2/be/src/util/simple-logger.cc@137 PS2, Line 137: Could add ", cause: " http://gerrit.cloudera.org:8080/#/c/22070/2/tests/custom_cluster/test_logging.py File tests/custom_cluster/test_logging.py: http://gerrit.cloudera.org:8080/#/c/22070/2/tests/custom_cluster/test_logging.py@73 PS2, Line 73: Log_Flush_Failures_Dir Constants should be all capital: "LOG_FLUSH_FAILURES_DIR". http://gerrit.cloudera.org:8080/#/c/22070/2/tests/custom_cluster/test_logging.py@74 PS2, Line 74: """Test logging of failures to open log files with cause Permission denied.""" The doc string should come first in the class, before the constant on L73. http://gerrit.cloudera.org:8080/#/c/22070/2/tests/custom_cluster/test_logging.py@88 PS2, Line 88: self.assert_impalad_log_contains("INFO", "Permission denied", 2) This could be done in one assertion, something like "self.assert_impalad_log_contains("INFO", "Could not open log file: {0}, Permission denied", 2) -- To view, visit http://gerrit.cloudera.org:8080/22070 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b281d807e47aad98fc256af4e0c2a9dd417c7ac Gerrit-Change-Number: 22070 Gerrit-PatchSet: 2 Gerrit-Owner: jiangwei <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Fri, 15 Nov 2024 12:24:22 +0000 Gerrit-HasComments: Yes
