Michael Brown has posted comments on this change. ( http://gerrit.cloudera.org:8080/9273 )
Change subject: IMPALA-5139: Update mvn-quiet.sh to print execution content to log file ...................................................................... Patch Set 5: (4 comments) > Line 38: I have tried to make the code more readable now Please use the inline commenting system to reply to comments left inline. Please reply using the "Done" button if you've finished them, or reply inline with a response otherwise. http://gerrit.cloudera.org:8080/#/c/9273/5/bin/mvn-quiet.sh File bin/mvn-quiet.sh: http://gerrit.cloudera.org:8080/#/c/9273/5/bin/mvn-quiet.sh@25 PS5, Line 25: $IMPALA_MVN_LOGS_DIR This should be quoted in case a parent directory has a space. http://gerrit.cloudera.org:8080/#/c/9273/5/bin/mvn-quiet.sh@27 PS5, Line 27: #Print the output of mvn commands to a dedicated log file in $IMPALA_HOME/logs/mvn/mvn.log I'm not sure this comment is needed. Typically comments should say why is something is done, not restate what is already happening in code. http://gerrit.cloudera.org:8080/#/c/9273/5/bin/mvn-quiet.sh@29 PS5, Line 29: echo -e " ========================================================================\ : \n Running mvn $IMPALA_MAVEN_OPTIONS $@ \ : \n Directory: $(pwd) \ : \n ========================================================================" >> $LOG_FILE I find this to be a little cluttered, with the backslashes and the \n at the beginning of each line. We also lose the previous verbosity we had in master, in which the console showed what how and from where mvn was being run. How about something like: cat << EOF | tee -a "$LOG_FILE" =============================================== Running mvn $IMPALA_MAVEN_OPTIONS $@ Directory $(pwd) =============================================== EOF This 1. Prints to the console 2. Saves the command context to the log 3. Reduces clutter of backslashes and \n chars. http://gerrit.cloudera.org:8080/#/c/9273/5/bin/mvn-quiet.sh@35 PS5, Line 35: mvn "$@" | tee -a $LOG_FILE | grep -E -e WARNING -e ERROR -e SUCCESS -e FAILURE -e Test; then > * I still think mvn being piped to mvn is an odd construction, and probably > something to be avoided Agreed. I don't see how that is necessary here. > * I strongly suspect that inside the if-block, as it stands, "echo [...] > exited with code $?" will *always* show an exit code of 0, regardless of > whether mvn failed or not. Inside the if block, $? will be 0 if mvn has failed. This is because pipefail is on, and because the condition: "! mvn ..." is 0. (The ! here is key. It negates the 1 returned when mvn fails). You can even use ! outside an if block: $ ! false $ echo $? 0 $ Look at it like this (from master): 1. mvn fails with mvn | grep => ${PIPESTATUS[@]} is 1 0. Overall status of this pipe is 1 since pipefail is set. 2. ! negates the 1 3. The if expression evaluation has completed, and $? is set to 0 for the next shell command to inspect. 4. If block is entered and $? is still 0 when echo runs. So you are half right: If we enter that block, $? will be 0, and that echo error message is misleading and wrong. But if mvn succeeds, and the rest of the pipeline succeeds, the if block will not be entered. $? would have been non-zero. > * That said, I do think that "exit 1" will execute if mvn failed, so at least > the script will fail as expected Agreed. > * we don't want to see the verbose output of mvn, but we want to capture it > in a log file Agreed. > * however, if there are lines ERRORS or WARNINGS or SUCCESS or FAILURE, we > want to send those lines to the console Agreed. > * in the chain of mvn | tee | grep, we want to exit with a non-zero status if > mvn fails, but we don't esepcially care about the exit status of tee and > certainly not grep We care less the exit status of mvn, really, we just need to know that it failed and exit with failure. Returning mvn's precise exit status would be a bonus, I guess. > I think that all of those things can be accomplished with something like this. > > set -uo pipefail # i.e., get rid of set -e > > [...] > > mvn $IMPALA_MAVEN_OPTIONS "$@" | tee -a $LOG_FILE | \ > grep -E -e WARNING -e ERROR -e SUCCESS -e FAILURE -e Test > > MVN_EXIT_STATUS=${PIPESTATUS[0]} > echo "mvn exited with ${MVN_EXIT_STATUS}" > exit ${MVN_EXIT_STATUS} I think your solution would work. In particular, the key expression is: > mvn $IMPALA_MAVEN_OPTIONS "$@" | tee -a $LOG_FILE | \ > grep -E -e WARNING -e ERROR -e SUCCESS -e FAILURE -e Test This sort of expression is what I expected Nithya to come up with on this patch, but directly in the if expression, and preserving !. -- To view, visit http://gerrit.cloudera.org:8080/9273 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I475b17a4dccfa624dda61402491b461c53473f8b Gerrit-Change-Number: 9273 Gerrit-PatchSet: 5 Gerrit-Owner: Nithya Janarthanan <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-Reviewer: Nithya Janarthanan <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Comment-Date: Thu, 15 Feb 2018 18:57:17 +0000 Gerrit-HasComments: Yes
