[Impala-ASF-CR] IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins 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 7: Verified+1


--
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: 7
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Thu, 22 Feb 2018 01:36:48 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/9273 )

Change subject: IMPALA-5139: Update mvn-quiet.sh to print execution content to 
log file
..

IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

Verified the changes to mvn-quiet.sh by trigerring an impala build by
running bootstrap_development.sh which invokes mvn-quiet at multiple
places. Verified the creation of mvn log file with the relevant content
in the $IMPALA_HOME/logs/mvn folder

Change-Id: I475b17a4dccfa624dda61402491b461c53473f8b
Reviewed-on: http://gerrit.cloudera.org:8080/9273
Reviewed-by: Michael Brown 
Tested-by: Impala Public Jenkins
---
M bin/impala-config.sh
M bin/mvn-quiet.sh
2 files changed, 15 insertions(+), 8 deletions(-)

Approvals:
  Michael Brown: Looks good to me, approved
  Impala Public Jenkins: Verified

--
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: merged
Gerrit-Change-Id: I475b17a4dccfa624dda61402491b461c53473f8b
Gerrit-Change-Number: 9273
Gerrit-PatchSet: 8
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Reviewer: Philip Zeyliger 

[Impala-ASF-CR] IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins 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 7:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1978/


--
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: 7
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Wed, 21 Feb 2018 21:59:45 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

[Michael Brown (Code Review)]

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 7:

> Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1973/

IMPALA-6394


--
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: 7
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Wed, 21 Feb 2018 21:59:10 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins 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 7: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1973/


--
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: 7
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Wed, 21 Feb 2018 21:19:43 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins 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 7:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1973/


--
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: 7
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Wed, 21 Feb 2018 17:43:35 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

[Michael Brown (Code Review)]

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 7: Code-Review+2

https://jenkins.impala.io/job/ubuntu-16.04-from-scratch/1344/artifact/Impala/logs_static/logs/mvn/
 confirms. Thanks!


--
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: 7
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Wed, 21 Feb 2018 17:42:25 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan 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 7:

https://jenkins.impala.io/job/gerrit-verify-dryrun-external/83/console - Gerrit 
dry run was successful


--
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: 7
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Wed, 21 Feb 2018 17:17:41 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan 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 6:

(7 comments)

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_DI
> Done
Done


http://gerrit.cloudera.org:8080/#/c/9273/5/bin/mvn-quiet.sh@27
PS5, Line 27: cat << EOF | tee -a "$LOG_FILE"
> Done
Done


http://gerrit.cloudera.org:8080/#/c/9273/5/bin/mvn-quiet.sh@29
PS5, Line 29: Running mvn $IMPALA_MAVEN_OPTIONS $@
: Directory $(pwd)
: 

: EOF
> Sure...will do
Done


http://gerrit.cloudera.org:8080/#/c/9273/5/bin/mvn-quiet.sh@35
PS5, Line 35:   echo "mvn $IMPALA_MAVEN_OPTIONS $@ exited with code $?"
> Done
Done


http://gerrit.cloudera.org:8080/#/c/9273/5/bin/mvn-quiet.sh@35
PS5, Line 35: ho "mvn $
> Done
Done


http://gerrit.cloudera.org:8080/#/c/9273/6/bin/mvn-quiet.sh
File bin/mvn-quiet.sh:

http://gerrit.cloudera.org:8080/#/c/9273/6/bin/mvn-quiet.sh@34
PS6, Line 34:
> Same as above will remove it before publishing it
Done


http://gerrit.cloudera.org:8080/#/c/9273/6/bin/mvn-quiet.sh@34
PS6, Line 34: if ! mvn $IMPALA_MAVEN_OPTIONS "$@" | tee -a "$LOG_FILE" | grep 
-E -e WARNING -e ERROR -e SUCCESS -e FAILURE -e Test;
> Yep...noticed itwas going to fix it before publishing the review.
Done



--
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: 6
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Tue, 20 Feb 2018 23:43:37 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan has uploaded a new patch set (#7). ( 
http://gerrit.cloudera.org:8080/9273 )

Change subject: IMPALA-5139: Update mvn-quiet.sh to print execution content to 
log file
..

IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

Verified the changes to mvn-quiet.sh by trigerring an impala build by
running bootstrap_development.sh which invokes mvn-quiet at multiple
places. Verified the creation of mvn log file with the relevant content
in the $IMPALA_HOME/logs/mvn folder

Change-Id: I475b17a4dccfa624dda61402491b461c53473f8b
---
M bin/impala-config.sh
M bin/mvn-quiet.sh
2 files changed, 15 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/9273/7
--
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: newpatchset
Gerrit-Change-Id: I475b17a4dccfa624dda61402491b461c53473f8b
Gerrit-Change-Number: 9273
Gerrit-PatchSet: 7
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Reviewer: Philip Zeyliger 

[Impala-ASF-CR] IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan has uploaded a new patch set (#6). ( 
http://gerrit.cloudera.org:8080/9273 )

Change subject: IMPALA-5139: Update mvn-quiet.sh to print execution content to 
log file
..

IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

Address David's review comment

remove white space

Update comments

code and comments cleanup mvn-quiet.sh

Fixed review comments

Change-Id: I475b17a4dccfa624dda61402491b461c53473f8b
---
M bin/impala-config.sh
M bin/mvn-quiet.sh
2 files changed, 15 insertions(+), 9 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/9273/6
--
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: newpatchset
Gerrit-Change-Id: I475b17a4dccfa624dda61402491b461c53473f8b
Gerrit-Change-Number: 9273
Gerrit-PatchSet: 6
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Reviewer: Philip Zeyliger 

[Impala-ASF-CR] IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan 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:

(1 comment)

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@35
PS5, Line 35: mvn "$@"
Regarding the pipe to mvn - my bad...I though I had removed it in the previous 
match I was trying to move around things to see the output and forgot to 
remove this.
Fixed the rest of the comments. Will push the changes soon.



--
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 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Thu, 15 Feb 2018 21:54:21 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan 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)

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.
Sure...will do


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 s
ok


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 th
Sure...will do


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 probab
Working on the review comments



--
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 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Thu, 15 Feb 2018 19:28:47 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

[Michael Brown (Code Review)]

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: 

[Impala-ASF-CR] IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan 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:

> Patch Set 3:
>
> (2 comments)Line 26 :  I have addressed this comment by keeping the exports 
> in impala-config.sh

Line 38: I have tried to make the code more readable now


--
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 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Thu, 15 Feb 2018 14:47:51 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan 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:

> Patch Set 1:
>
> I think this is a good start, but this patch be improved again. I concede too 
> that the IMPALA-5139 description doesn't provide full context. Let me provide 
> some now:
>
> - I think mvn-quiet.sh capturing of mvn INFO output should go into one or 
> more files rooted in $IMPALA_LOG_DIR. This will let Jenkins jobs which tend 
> to look in that place collect the mvn logs, too.
>
> - The working directory and args matter. You can see that's already captured 
> L25-26 of this file, but I think we need that info alongside all the maven 
> output and be written to the log.
>
> - More than one thing calls mvn-quiet.sh. You need a strategy for appending 
> to a log file or writing multiple log files. tee -a might be of assistance 
> here.

Adding my responses to the review comments long after I have addressed them,

- Thanks Michael. I have addressed your comments to
  --- Write the mvn outputs to a single log file
  --- log file located in $IMPALA_HOME/logs/mvn


--
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 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Thu, 15 Feb 2018 14:45:51 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

[David Knupp (Code Review)]

David Knupp 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:

(1 comment)

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@35
PS5, Line 35: mvn "$@" | tee -a $LOG_FILE | grep -E -e WARNING -e ERROR -e 
SUCCESS -e FAILURE -e Test; then
I'll repeat my caveat about not being any sort of bash expert. I recommend 
running my comments past someone else -- e.g., Phil Z. or Michael.

I feel like there is something that is still confusing about this code, and 
some of it has to do with this patch, but a lot of it preceded your patch as 
well.

* I still think mvn being piped to mvn is an odd construction, and probably 
something to be avoided

* I think that the combined interaction between "set -eo pipefail" and the 
if-block and the piping is a little hard to reason about

* 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

* That said, I do think that "exit 1" will execute if mvn failed, so at least 
the script will fail as expected


Stepping back for a minute to consider what we want this script to do, I think 
it's something like this:

* we don't want to see the verbose output of mvn, but we want to capture it in 
a log file

* however, if there are lines ERRORS or WARNINGS or SUCCESS or FAILURE, we want 
to send those lines to the console

* 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


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}


Getting rid of "set -e" means that we can continue to execute in this script 
even if mvn fails, but without having to resort to "if mvn [...]; then...".

Everything gets logged, but only lines matching our grep pattern go to console.

mvn exit status is captured in ${PIPESTATUS[0]}, but tee and grep status are 
ignored.

The script exits with the same status as mvn.

This manages to avoid piping mvn to mvn.

FYI, this is the page I read about to clarify my understanding of pipefail and 
PIPESTATUS:
https://unix.stackexchange.com/questions/14270/get-exit-status-of-process-thats-piped-to-another



--
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 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Thu, 15 Feb 2018 04:59:06 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan has uploaded a new patch set (#5). ( 
http://gerrit.cloudera.org:8080/9273 )

Change subject: IMPALA-5139: Update mvn-quiet.sh to print execution content to 
log file
..

IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

Address David's review comment

remove white space

Update comments

code and comments cleanup mvn-quiet.sh

Change-Id: I475b17a4dccfa624dda61402491b461c53473f8b
---
M bin/impala-config.sh
M bin/mvn-quiet.sh
2 files changed, 15 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/9273/5
--
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: newpatchset
Gerrit-Change-Id: I475b17a4dccfa624dda61402491b461c53473f8b
Gerrit-Change-Number: 9273
Gerrit-PatchSet: 5
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 

[Impala-ASF-CR] IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan has abandoned this change. ( 
http://gerrit.cloudera.org:8080/9308 )

Change subject: IMPALA-5139: Update mvn-quiet.sh to print execution content to 
log file
..


Abandoned
--
To view, visit http://gerrit.cloudera.org:8080/9308
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I83034a879643f74cfab141a50fba22e5fb07ab17
Gerrit-Change-Number: 9308
Gerrit-PatchSet: 1
Gerrit-Owner: Nithya Janarthanan 

[Impala-ASF-CR] IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9308


Change subject: IMPALA-5139: Update mvn-quiet.sh to print execution content to 
log file
..

IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

Change-Id: I475b17a4dccfa624dda61402491b461c53473f8b

Address David's review comment

remove white space

Update comments

code and comments cleanup mvn-quiet.sh

Change-Id: I83034a879643f74cfab141a50fba22e5fb07ab17
---
M bin/impala-config.sh
M bin/mvn-quiet.sh
2 files changed, 15 insertions(+), 8 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/9308/1
--
To view, visit http://gerrit.cloudera.org:8080/9308
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I83034a879643f74cfab141a50fba22e5fb07ab17
Gerrit-Change-Number: 9308
Gerrit-PatchSet: 1
Gerrit-Owner: Nithya Janarthanan 

[Impala-ASF-CR] IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan has uploaded a new patch set (#4). ( 
http://gerrit.cloudera.org:8080/9273 )

Change subject: IMPALA-5139: Update mvn-quiet.sh to print execution content to 
log file
..

IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

Change-Id: I475b17a4dccfa624dda61402491b461c53473f8b

Address David's review comment

Change-Id: I475b17a4dccfa624dda61402491b461c53473f8b
---
M bin/impala-config.sh
M bin/mvn-quiet.sh
2 files changed, 16 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/9273/4
--
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: newpatchset
Gerrit-Change-Id: I475b17a4dccfa624dda61402491b461c53473f8b
Gerrit-Change-Number: 9273
Gerrit-PatchSet: 4
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 

[Impala-ASF-CR] IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan has abandoned this change. ( 
http://gerrit.cloudera.org:8080/9303 )

Change subject: IMPALA-5139: Update mvn-quiet.sh to print execution content to 
log file
..


Abandoned
--
To view, visit http://gerrit.cloudera.org:8080/9303
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I37f27788410e8aab8c41266329556103a92864e3
Gerrit-Change-Number: 9303
Gerrit-PatchSet: 1
Gerrit-Owner: Nithya Janarthanan 

[Impala-ASF-CR] IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

[Nithya Janarthanan (Code Review)]

Nithya Janarthanan has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9303


Change subject: IMPALA-5139: Update mvn-quiet.sh to print execution content to 
log file
..

IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

Change-Id: I475b17a4dccfa624dda61402491b461c53473f8b

Address David's review comment

Change-Id: I475b17a4dccfa624dda61402491b461c53473f8b

remove white space

Change-Id: I37f27788410e8aab8c41266329556103a92864e3
---
M bin/impala-config.sh
M bin/mvn-quiet.sh
2 files changed, 15 insertions(+), 8 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/03/9303/1
--
To view, visit http://gerrit.cloudera.org:8080/9303
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I37f27788410e8aab8c41266329556103a92864e3
Gerrit-Change-Number: 9303
Gerrit-PatchSet: 1
Gerrit-Owner: Nithya Janarthanan 

[Impala-ASF-CR] IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

[Michael Brown (Code Review)]

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 1:

I think this is a good start, but this patch be improved again. I concede too 
that the IMPALA-5139 description doesn't provide full context. Let me provide 
some now:

- I think mvn-quiet.sh capturing of mvn INFO output should go into one or more 
files rooted in $IMPALA_LOG_DIR. This will let Jenkins jobs which tend to look 
in that place collect the mvn logs, too.

- The working directory and args matter. You can see that's already captured 
L25-26 of this file, but I think we need that info alongside all the maven 
output and be written to the log.

- More than one thing calls mvn-quiet.sh. You need a strategy for appending to 
a log file or writing multiple log files. tee -a might be of assistance here.


--
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: 1
Gerrit-Owner: njanartha...@cloudera.com
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Comment-Date: Mon, 12 Feb 2018 20:33:53 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

[Anonymous Coward (Code Review)]

njanartha...@cloudera.com has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9273


Change subject: IMPALA-5139: Update mvn-quiet.sh to print execution content to 
log file
..

IMPALA-5139: Update mvn-quiet.sh to print execution content to log file

Change-Id: I475b17a4dccfa624dda61402491b461c53473f8b
---
M bin/mvn-quiet.sh
1 file changed, 3 insertions(+), 1 deletion(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/9273/1
--
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: newchange
Gerrit-Change-Id: I475b17a4dccfa624dda61402491b461c53473f8b
Gerrit-Change-Number: 9273
Gerrit-PatchSet: 1
Gerrit-Owner: njanartha...@cloudera.com
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown