You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@impala.apache.org by "Philip Zeyliger (Code Review)" <ge...@cloudera.org> on 2017/09/14 16:57:30 UTC

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Philip Zeyliger has uploaded a new change for review.

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

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................

IMPALA-5908: Allow SET to unset modified query options.

The query 'SET <option>=""' will now unset option, reverting it to
its default state.

This change became necessary with "SET" started returning an
empty string for unset options which don't have a default. The test
infrastructure (impala_test_suite.py) resets options to what it
thinks is its defaults, and, when this broke, some ASAN builds
started to fail, presumably due to a timing issue with how
we re-use connections between tests.

Testing:
* Added a simple test that triggered this side-effect without this code.
Specifically, "impala-python infra/python/env/bin/py.test tests/metadata/test_set.py -s"
with the modified set.test triggers.
* Added cases to query-options-test to check behavior for both
defaulted and non-defaulted values.
* Ran an ASAN build where this was triggering previously.

Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
---
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M be/src/service/query-options.h
M testdata/workloads/functional-query/queries/QueryTest/set.test
M tests/common/impala_test_suite.py
5 files changed, 483 insertions(+), 425 deletions(-)

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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.

Hello Impala Public Jenkins, Bharath Vissapragada, Dan Hecht, Tim Armstrong,

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

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

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

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................

IMPALA-5908: Allow SET to unset modified query options.

The query 'SET <option>=""' will now unset an option within the session,
reverting it to its default state.

This change became necessary when "SET" started returning an empty
string for unset options which don't have a default. The test
infrastructure (impala_test_suite.py) resets options to what it thinks
is its defaults, and, when this broke, some ASAN builds started to fail,
presumably due to a timing issue with how we re-use connections between
tests.

Because "set key=''" is ambiguous between "set to the empty string" and
"unset", it's now impossible to set to the empty string, at the session
layer, an option that is configured at a previous layer. In practice,
this is just debug_action and request_pool. debug_action is essentially
an internal tool. For request_pool, this means that setting the default
request_pool via impalad command line is now a bad idea, but that
doesn't seem worse than before.

Testing:
* Added a simple test that triggered this side-effect without this code.
Specifically, "impala-python infra/python/env/bin/py.test tests/metadata/test_set.py -s"
with the modified set.test triggers.
* Added cases to query-options-test to check behavior for both
defaulted and non-defaulted values.
* Added a custom cluster test that checks that overlays are
working against
* Ran an ASAN build where this was triggering previously.

Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
---
M be/src/service/client-request-state.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaInternalService.thrift
M testdata/workloads/functional-query/queries/QueryTest/set.test
M tests/common/impala_test_suite.py
M tests/custom_cluster/test_admission_controller.py
A tests/custom_cluster/test_set_and_unset.py
M tests/hs2/hs2_test_suite.py
14 files changed, 228 insertions(+), 19 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/70/8070/8
--
To view, visit http://gerrit.cloudera.org:8080/8070
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

Dan Hecht has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 3: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8070 )

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 15:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-Change-Number: 8070
Gerrit-PatchSet: 15
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 04 Oct 2017 23:00:58 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.

Philip Zeyliger has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 5:

How embarrassing! New patch uploaded. The relevant test passes now.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.

Philip Zeyliger has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 5:

Following through on the layering (process options, pool defaults, session options, and request options), there are some complexities.

When I changed SetQueryOption(option, "") to reset the mask bit, a new test found that server process options weren't surviving a reset cycle. What's going on is that SessionState.default_query_options is initialized to ImpalaServer.default_query_options, and that resolution is never re-applied. So, when we "reset", we don't handily have a reference to what to reset it to.

I'm planning to rename session->default_query_options to session->session_query_options, and resolve against the server defaults every time.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

Dan Hecht has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................

Patch Set 9:

(9 comments)

If you haven't already, please file a doc jira to update the docs based on this new behavior.

http://gerrit.cloudera.org:8080/#/c/8070/9//COMMIT_MSG
Commit Message:

PS9, Line 29: For request_pool, this means that setting the default
: request_pool via impalad command line is now a bad idea
could you check with MJ about whether people do that? I get the feeling that they might.

Line 31: doesn't seem worse than before.
what do you mean by "before"? what was the before behavior you are comparing with?

http://gerrit.cloudera.org:8080/#/c/8070/9/be/src/service/impala-server.h
File be/src/service/impala-server.h:

PS9, Line 346: server
let's be super explicit and say ImpalaServer's

PS9, Line 352: default_query_options
update

http://gerrit.cloudera.org:8080/#/c/8070/9/be/src/service/query-options.cc
File be/src/service/query-options.cc:

PS9, Line 61:
we had never set the dst isset bits? how did that not cause trouble before?

http://gerrit.cloudera.org:8080/#/c/8070/9/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:

PS9, Line 75: to override the defaults, which are stored in the
: // SessionState
maybe that could use some rewording (depending on what "defaults" is referring to)

PS9, Line 79: SET
: // <key>="".
unset will also affect options set by OpenSession() RPC, right? Let's clarify that.

And unset doesn't affect options set in (1) and (2), right? (it causes revert back to those "defaults").

http://gerrit.cloudera.org:8080/#/c/8070/9/tests/custom_cluster/test_admission_controller.py
File tests/custom_cluster/test_admission_controller.py:

Line 263: # abort on error, because it's back to being the default.
nit: line breaking seems early

http://gerrit.cloudera.org:8080/#/c/8070/9/tests/custom_cluster/test_set_and_unset.py
File tests/custom_cluster/test_set_and_unset.py:

PS9, Line 52: # "set" returns the session options; you have to run a real query to
: # see what would actually take effect.
i'm not following that comment given you had just used SET to see the query option setting. (The test case below makes sense to me though.)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/8070 )

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 12: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-Change-Number: 8070
Gerrit-PatchSet: 12
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 29 Sep 2017 22:06:15 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.

Philip Zeyliger has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 7:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8070/7/be/src/service/impala-server.h
File be/src/service/impala-server.h:

Line 346:     /// Reference to the server's query options
I think this is kosher because the server keeps references around to the session. I also considered making a copy for simplicity.


Line 392:     TQueryOptions query_options();
This is violating our style guide two-fold.

First of all, it should be QueryOptions(), which I'll change in the next iteration. Second of all, now we've got two functions in the stuct, and it should be a class. Is it worth it?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.

Philip Zeyliger has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 8:

I'm still re-running tests. The Overlay function wasn't setting __is_set when it should have been, so that broke some things. I'll report back when tests are passing.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.

Philip Zeyliger has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 3: Code-Review+1

Carry'ing Tim's +1.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8070 )

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 15: Code-Review+2

Let me retry


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-Change-Number: 8070
Gerrit-PatchSet: 15
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 04 Oct 2017 23:00:51 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.

Hello Impala Public Jenkins, Bharath Vissapragada, Dan Hecht, Tim Armstrong,

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

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

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

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................

IMPALA-5908: Allow SET to unset modified query options.

The query 'SET <option>=""' will now unset option, reverting it to
its default state.

Testing:
* Added a simple test that triggered this side-effect without this code.
Specifically, "impala-python infra/python/env/bin/py.test tests/metadata/test_set.py -s"
with the modified set.test triggers.
* Added cases to query-options-test to check behavior for both
defaulted and non-defaulted values.
* Ran an ASAN build where this was triggering previously.

Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
---
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaInternalService.thrift
M testdata/workloads/functional-query/queries/QueryTest/set.test
M tests/common/impala_test_suite.py
6 files changed, 67 insertions(+), 8 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/70/8070/6
--
To view, visit http://gerrit.cloudera.org:8080/8070
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8070 )

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 12:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-Change-Number: 8070
Gerrit-PatchSet: 12
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 29 Sep 2017 22:06:33 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.

Philip Zeyliger has uploaded a new patch set (#2).

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................

IMPALA-5908: Allow SET to unset modified query options.

The query 'SET <option>=""' will now unset option, reverting it to
its default state.

Testing:
* Added a simple test that triggered this side-effect without this code.
Specifically, "impala-python infra/python/env/bin/py.test tests/metadata/test_set.py -s"
with the modified set.test triggers.
* Added cases to query-options-test to check behavior for both
defaulted and non-defaulted values.
* Ran an ASAN build where this was triggering previously.

Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
---
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaInternalService.thrift
M testdata/workloads/functional-query/queries/QueryTest/set.test
M tests/common/impala_test_suite.py
6 files changed, 63 insertions(+), 4 deletions(-)

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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

Dan Hecht has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8070/1/be/src/service/query-options.cc
File be/src/service/query-options.cc:

Line 113: static int GetQueryOptionForKey(const string& key) {
> Nice catch. I don't feel strongly either about this (static makes more sens
'static' seems fine, and is already used in this file.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/8070 )

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8070/9//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8070/9//COMMIT_MSG@29
PS9, Line 29: For request_pool, this means that setting the default
            : request_pool via impalad command line is now a bad idea
> To do what? To have a default pool? That should rely on using the 'default'
okay



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-Change-Number: 8070
Gerrit-PatchSet: 9
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Sep 2017 20:56:16 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.

Philip Zeyliger has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 4:

I'm not an expert at reading these yet, but I think it's complaining about the following. This doesn't smell like something my change could have caused.

Am I misreading the logs?

https://jenkins.impala.io/job/ubuntu-16.04-from-scratch/281/console
20:43:45 [WS-CLEANUP] Deleting project workspace...Cannot delete workspace :remote file operation failed: /home/ubuntu at hudson.remoting.Channel@44f2e578:ubuntu-16.04 (i-04402f53583d90ca1): java.io.IOException: Unable to delete '/home/ubuntu'. Tried 3 times (of a maximum of 3) waiting 0.1 sec between attempts.
20:43:53 ERROR: Step ‘Delete workspace when build is done’ failed: Cannot delete workspace: remote file operation failed: /home/ubuntu at hudson.remoting.Channel@44f2e578:ubuntu-16.04 (i-04402f53583d90ca1): java.io.IOException: Unable to delete '/home/ubuntu'. Tried 3 times (of a maximum of 3) waiting 0.1 sec between attempts.
20:43:53 Finished: FAILURE

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

Dan Hecht has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8070/9//COMMIT_MSG
Commit Message:

PS9, Line 29: For request_pool, this means that setting the default
            : request_pool via impalad command line is now a bad idea
> I don't have data to back this up, but I'd guess that some folks are probab
Okay. What is the right way for the user to do this (so that we can document that as the "workaround")?  Or do we think that this setup is not useful?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.

Hello Impala Public Jenkins, Bharath Vissapragada, Dan Hecht, Tim Armstrong,

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

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

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

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................

IMPALA-5908: Allow SET to unset modified query options.

The query 'SET <option>=""' will now unset an option within the session,
reverting it to its default state.

Testing:
* Added a simple test that triggered this side-effect without this code.
Specifically, "impala-python infra/python/env/bin/py.test tests/metadata/test_set.py -s"
with the modified set.test triggers.
* Added cases to query-options-test to check behavior for both
defaulted and non-defaulted values.
* Added a custom cluster test that checks that overlays are
working against
* Ran an ASAN build where this was triggering previously.

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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.

Hello Impala Public Jenkins, Bharath Vissapragada, Dan Hecht, Tim Armstrong,

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

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

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

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................

IMPALA-5908: Allow SET to unset modified query options.

The query 'SET <option>=""' will now unset an option within the session,
reverting it to its default state.

Testing:
* Added a simple test that triggered this side-effect without this code.
Specifically, "impala-python infra/python/env/bin/py.test tests/metadata/test_set.py -s"
with the modified set.test triggers.
* Added cases to query-options-test to check behavior for both
defaulted and non-defaulted values.
* Added a custom cluster test that checks that overlays are
working against
* Ran an ASAN build where this was triggering previously.

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/70/8070/9
--
To view, visit http://gerrit.cloudera.org:8080/8070
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.

Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/8070 )

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................

Patch Set 10:

(8 comments)

I filed https://issues.apache.org/jira/browse/IMPALA-5981 for the documentation.

http://gerrit.cloudera.org:8080/#/c/8070/9//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8070/9//COMMIT_MSG@29
PS9, Line 29: hat is configured at a previous layer. In practice,
: this is just debug_action and request_pool. debug_actio
> okay
I did some analysis on cluster configurations in the wild, and very few clusters had request_pool set in default_query_options.

http://gerrit.cloudera.org:8080/#/c/8070/9//COMMIT_MSG@31
PS9, Line 31: an internal tool. For request_pool, this means that setting the default
> what do you mean by "before"? what was the before behavior you are comparin
I removed this; too ambiguous. I was trying to refer to the behavior of SET before and after this change.

http://gerrit.cloudera.org:8080/#/c/8070/9/be/src/service/impala-server.h
File be/src/service/impala-server.h:

http://gerrit.cloudera.org:8080/#/c/8070/9/be/src/service/impala-server.h@346
PS9, Line 346: Impala
> let's be super explicit and say ImpalaServer's
Done

http://gerrit.cloudera.org:8080/#/c/8070/9/be/src/service/impala-server.h@352
PS9, Line 352: set_query_options hav
> update
Done

http://gerrit.cloudera.org:8080/#/c/8070/9/be/src/service/query-options.cc
File be/src/service/query-options.cc:

http://gerrit.cloudera.org:8080/#/c/8070/9/be/src/service/query-options.cc@a61
PS9, Line 61:
> we had never set the dst isset bits? how did that not cause trouble before?
By accident. Many settings have isset true at creation time, so they would have worked to begin with. The ones that don't are below. I suspect that buffer pool settings were not overriding the following configs. Of these, request_pool has somewhat special handling (it's set in scheduler.cc). v_cpu_cores is not currently used, nor is reservation_request_timeout.

compression_codec(false)
request_pool(false)
v_cpu_cores(false)
reservation_request_timeout(false)
buffer_pool_limit(false)
seq_compression_mode(false)
mt_dop(false)

http://gerrit.cloudera.org:8080/#/c/8070/9/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:

http://gerrit.cloudera.org:8080/#/c/8070/9/common/thrift/ImpalaInternalService.thrift@75
PS9, Line 75: to override the preceding layers; these
: // overrides ar
> maybe that could use some rewording (depending on what "defaults" is referr
Done.

http://gerrit.cloudera.org:8080/#/c/8070/9/common/thrift/ImpalaInternalService.thrift@79
PS9, Line 79: >=<value>
: // or in the
> unset will also affect options set by OpenSession() RPC, right? Let's clar
I tried to clarify this. I'm still not happy with it, but see what you think.

Writing this, I'm further convinced that implementing UNSET foo; (vs SET FOO="") is unhelpful: the ambiguity will just come around in other places because we parse strings for these values.

http://gerrit.cloudera.org:8080/#/c/8070/9/tests/custom_cluster/test_set_and_unset.py
File tests/custom_cluster/test_set_and_unset.py:

http://gerrit.cloudera.org:8080/#/c/8070/9/tests/custom_cluster/test_set_and_unset.py@52
PS9, Line 52: # Use a "request overlay" to change the option for a specific
: # request within a session. We run a r
> i'm not following that comment given you had just used SET to see the query
I updated the comment. And added another test case. Which failed, so I updated some more code... This time, setting the per-request conf overlay to "" would cause the option to be reset to its default, and it makes more logical sense for that to be a no-op and the session setting to take hold.

I'm still pondering the fragility here and don't have any simple solutions. Clearest would be to carry all four layers of state around, and merge them as late as possible. You'd still need the mask type because Thrift's "isset" doesn't capture everything.

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-Change-Number: 8070
Gerrit-PatchSet: 10
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 26 Sep 2017 04:39:08 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.

Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 2: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8070/1//COMMIT_MSG
Commit Message:

Line 9: The query 'SET <option>=""' will now unset option, reverting it to
> The new behaviour is more consistent with us reporting the default value of
Fair point. Thanks for explaining.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.

Hello Bharath Vissapragada, Matthew Jacobs, Tim Armstrong, Dan Hecht, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................

IMPALA-5908: Allow SET to unset modified query options.

The query 'SET <option>=""' will now unset an option within the session,
reverting it to its default state.

Previously, SessionState copied over the default options from the server
when the session was created and then mutated that. To support unsetting
options at the session layer, this change keeps a pointer to the default
server settings, keeps separately the mutations, and overlays the
options each time they're requested. Similarly, for configuration
overlays that happen per-query, the overlay is now done explicitly,
because empty per-query overlay values (key=..., value="") now have no effect.

Testing:
* Added a simple test that triggered this side-effect without this code.
Specifically, "impala-python infra/python/env/bin/py.test tests/metadata/test_set.py -s"
with the modified set.test triggers.
* Amended tests/custom_cluster/test_admission_controller.py; it was
useful for testing these code paths.
* Added cases to query-options-test to check behavior for both
defaulted and non-defaulted values.
* Added a custom cluster test that checks that overlays are
working against
* Ran an ASAN build where this was triggering previously.

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-Change-Number: 8070
Gerrit-PatchSet: 11
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8070/1/be/src/service/query-options.cc
File be/src/service/query-options.cc:

Line 113: static int GetQueryOptionForKey(const string& key) {
> 'static' seems fine, and is already used in this file.
Yeah I agree. Some people seem to prefer the alternative even though it's equivalent.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

Dan Hecht has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 4: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

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

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................

IMPALA-5908: Allow SET to unset modified query options.

The query 'SET <option>=""' will now unset an option within the session,
reverting it to its default state.

Testing:
* Added a simple test that triggered this side-effect without this code.
Specifically, "impala-python infra/python/env/bin/py.test tests/metadata/test_set.py -s"
with the modified set.test triggers.
* Amended tests/custom_cluster/test_admission_controller.py; it was
useful for testing these code paths.
* Added cases to query-options-test to check behavior for both
defaulted and non-defaulted values.
* Added a custom cluster test that checks that overlays are
working against
* Ran an ASAN build where this was triggering previously.

Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Reviewed-on: http://gerrit.cloudera.org:8080/8070
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/service/client-request-state.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaInternalService.thrift
M testdata/workloads/functional-query/queries/QueryTest/set.test
M tests/common/impala_test_suite.py
M tests/custom_cluster/test_admission_controller.py
A tests/custom_cluster/test_set_and_unset.py
M tests/hs2/hs2_test_suite.py
14 files changed, 266 insertions(+), 26 deletions(-)

Approvals:
Tim Armstrong: Looks good to me, approved
Impala Public Jenkins: Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-Change-Number: 8070
Gerrit-PatchSet: 16
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 4:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.

Philip Zeyliger has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................

Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/8070/1//COMMIT_MSG
Commit Message:

Line 9: The query 'SET <option>=""' will now unset option, reverting it to
> A quick question, did you consider other alternatives like "UNSET <option>"
I did, and discussed it somewhat in the JIRA.

Our shell has 'unset' functionality today, but the server-side state doesn't. I think on balance having one command is going to have less complexity.

Interestingly, the confusion with compression_codec was what spurred the issue that caused this. In IMPALA-5589, it looked like resetting the setting to its default was NONE, which wasn't the right default value.

(This is all a cautionary tale about treating "unset" differently in Thrift...)

http://gerrit.cloudera.org:8080/#/c/8070/1/be/src/service/query-options.cc
File be/src/service/query-options.cc:

Line 113: static int GetQueryOptionForKey(const string& key) {
> Let's default to using 'static' -- it's concise. It's also been un-deprecia
https://google.github.io/styleguide/cppguide.html#Unnamed_Namespaces_and_Static_Variables agrees that 'static' and anonymous namespaces are both cool. I've left this alone.

Line 133: if (value == "") {
> Nit: Maybe make this an "else if" to avoid the extra indentation. This woul
Done

Line 137: case TImpalaQueryOptions::ABORT_ON_ERROR:
> I did a pass over the query options to check that the new behaviour made se
I agree. I'm adding a note to common/thrift/ImpalaInternalService.thrift to try to explain a caveat.

I looked over the documentation and I didn't see any information about UNSET outside of impala-shell.

I filed https://issues.apache.org/jira/browse/IMPALA-5937 to add documentation for a few query options that are missing it.

Line 196: if (value.empty()) break;
> This is dead now I think.
Done

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 4: Verified-1

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8070 )

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 14: Verified-1

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-Change-Number: 8070
Gerrit-PatchSet: 14
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 04 Oct 2017 19:44:26 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.

Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/8070 )

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 10:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/8070/10/be/src/service/impala-server.h
File be/src/service/impala-server.h:

http://gerrit.cloudera.org:8080/#/c/8070/10/be/src/service/impala-server.h@347
PS10, Line 347:     TQueryOptions *server_default_query_options;
> nit: placement of *
Done


http://gerrit.cloudera.org:8080/#/c/8070/7/be/src/service/query-options.cc
File be/src/service/query-options.cc:

http://gerrit.cloudera.org:8080/#/c/8070/7/be/src/service/query-options.cc@134
PS7, Line 134: NULL
> nullptr (we still have NULL all over the codebase but we're gradually switc
Done


http://gerrit.cloudera.org:8080/#/c/8070/10/be/src/service/query-options.cc
File be/src/service/query-options.cc:

http://gerrit.cloudera.org:8080/#/c/8070/10/be/src/service/query-options.cc@a61
PS10, Line 61: 
> Ahh we've seen this mistake a few times before.
Ack


http://gerrit.cloudera.org:8080/#/c/8070/10/be/src/service/query-options.cc@83
PS10, Line 83:  
> nit: placement of *
Done


http://gerrit.cloudera.org:8080/#/c/8070/10/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:

http://gerrit.cloudera.org:8080/#/c/8070/10/common/thrift/ImpalaInternalService.thrift@86
PS10, Line 86: a
> a what?
done. "a user"


http://gerrit.cloudera.org:8080/#/c/8070/9/tests/custom_cluster/test_set_and_unset.py
File tests/custom_cluster/test_set_and_unset.py:

http://gerrit.cloudera.org:8080/#/c/8070/9/tests/custom_cluster/test_set_and_unset.py@52
PS9, Line 52:     # Use a "request overlay" to change the option for a specific
            :     # request within a session. We run a r
> What do you want to do about this?  I'm okay with submitting this patch as-
I think these semantics are plausible. It's sensible to have a command to return the current session options, which is what this does. I think we can leave it alone.


http://gerrit.cloudera.org:8080/#/c/8070/10/tests/custom_cluster/test_set_and_unset.py
File tests/custom_cluster/test_set_and_unset.py:

http://gerrit.cloudera.org:8080/#/c/8070/10/tests/custom_cluster/test_set_and_unset.py@25
PS10, Line 25: class TestSetAndUnset(CustomClusterTestSuite, HS2TestSuite):
> Thanks for adding this, clearly there was a gap in test coverage here.
Ack



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-Change-Number: 8070
Gerrit-PatchSet: 10
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 29 Sep 2017 20:03:53 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.

Philip Zeyliger has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 6: Code-Review-1

Dan pointed out an important point that unsetting might mean that we should *not* set the mask. I'm working out what that means in terms of implementation and adding a test. In the meanwhile, https://gerrit.cloudera.org/#/c/8087/ is the revert of the original change.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 4:

That last error looks like an infra issue but it does looks like there was a test failure earlier (I ctrl-fed for "failed"):

  20:43:48 ] [ RUN      ] QueryOptions.ResetToDefaultViaEmptyString
20:43:48 ] /home/ubuntu/Impala/be/src/service/query-options-test.cc:170: Failure
20:43:48 ] Value of: expectedMask
20:43:48 ]   Actual: 0000000000000000000010000000000000000000000000000000000000000000
20:43:48 ] Expected: mask
20:43:48 ] Which is: 0000000000000000000000000000000000000000000000000000000000000000
20:43:48 ] [  FAILED  ] QueryOptions.ResetToDefaultViaEmptyString (0 ms)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8070 )

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 12: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-Change-Number: 8070
Gerrit-PatchSet: 12
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 30 Sep 2017 02:08:32 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 1:

(4 comments)

Makes sense to me. Had some minor comments but I'd also like someone else to confirm that the new behaviour makes sense.

http://gerrit.cloudera.org:8080/#/c/8070/1/be/src/service/query-options.cc
File be/src/service/query-options.cc:

Line 113: static int GetQueryOptionForKey(const string& key) {
Nice catch. I don't feel strongly either about this (static makes more sense to me) but putting functions in an anonymous namespace is the C++-ish way to prevent the symbols from being exported. The codebase hasn't really converged to standardise on one or the other.


Line 133:     if (value == "") {
Nit: Maybe make this an "else if" to avoid the extra indentation. This would also make the diff look less scary :).


Line 137:         case TImpalaQueryOptions::ABORT_ON_ERROR:
I did a pass over the query options to check that the new behaviour made sense. I think in almost every case either the behaviour is unchanged or the option did not accept "" as input.

We were a bit lucky there in the the default value of many options is 0 and atoi() returns 0 for invalid input like an empty string.

The behaviour changes I found were:
* compression_codec, which used to just ignore the "set" command if it was an empty string, which seems wrong
*  exec_single_node_rows_threshold, which defaults to 100 but interprets "" as 0.
* parquet_dictionary_filtering and parquet_read_statistics, which default to true but interpret "" as false

I don't think any of the above behaviour is documented or makes much sense so it seems fine to change it. Would be good to have someone confirm this reasoning though.


Line 196:           if (value.empty()) break;
This is dead now I think.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

Dan Hecht has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8070/1/be/src/service/query-options.cc
File be/src/service/query-options.cc:

Line 113: static int GetQueryOptionForKey(const string& key) {
> Yeah I agree. Some people seem to prefer the alternative even though it's e
Let's default to using 'static' -- it's concise. It's also been un-depreciated from C++.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.

Hello Bharath Vissapragada, Tim Armstrong,

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

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

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

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................

IMPALA-5908: Allow SET to unset modified query options.

The query 'SET <option>=""' will now unset option, reverting it to
its default state.

This change became necessary with "SET" started returning an
empty string for unset options which don't have a default. The test
infrastructure (impala_test_suite.py) resets options to what it
thinks is its defaults, and, when this broke, some ASAN builds
started to fail, presumably due to a timing issue with how
we re-use connections between tests.

Testing:
* Added a simple test that triggered this side-effect without this code.
  Specifically, "impala-python infra/python/env/bin/py.test tests/metadata/test_set.py -s"
  with the modified set.test triggers.
* Added cases to query-options-test to check behavior for both
  defaulted and non-defaulted values.
* Ran an ASAN build where this was triggering previously.

Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
---
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaInternalService.thrift
M testdata/workloads/functional-query/queries/QueryTest/set.test
M tests/common/impala_test_suite.py
6 files changed, 63 insertions(+), 4 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.

Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8070/1//COMMIT_MSG
Commit Message:

Line 9: The query 'SET <option>=""' will now unset option, reverting it to
A quick question, did you consider other alternatives like "UNSET <option>" or "RESET <option>" before settling for this?  

Reason being, users might confuse this with NONE. For example,

SET COMPRESSION_CODEC="" -> COMPRESSION_CODEC=SNAPPY (default)
SET COMPRESSION_CODE=NONE-> COMPRESSION_CODEC=NONE


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8070 )

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 15: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-Change-Number: 8070
Gerrit-PatchSet: 15
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 05 Oct 2017 03:04:37 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.

Philip Zeyliger has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 7:

I'm still fighting through some test failures in this latest iteration.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/8070 )

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 10: Code-Review+1

(1 comment)

Tim, since you were a reviewer on the first go around, would you like to take a look at the fixed version of this change?

http://gerrit.cloudera.org:8080/#/c/8070/9/tests/custom_cluster/test_set_and_unset.py
File tests/custom_cluster/test_set_and_unset.py:

http://gerrit.cloudera.org:8080/#/c/8070/9/tests/custom_cluster/test_set_and_unset.py@52
PS9, Line 52:     # Use a "request overlay" to change the option for a specific
            :     # request within a session. We run a r
> I updated the comment. And added another test case. Which failed, so I upda
What do you want to do about this?  I'm okay with submitting this patch as-is, but if you think there's something that would be better, that's okay too.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-Change-Number: 8070
Gerrit-PatchSet: 10
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 29 Sep 2017 00:15:18 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8070/9//COMMIT_MSG
Commit Message:

PS9, Line 29: For request_pool, this means that setting the default
            : request_pool via impalad command line is now a bad idea
> could you check with MJ about whether people do that? I get the feeling tha
I don't have data to back this up, but I'd guess that some folks are probably doing it even though it isn't the right thing to do (they should use the "default" placement rule to map specifically into the default pool.)

The case that might break is when a user has:
a) placement rules set up like "1) specified, 2) anything else, e.g. 'default' or 'user'"
b) they set --default_query_options=request_pool=foo
c) then rely on manually setting the session query option request_pool="" to get the mapping defined by the 2nd placement rule.

That said, I think this is a bad practice so I wouldn't be opposed to fixing this as long as we clearly release note it, and perhaps issue a warning if starting up the impala server with default_query_options=request_pool=foo


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.

Philip Zeyliger has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8070/2/be/src/service/query-options.h
File be/src/service/query-options.h:

PS2, Line 127: unset
> "reset the key to its default value" I think is more accurate
Done


http://gerrit.cloudera.org:8080/#/c/8070/2/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:

PS2, Line 80: stirng
> Thanks, this is helpful
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 2: Code-Review+1

(3 comments)

Wrote these up earlier but didn't push them out.

http://gerrit.cloudera.org:8080/#/c/8070/1//COMMIT_MSG
Commit Message:

Line 9: The query 'SET <option>=""' will now unset option, reverting it to
> I did, and discussed it somewhat in the JIRA.
The new behaviour is more consistent with us reporting the default value of compression_codec as "".

The current behaviour of running set compression_codec="" server-side is much weirder - it is just completely ignored. We don't document it as a valid value: https://impala.incubator.apache.org/docs/build/html/topics/impala_compression_codec.html so it seems unlikely that anyone is depending on it.


http://gerrit.cloudera.org:8080/#/c/8070/2/be/src/service/query-options.h
File be/src/service/query-options.h:

PS2, Line 127: unset
"reset the key to its default value" I think is more accurate


http://gerrit.cloudera.org:8080/#/c/8070/2/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:

PS2, Line 80: stirng
Thanks, this is helpful

nit: string


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8070 )

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 10: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-Change-Number: 8070
Gerrit-PatchSet: 10
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 29 Sep 2017 00:34:26 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.

Philip Zeyliger has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 9:

This passes tests, so is ready to be reviewed.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8070 )

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 14:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-Change-Number: 8070
Gerrit-PatchSet: 14
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 04 Oct 2017 17:09:19 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8070 )

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 7:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/8070/10/be/src/service/impala-server.h
File be/src/service/impala-server.h:

http://gerrit.cloudera.org:8080/#/c/8070/10/be/src/service/impala-server.h@347
PS10, Line 347:     TQueryOptions *server_default_query_options;
nit: placement of *


http://gerrit.cloudera.org:8080/#/c/8070/7/be/src/service/impala-server.h
File be/src/service/impala-server.h:

http://gerrit.cloudera.org:8080/#/c/8070/7/be/src/service/impala-server.h@306
PS7, Line 306:   struct SessionState {
This "struct' is pretty out of control. I agree with your earlier comment that this should be a class, particularly since it has non-trivial locking rules.

I don't think we need to do that cleanup now though.


http://gerrit.cloudera.org:8080/#/c/8070/7/be/src/service/query-options.cc
File be/src/service/query-options.cc:

http://gerrit.cloudera.org:8080/#/c/8070/7/be/src/service/query-options.cc@134
PS7, Line 134: NULL
nullptr (we still have NULL all over the codebase but we're gradually switching).


http://gerrit.cloudera.org:8080/#/c/8070/10/be/src/service/query-options.cc
File be/src/service/query-options.cc:

http://gerrit.cloudera.org:8080/#/c/8070/10/be/src/service/query-options.cc@a61
PS10, Line 61: 
Ahh we've seen this mistake a few times before.


http://gerrit.cloudera.org:8080/#/c/8070/10/be/src/service/query-options.cc@83
PS10, Line 83:  
nit: placement of *


http://gerrit.cloudera.org:8080/#/c/8070/10/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:

http://gerrit.cloudera.org:8080/#/c/8070/10/common/thrift/ImpalaInternalService.thrift@86
PS10, Line 86: 
a what?


http://gerrit.cloudera.org:8080/#/c/8070/10/tests/custom_cluster/test_set_and_unset.py
File tests/custom_cluster/test_set_and_unset.py:

http://gerrit.cloudera.org:8080/#/c/8070/10/tests/custom_cluster/test_set_and_unset.py@25
PS10, Line 25: class TestSetAndUnset(CustomClusterTestSuite, HS2TestSuite):
Thanks for adding this, clearly there was a gap in test coverage here.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-Change-Number: 8070
Gerrit-PatchSet: 7
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 29 Sep 2017 00:34:21 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.

Hello Impala Public Jenkins, Bharath Vissapragada, Dan Hecht, Tim Armstrong,

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

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

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

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................

IMPALA-5908: Allow SET to unset modified query options.

The query 'SET <option>=""' will now unset option, reverting it to
its default state.

This change became necessary with "SET" started returning an
empty string for unset options which don't have a default. The test
infrastructure (impala_test_suite.py) resets options to what it
thinks is its defaults, and, when this broke, some ASAN builds
started to fail, presumably due to a timing issue with how
we re-use connections between tests.

Testing:
* Added a simple test that triggered this side-effect without this code.
  Specifically, "impala-python infra/python/env/bin/py.test tests/metadata/test_set.py -s"
  with the modified set.test triggers.
* Added cases to query-options-test to check behavior for both
  defaulted and non-defaulted values.
* Ran an ASAN build where this was triggering previously.

Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
---
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M be/src/service/query-options.h
M bin/bootstrap_development.sh
M common/thrift/ImpalaInternalService.thrift
M testdata/workloads/functional-query/queries/QueryTest/set.test
M tests/common/impala_test_suite.py
7 files changed, 214 insertions(+), 130 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8070/9//COMMIT_MSG
Commit Message:

PS9, Line 29: For request_pool, this means that setting the default
            : request_pool via impalad command line is now a bad idea
> Okay. What is the right way for the user to do this (so that we can documen
To do what? To have a default pool? That should rely on using the 'default' rule.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.

Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/8070 )

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 14:

The build error seems to be:

> 19:44:25 ] [ERROR] /home/ubuntu/Impala/fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java:[593,11] no suitable method found for putString(java.lang.String)

This is https://issues.apache.org/jira/browse/IMPALA-6012. I believe Tim's pushed a workaround, so we could try again.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-Change-Number: 8070
Gerrit-PatchSet: 14
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 04 Oct 2017 22:59:35 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/8070 )

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 13:

Phil, please rebase this one now that the other has been committed, and I'll kick off the GVD.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-Change-Number: 8070
Gerrit-PatchSet: 13
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 03 Oct 2017 17:36:14 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/8070 )

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................


Patch Set 14: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-Change-Number: 8070
Gerrit-PatchSet: 14
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 04 Oct 2017 17:09:14 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.

Hello Bharath Vissapragada, Matthew Jacobs, Tim Armstrong, Dan Hecht, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-5908: Allow SET to unset modified query options.
......................................................................

IMPALA-5908: Allow SET to unset modified query options.

The query 'SET <option>=""' will now unset an option within the session,
reverting it to its default state.

Testing:
* Added a simple test that triggered this side-effect without this code.
Specifically, "impala-python infra/python/env/bin/py.test tests/metadata/test_set.py -s"
with the modified set.test triggers.
* Amended tests/custom_cluster/test_admission_controller.py; it was
useful for testing these code paths.
* Added cases to query-options-test to check behavior for both
defaulted and non-defaulted values.
* Added a custom cluster test that checks that overlays are
working against
* Ran an ASAN build where this was triggering previously.

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/70/8070/10
--
To view, visit http://gerrit.cloudera.org:8080/8070
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-Change-Number: 8070
Gerrit-PatchSet: 10
Gerrit-Owner: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@apache.org>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>