You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Sailesh Mukil (Code Review)" <ge...@cloudera.org> on 2016/09/14 04:21:19 UTC

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse 

Sailesh Mukil has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/4411

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
......................................................................

Make gen_build_version.py resilient to a failing git rev-parse

It was noticed that some build processes did not checkout Impala and
instead built it from a tarball. This would cause our gen_build_version
script to write a blank version info everytime to the version.info file.

This patch takes care of the case where if there is an already existing
version.info file and we cannot get the git rev-parse output, we use
the old file instead. Blank version info is written only when we don't
have an old version.info file and we cannot do a git rev-parse.

Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
---
M bin/gen_build_version.py
M bin/save-version.sh
2 files changed, 23 insertions(+), 10 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
......................................................................


Patch Set 1:

(3 comments)

Thanks for fixing this.

http://gerrit.cloudera.org:8080/#/c/4411/1/bin/gen_build_version.py
File bin/gen_build_version.py:

PS1, Line 34: can_obtain_git_hash = False if os.system('git rev-parse HEAD') else True
This took me a second glance to understand, maybe replace with

can_obtain_git_hash = os.system(...) == 0 ?

Also, discard stdout and stderr of the git command?


PS1, Line 39: if not version_file_exists or can_obtain_git_hash:
what happens if we cannot obtain the git hash and don't have a version file? Maybe explain here that it is intended to call SAVE_VERSION_SCRIPT anyways in this case. You could also reverse the if and else clauses and make the if condition's structure the comment. Maybe that would make it easier to understand.


http://gerrit.cloudera.org:8080/#/c/4411/1/bin/save-version.sh
File bin/save-version.sh:

Line 25: GIT_HASH=$(git rev-parse HEAD)
Discard stderr?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
......................................................................


Patch Set 8:

> (1 comment)

Sorry about that.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4411/3/bin/gen_build_version.py
File bin/gen_build_version.py:

Line 35: # Redirecting stdout and stderr to os.devnull
> nit: You could remove both parameters altogether, since None is the default
Didn't realize this earlier, but None actually doesn't redirect stdout or stderr. So it does end up getting printed on stdout/stderr. I've added a change to redirect to os.devnull.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
......................................................................


Patch Set 4: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4411/3/bin/gen_build_version.py
File bin/gen_build_version.py:

Line 35: # Redirecting stdout and stderr to os.devnull
> Didn't realize this earlier, but None actually doesn't redirect stdout or s
Uh, I didn't know either. Nice way to redirect the output.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has uploaded a new patch set (#4).

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
......................................................................

Make gen_build_version.py resilient to a failing git rev-parse

It was noticed that some build processes did not checkout Impala and
instead built it from a tarball. This would cause our gen_build_version
script to write a blank version info everytime to the version.info file.

This patch takes care of the case where if there is an already existing
version.info file and we cannot get the git rev-parse output, we use
the old file instead. Blank version info is written only when we don't
have an old version.info file and we cannot do a git rev-parse.

Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
---
M bin/gen_build_version.py
M bin/save-version.sh
2 files changed, 30 insertions(+), 16 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/4411/4
-- 
To view, visit http://gerrit.cloudera.org:8080/4411
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has posted comments on this change.

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
......................................................................


Patch Set 6: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
......................................................................


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4411/2/bin/gen_build_version.py
File bin/gen_build_version.py:

Line 35: can_obtain_git_hash = call(['git', 'rev-parse', 'HEAD'], stdout=PIPE, stderr=PIPE) == 0
> Python docs discourage using PIPE here as it can block, depending on the ou
Done


Line 43:   # SAVE_VERSION_SCRIPT will generate a dummy version.info file if we cannot obtain the
> Maybe replace L54 with this?
Done


http://gerrit.cloudera.org:8080/#/c/4411/2/bin/save-version.sh
File bin/save-version.sh:

Line 25: GIT_HASH=$(git rev-parse HEAD) > /dev/null
> I think this should be 2> /dev/null, no?
Yup, thanks.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
......................................................................


Patch Set 5: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
......................................................................


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4411/1/bin/gen_build_version.py
File bin/gen_build_version.py:

PS1, Line 34: can_obtain_git_hash = False if os.system('git rev-parse HEAD') else True
> This took me a second glance to understand, maybe replace with
Done


PS1, Line 39: if not version_file_exists or can_obtain_git_hash:
> what happens if we cannot obtain the git hash and don't have a version file
Done


http://gerrit.cloudera.org:8080/#/c/4411/1/bin/save-version.sh
File bin/save-version.sh:

Line 25: GIT_HASH=$(git rev-parse HEAD)
> Discard stderr?
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
......................................................................


Patch Set 6: Code-Review+2

Thanks Alex.

Carry +2.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
Michael Brown has posted comments on this change.

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
......................................................................


Patch Set 4:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4411/4/bin/gen_build_version.py
File bin/gen_build_version.py:

PS4, Line 36: with open(os.devnull, 'w') as devnull:
Does this Python script need to support ancient Python 2.4? If yes, with won't be supported and this will fail.


http://gerrit.cloudera.org:8080/#/c/4411/4/bin/save-version.sh
File bin/save-version.sh:

PS4, Line 25: GIT_HASH=$(git rev-parse HEAD) 2> /dev/null
"2> /dev/null" should go inside the parens with the command.


PS4, Line 28:   GIT_HASH="Could not obtain git hash"
Has this path been tested? Does Impala work when the derived git hash doesn't match the typical pattern of a git hash? It might be safer to echo a warning here and make GIT_HASH empty. We know an empty GIT_HASH already won't cause Impala to crash or exit. If the point here is "best effort", this seems safer.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Michael Brown, Lars Volker, Internal Jenkins, Alex Behm,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/4411

to look at the new patch set (#7).

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
......................................................................

Make gen_build_version.py resilient to a failing git rev-parse

It was noticed that some build processes did not checkout Impala and
instead built it from a tarball. This would cause our gen_build_version
script to write a blank version info everytime to the version.info file.

This patch takes care of the case where if there is an already existing
version.info file and we cannot get the git rev-parse output, we use
the old file instead. Blank version info is written only when we don't
have an old version.info file and we cannot do a git rev-parse.

Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
---
M bin/gen_build_version.py
M bin/save-version.sh
2 files changed, 35 insertions(+), 17 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/4411/7
-- 
To view, visit http://gerrit.cloudera.org:8080/4411
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has uploaded a new patch set (#2).

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
......................................................................

Make gen_build_version.py resilient to a failing git rev-parse

It was noticed that some build processes did not checkout Impala and
instead built it from a tarball. This would cause our gen_build_version
script to write a blank version info everytime to the version.info file.

This patch takes care of the case where if there is an already existing
version.info file and we cannot get the git rev-parse output, we use
the old file instead. Blank version info is written only when we don't
have an old version.info file and we cannot do a git rev-parse.

Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
---
M bin/gen_build_version.py
M bin/save-version.sh
2 files changed, 27 insertions(+), 16 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/4411/2
-- 
To view, visit http://gerrit.cloudera.org:8080/4411
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has submitted this change and it was merged.

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
......................................................................


Make gen_build_version.py resilient to a failing git rev-parse

It was noticed that some build processes did not checkout Impala and
instead built it from a tarball. This would cause our gen_build_version
script to write a blank version info everytime to the version.info file.

This patch takes care of the case where if there is an already existing
version.info file and we cannot get the git rev-parse output, we use
the old file instead. Blank version info is written only when we don't
have an old version.info file and we cannot do a git rev-parse.

Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Reviewed-on: http://gerrit.cloudera.org:8080/4411
Reviewed-by: Sailesh Mukil <sa...@cloudera.com>
Tested-by: Sailesh Mukil <sa...@cloudera.com>
---
M bin/gen_build_version.py
M bin/save-version.sh
2 files changed, 35 insertions(+), 17 deletions(-)

Approvals:
  Sailesh Mukil: Looks good to me, approved; Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has uploaded a new patch set (#3).

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
......................................................................

Make gen_build_version.py resilient to a failing git rev-parse

It was noticed that some build processes did not checkout Impala and
instead built it from a tarball. This would cause our gen_build_version
script to write a blank version info everytime to the version.info file.

This patch takes care of the case where if there is an already existing
version.info file and we cannot get the git rev-parse output, we use
the old file instead. Blank version info is written only when we don't
have an old version.info file and we cannot do a git rev-parse.

Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
---
M bin/gen_build_version.py
M bin/save-version.sh
2 files changed, 27 insertions(+), 16 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/4411/3
-- 
To view, visit http://gerrit.cloudera.org:8080/4411
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
......................................................................


Patch Set 7: Code-Review+2 Verified+1

There was a path conflict during submission. I guess it didn't rebase properly. However, there are no build issues and it gave a verified +1. So carrying CR +2 and Verified +1.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
......................................................................


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4411/8/bin/save-version.sh
File bin/save-version.sh:

Line 24: VERSION=2.8.0-cdh5-INTERNAL
Was this the conflict? I think maybe it should be rolled back to 2.7.0.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
Michael Brown has posted comments on this change.

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
......................................................................


Patch Set 5: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4411/3/bin/gen_build_version.py
File bin/gen_build_version.py:

Line 35: can_obtain_git_hash = call(['git', 'rev-parse', 'HEAD'], stdout=None, stderr=None) == 0
nit: You could remove both parameters altogether, since None is the default for both.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/4411

to look at the new patch set (#5).

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
......................................................................

Make gen_build_version.py resilient to a failing git rev-parse

It was noticed that some build processes did not checkout Impala and
instead built it from a tarball. This would cause our gen_build_version
script to write a blank version info everytime to the version.info file.

This patch takes care of the case where if there is an already existing
version.info file and we cannot get the git rev-parse output, we use
the old file instead. Blank version info is written only when we don't
have an old version.info file and we cannot do a git rev-parse.

Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
---
M bin/gen_build_version.py
M bin/save-version.sh
2 files changed, 34 insertions(+), 16 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/4411/5
-- 
To view, visit http://gerrit.cloudera.org:8080/4411
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
......................................................................


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4411/2/bin/gen_build_version.py
File bin/gen_build_version.py:

Line 35: can_obtain_git_hash = call(['git', 'rev-parse', 'HEAD'], stdout=PIPE, stderr=PIPE) == 0
Python docs discourage using PIPE here as it can block, depending on the output volume of the subprocess (https://docs.python.org/2/library/subprocess.html). However, in your case you're not reading the output, so you could just use None for both instead, which is also the default.


Line 43:   # SAVE_VERSION_SCRIPT will generate a dummy version.info file if we cannot obtain the
Maybe replace L54 with this?


http://gerrit.cloudera.org:8080/#/c/4411/2/bin/save-version.sh
File bin/save-version.sh:

Line 25: GIT_HASH=$(git rev-parse HEAD) > /dev/null
I think this should be 2> /dev/null, no?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
......................................................................


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4411/8/bin/save-version.sh
File bin/save-version.sh:

Line 24: VERSION=2.8.0-cdh5-INTERNAL
> Was this the conflict? I think maybe it should be rolled back to 2.7.0.
I agree with Jim. Here's the change that fixes this: http://gerrit.cloudera.org:8080/4439


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] Make gen build version.py resilient to a failing git rev-parse

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.

Change subject: Make gen_build_version.py resilient to a failing git rev-parse
......................................................................


Patch Set 4:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4411/4/bin/gen_build_version.py
File bin/gen_build_version.py:

PS4, Line 36: with open(os.devnull, 'w') as devnull:
> Does this Python script need to support ancient Python 2.4? If yes, with wo
Good catch. Made it Python 2.4 friendly.


http://gerrit.cloudera.org:8080/#/c/4411/4/bin/save-version.sh
File bin/save-version.sh:

PS4, Line 25: GIT_HASH=$(git rev-parse HEAD) 2> /dev/null
> "2> /dev/null" should go inside the parens with the command.
Oops yes. Done.


PS4, Line 28:   GIT_HASH="Could not obtain git hash"
> Has this path been tested? Does Impala work when the derived git hash doesn
Yes, the git hash is only used in debug prints for now. See GetDaemonBuildHash(). So, this won't cause any crash.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id7af33502bbb70185dc15ffca6219436a616f25b
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes