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 <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 26 Sep 2017 04:39:08 +0000 Gerrit-HasComments: Yes
