Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12124 )
Change subject: IMPALA-8071: Initial unified backend test framework ...................................................................... Patch Set 7: (4 comments) Looking good. I had some minor comments but I can +2 after http://gerrit.cloudera.org:8080/#/c/12124/7/be/src/service/unified-betest-main.cc File be/src/service/unified-betest-main.cc: http://gerrit.cloudera.org:8080/#/c/12124/7/be/src/service/unified-betest-main.cc@34 PS7, Line 34: for (int i = 0; i < argc; i++) { Ok, this is kinda hacky but I guess fine :) http://gerrit.cloudera.org:8080/#/c/12124/7/be/src/service/unified-betest-main.cc@36 PS7, Line 36: = space around parens http://gerrit.cloudera.org:8080/#/c/12124/7/be/src/util/CMakeLists.txt File be/src/util/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/12124/7/be/src/util/CMakeLists.txt@182 PS7, Line 182: # redactor-test and redactor-unconfigured-test both use redactor-test-util.h, which Pretty sure we just need to mark those functions in the header as inline to allow the linker to merge the symbols. Or we can move the functions to the .cc. It would be good to fix it now rather than leaving it for someone else to stumble on. http://gerrit.cloudera.org:8080/#/c/12124/7/bin/gen-backend-test-script.sh File bin/gen-backend-test-script.sh: http://gerrit.cloudera.org:8080/#/c/12124/7/bin/gen-backend-test-script.sh@24 PS7, Line 24: # This script is used by the build system and is not intended to be run directly. We don't necessarily need to fix, but I wonder if we should have a bin/buildsupport directory or similar for scripts like this that aren't meant to be run by developers - bin/ is pretty unapproachable now. -- To view, visit http://gerrit.cloudera.org:8080/12124 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia03ef38719b1fbc0fe2025e16b7b3d3dd4488842 Gerrit-Change-Number: 12124 Gerrit-PatchSet: 7 Gerrit-Owner: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Thu, 07 Feb 2019 23:55:37 +0000 Gerrit-HasComments: Yes
