Quanlong Huang 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 6: (8 comments) 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 vir > This is unused? Yeah, removed it. http://gerrit.cloudera.org:8080/#/c/21646/5/bin/jenkins/critique-gerrit-review.py@94 PS5, Line 94: "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.") : FBS_FILE_COMMENT = ( > nit: For here and below, I think using parenthesis is tidier than using bac Done http://gerrit.cloudera.org:8080/#/c/21646/5/bin/jenkins/critique-gerrit-review.py@127 PS5, Line 127: > Make constant for 'REVISION' and 'PARENT'? Done http://gerrit.cloudera.org:8080/#/c/21646/5/bin/jenkins/critique-gerrit-review.py@245 PS5, Line 245: comments[curr_file].append(get_comment_input( > Please make sure you don't output sensitive data with ThriftDebugString(). This is a false positive alert. We should also check 'curr_file' is under be/src/. http://gerrit.cloudera.org:8080/#/c/21646/5/bin/jenkins/critique-gerrit-review.py@272 PS5, Line 272: tart of diff for a fi > nit: 'not curr_file' is shorter. Done http://gerrit.cloudera.org:8080/#/c/21646/5/bin/jenkins/critique-gerrit-review.py@314 PS5, Line 314: nge = diff_lin > This condition can be moved to L312 since the remaining checks below only a Done. Nice catch! http://gerrit.cloudera.org:8080/#/c/21646/5/bin/jenkins/critique-gerrit-review.py@335 PS5, Line 335: atch and not check_source_file and : match.group(1) not in EXCLUDE_THRIFT_FILES): > nit: Use parentheses so backslash is not required. Done 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 eff Oops, this is due to is_thrift_file=False when goes here.. Resolved the issue. -- 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: 6 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: Thu, 15 Aug 2024 02:15:11 +0000 Gerrit-HasComments: Yes
