Fredy Wijaya has posted comments on this change. (
http://gerrit.cloudera.org:8080/12005 )
Change subject: IMPALA-6293: Unset JAVA_TOOL_OPTIONS variable from shell
commands
......................................................................
Patch Set 3:
> Patch Set 1:
>
> I'm vaguely uncomfortable with the non-genericness of the solution here. If
> we wanted to expose generic "unset" options, we could model that as a
> set<string>, and act accordingly. I think defaulting to clearing out
> JAVA_TOOL_OPTIONS is pretty sneaky. You also would need to think about shell
> escaping and so on (or verifying that these are actually variables.)
>
> In practice, maybe just change:
>
> RunShellProcess(FLAGS_s3a_access_key_cmd, &s3a_access_key_, true)
>
> to
>
> RunShellProcess("unset JAVA_TOOL_OPTIONS; " + FLAGS_s3a_access_key_cmd,
> &s3a_access_key_, true)
> (or use string::substitute or whatever)
>
> in the three or so places you need it?
I updated the code to make RunShellProcess more generic. I prefer putting
RunShellProcess so that I can easily unit test it. Furthermore, this makes
RunShellProcess somewhat similar (I know it's not quite the same) to Python's
Popen with env parameter.
--
To view, visit http://gerrit.cloudera.org:8080/12005
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I96a84a27d3bf7b02f04e70562acfd67386b93183
Gerrit-Change-Number: 12005
Gerrit-PatchSet: 3
Gerrit-Owner: Fredy Wijaya <fwij...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com>
Gerrit-Comment-Date: Wed, 28 Nov 2018 22:03:00 +0000
Gerrit-HasComments: No
