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 <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
