Laszlo Gaal has posted comments on this change. ( http://gerrit.cloudera.org:8080/23290 )
Change subject: IMPALA-14272: Add Sonar reporting capabilities ...................................................................... Patch Set 3: (3 comments) Thank you for the improvement proposal Peter. I have a kinda split opinion here: - I think that adding/extending code coverage reporting is a useful thing: any changes that help Apache Impala to be tested better are welcome, and coverage information is definitely informative. - OTOH the reporting script looks like something downstream-specific: -- Apache Impala does not have a Sonarqube instance for reporting (and I don't know about any plans to stand one up either) -- The script is not even called from anywhere IIUC so this looks more like a downstream infrastructure placeholder, which I would prefer not having in Apache Impala. I'd suggest adding a placeholder parameter to the GCOV call, using which the downstream environment could inject any additional formatting/reporting requirements into the GCOV reporting process, and moving the collection script and parameters to the downstream environment where the Sonarqube server presumably lives. To me this dicision seems to be the cleanest way to separate concerns/responsibilities, but I'm willing to be convinced otherwise. I'll add detailed suggestions to the code locations. http://gerrit.cloudera.org:8080/#/c/23290/3/bin/coverage_helper.sh File bin/coverage_helper.sh: http://gerrit.cloudera.org:8080/#/c/23290/3/bin/coverage_helper.sh@84 PS3, Line 84: --sonarqube="${REPORT_DIRECTORY}/sonar.xml" I'd suggest replacing the Sonarqube-specific parameter with a generic placeholder (e.g. ${EXTRA_COVERAGE_REPORTING_ARGUMENTS:-} or similar), with the necessary guarding about undefined/empty state. That would preserve the extension point while loosening the currently tight coupling to Sonarqube. http://gerrit.cloudera.org:8080/#/c/23290/3/bin/sonar_report.sh File bin/sonar_report.sh: http://gerrit.cloudera.org:8080/#/c/23290/3/bin/sonar_report.sh@1 PS3, Line 1: #!/bin/bash Consider moving this helper script to downstream http://gerrit.cloudera.org:8080/#/c/23290/3/sonar-project.properties File sonar-project.properties: http://gerrit.cloudera.org:8080/#/c/23290/3/sonar-project.properties@1 PS3, Line 1: # Licensed to the Apache Software Foundation (ASF) under one Consider moving the parameter file to downstream together with the reporting script. -- To view, visit http://gerrit.cloudera.org:8080/23290 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iea26c9967b62b06ded6a0cb4c0346f0e789beb80 Gerrit-Change-Number: 23290 Gerrit-PatchSet: 3 Gerrit-Owner: Peter Rozsa <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Laszlo Gaal <[email protected]> Gerrit-Reviewer: Peter Rozsa <[email protected]> Gerrit-Comment-Date: Tue, 11 Nov 2025 16:26:04 +0000 Gerrit-HasComments: Yes
