Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21646 )
Change subject: IMPALA-13240: Add gerrit comments for Thrift/FlatBuffers changes ...................................................................... Patch Set 4: (5 comments) http://gerrit.cloudera.org:8080/#/c/21646/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21646/4//COMMIT_MSG@27 PS4, Line 27: -revision nit: --revision http://gerrit.cloudera.org:8080/#/c/21646/3/bin/jenkins/critique-gerrit-review.py File bin/jenkins/critique-gerrit-review.py: http://gerrit.cloudera.org:8080/#/c/21646/3/bin/jenkins/critique-gerrit-review.py@353 PS3, Line 353: parser.add_argument("--dryrun", action='store_true', : help="Don't post comments back to gerrit") > Done. It seems flake-diff works when I compare two revisions. So I just add Done http://gerrit.cloudera.org:8080/#/c/21646/4/bin/jenkins/critique-gerrit-review.py File bin/jenkins/critique-gerrit-review.py: http://gerrit.cloudera.org:8080/#/c/21646/4/bin/jenkins/critique-gerrit-review.py@77 PS4, Line 77: # Thrift files that are not used in communication between impalad and catalogd/statestore : EXCLUDE_THRIFT_FILES I think this script should warn if there is a new 'include "XXX.thrift"' line where XXX.thrift is not listed in EXCLUDE_THRIFT_FILES. http://gerrit.cloudera.org:8080/#/c/21646/4/bin/jenkins/critique-gerrit-review.py@306 PS4, Line 306: comments[curr_file].append( : {"message": "Modifying/adding this required field might break the compatibility" : " between impalad and catalogd/statestore during upgrade", : "line": new_line_num}) It will be great if we can have a method to compose CommentInput and consistently use it everywhere. nit: When in dryrun mode, I wish I can see the diff_line that is being criticized. Perhaps we can append ContextLine in dryrun mode, so it will look like the following: { "comments": { "common/thrift/JniCatalog.thrift": [ { "message": "Modifying/adding this required field might break the compatibility between impalad and catalogd/statestore during upgrade", "line": 1070, "context_lines": [ { "line_number": 1070, "context_line": "+ 1: required i32 num_hms_events" } ] }, ... I understand that 'context_line' is a valid field for CommentInfo, but not for CommentInput. What do you think? http://gerrit.cloudera.org:8080/#/c/21646/4/bin/jenkins/critique-gerrit-review.py@360 PS4, Line 360: "--base_revision" nit: better to use dash consistently. "--base-revision" -- To view, visit http://gerrit.cloudera.org:8080/21646 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib35fafa50bfd38631312d22464df14d426f55346 Gerrit-Change-Number: 21646 Gerrit-PatchSet: 4 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Mon, 12 Aug 2024 16:25:05 +0000 Gerrit-HasComments: Yes
