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 5: (9 comments) 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 > Good point! Done http://gerrit.cloudera.org:8080/#/c/21646/4/bin/jenkins/critique-gerrit-review.py@306 PS4, Line 306: old_line_num, "PARENT", diff_line, dryrun)) : elif OPTIONAL_FIELD_RE.match(change): : # Removing an optional field should be OK unless the field number is reused by : # a new optional field. A > In our Gerrit version (2.14.15), CommentInfo doesn't have 'context_line' ye Done http://gerrit.cloudera.org:8080/#/c/21646/5/bin/jenkins/critique-gerrit-review.py File bin/jenkins/critique-gerrit-review.py: http://gerrit.cloudera.org:8080/#/c/21646/5/bin/jenkins/critique-gerrit-review.py@48 PS5, Line 48: import sys This is unused? http://gerrit.cloudera.org:8080/#/c/21646/5/bin/jenkins/critique-gerrit-review.py@94 PS5, Line 94: THRIFT_FILE_COMMENT = \ : "This file is used in communication between impalad and catalogd/statestore. " \ : "Please make sure impalads can still work with new/old versions of catalogd and " \ : "statestore. Basically only new optional fields can be added." nit: For here and below, I think using parenthesis is tidier than using backslash to break string. THRIFT_FILE_COMMENT = ( "This file is used in communication between impalad and catalogd/statestore. " "Please make sure impalads can still work with new/old versions of catalogd and " "statestore. Basically only new optional fields can be added.") http://gerrit.cloudera.org:8080/#/c/21646/5/bin/jenkins/critique-gerrit-review.py@127 PS5, Line 127: 'REVISION' Make constant for 'REVISION' and 'PARENT'? http://gerrit.cloudera.org:8080/#/c/21646/5/bin/jenkins/critique-gerrit-review.py@272 PS5, Line 272: curr_file is not None nit: 'not curr_file' is shorter. http://gerrit.cloudera.org:8080/#/c/21646/5/bin/jenkins/critique-gerrit-review.py@314 PS5, Line 314: is_thrift_file This condition can be moved to L312 since the remaining checks below only applies for thrift files. http://gerrit.cloudera.org:8080/#/c/21646/5/bin/jenkins/critique-gerrit-review.py@335 PS5, Line 335: match and not check_source_file and\ : match.group(1) not in EXCLUDE_THRIFT_FILES nit: Use parentheses so backslash is not required. (match and not check_source_file and match.group(1) not in EXCLUDE_THRIFT_FILES) http://gerrit.cloudera.org:8080/#/c/21646/5/bin/jenkins/critique-gerrit-review.py@342 PS5, Line 342: elif check_source_file: Missing indentation here? critique-gerrit-review.py --dryrun --revision effc9df93 does not show anything anymore. -- 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: 5 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: Tue, 13 Aug 2024 18:06:51 +0000 Gerrit-HasComments: Yes
