You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Kurt Deschler (Code Review)" <ge...@cloudera.org> on 2021/04/29 20:25:06 UTC
[Impala-ASF-CR] IMPALA-10684: Report invalid parameter setting errors during HS2 OpenSession
Kurt Deschler has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17363
Change subject: IMPALA-10684: Report invalid parameter setting errors during HS2 OpenSession
......................................................................
IMPALA-10684: Report invalid parameter setting errors during HS2 OpenSession
This patch reports invalid parameter settings to the client during
OpenSession. Previously, impalad logged a message and stacktrace but did
not report the error to the client.
Testing: Added negative test to tests/hs2/test_hs2.py
Change-Id: I19a859bf5226feb903017a66f199ca5da916f217
---
M be/src/common/status.h
M be/src/service/impala-hs2-server.cc
M be/src/service/query-options.cc
M tests/hs2/test_hs2.py
4 files changed, 16 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/63/17363/1
--
To view, visit http://gerrit.cloudera.org:8080/17363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I19a859bf5226feb903017a66f199ca5da916f217
Gerrit-Change-Number: 17363
Gerrit-PatchSet: 1
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
[Impala-ASF-CR] IMPALA-10684: Report invalid parameter setting errors during HS2 OpenSession
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/17363 )
Change subject: IMPALA-10684: Report invalid parameter setting errors during HS2 OpenSession
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17363/2/be/src/service/impala-hs2-server.cc
File be/src/service/impala-hs2-server.cc:
http://gerrit.cloudera.org:8080/#/c/17363/2/be/src/service/impala-hs2-server.cc@364
PS2, Line 364: HS2_RETURN_IF_ERROR(return_val, SetQueryOption(v.first, v.second,
: &state->set_query_options, &state->set_query_options_mask),
: SQLSTATE_GENERAL_ERROR);
: if (iequals(v.first, "idle_session_timeout")) {
Another thing to consider is whether you want to detect all the bad query options and merge the status into a single error message. We do that in ParseQueryOptions here:
https://github.com/apache/impala/blob/master/be/src/service/query-options.cc#L1098-L1121
--
To view, visit http://gerrit.cloudera.org:8080/17363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I19a859bf5226feb903017a66f199ca5da916f217
Gerrit-Change-Number: 17363
Gerrit-PatchSet: 2
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Comment-Date: Wed, 12 May 2021 23:36:59 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10684: Report invalid parameter setting errors during HS2 OpenSession
Posted by "Kurt Deschler (Code Review)" <ge...@cloudera.org>.
Kurt Deschler has posted comments on this change. ( http://gerrit.cloudera.org:8080/17363 )
Change subject: IMPALA-10684: Report invalid parameter setting errors during HS2 OpenSession
......................................................................
Patch Set 1:
(3 comments)
Currently with hs2, any parameters supplied during OpenSession that fail the parsing check are logged and discarded. This is bad because the user may have made a simple mistake such as quoting the parameter value. The user would be able to determine that the parameter was not set using the set command or looking for the error in the log. Further confusing matters, the set statement does not run this check values either - parameters can be set to invalid values and are not reported until a query is executed.
http://gerrit.cloudera.org:8080/#/c/17363/1/be/src/common/status.h
File be/src/common/status.h:
http://gerrit.cloudera.org:8080/#/c/17363/1/be/src/common/status.h@164
PS1, Line 164: // Same as Status(string) but can supress logging
: Status(const std::string& error_msg, bool silent);
> The "Expected" constructors are equivalent to the usual constructors with s
Ack
http://gerrit.cloudera.org:8080/#/c/17363/1/be/src/common/status.h@183
PS1, Line 183: static Status Expected(const std::string& error_msg);
> This is equivalent to silent=true.
Ack
http://gerrit.cloudera.org:8080/#/c/17363/1/be/src/service/impala-hs2-server.cc
File be/src/service/impala-hs2-server.cc:
http://gerrit.cloudera.org:8080/#/c/17363/1/be/src/service/impala-hs2-server.cc@364
PS1, Line 364: // Ignore failure (failures will be logged in SetQueryOption()).
> Update this comment.
Removed comment
--
To view, visit http://gerrit.cloudera.org:8080/17363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I19a859bf5226feb903017a66f199ca5da916f217
Gerrit-Change-Number: 17363
Gerrit-PatchSet: 1
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 May 2021 00:15:46 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10684: Report invalid parameter setting errors during HS2 OpenSession
Posted by "Kurt Deschler (Code Review)" <ge...@cloudera.org>.
Hello Aman Sinha, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17363
to look at the new patch set (#2).
Change subject: IMPALA-10684: Report invalid parameter setting errors during HS2 OpenSession
......................................................................
IMPALA-10684: Report invalid parameter setting errors during HS2 OpenSession
This patch reports invalid parameter settings to the client during
OpenSession. Previously, impalad logged a message and stacktrace but did
not report the error to the client.
Testing: Added negative test to tests/hs2/test_hs2.py
Change-Id: I19a859bf5226feb903017a66f199ca5da916f217
---
M be/src/service/impala-hs2-server.cc
M be/src/service/query-options.cc
M shell/impala_client.py
M tests/hs2/test_hs2.py
4 files changed, 14 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/63/17363/2
--
To view, visit http://gerrit.cloudera.org:8080/17363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I19a859bf5226feb903017a66f199ca5da916f217
Gerrit-Change-Number: 17363
Gerrit-PatchSet: 2
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
[Impala-ASF-CR] IMPALA-10684: Report invalid parameter setting errors during HS2 OpenSession
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17363 )
Change subject: IMPALA-10684: Report invalid parameter setting errors during HS2 OpenSession
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/8666/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/17363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I19a859bf5226feb903017a66f199ca5da916f217
Gerrit-Change-Number: 17363
Gerrit-PatchSet: 1
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Comment-Date: Thu, 29 Apr 2021 20:46:38 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10684: Report invalid parameter setting errors during HS2 OpenSession
Posted by "Kurt Deschler (Code Review)" <ge...@cloudera.org>.
Kurt Deschler has posted comments on this change. ( http://gerrit.cloudera.org:8080/17363 )
Change subject: IMPALA-10684: Report invalid parameter setting errors during HS2 OpenSession
......................................................................
Patch Set 2:
More on the mechanics.. In Beeswax server, set options are stored in beeswax::Query.configuration which is a list of strings that can contain anything. Beeswax options are parsed during impala::ImpalaServer::QueryToTQueryContext. With HS2, OpenSession parses the options directly into TQueryOptions so illegal values cannot be preserved.
--
To view, visit http://gerrit.cloudera.org:8080/17363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I19a859bf5226feb903017a66f199ca5da916f217
Gerrit-Change-Number: 17363
Gerrit-PatchSet: 2
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 May 2021 17:33:51 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10684: Report invalid parameter setting errors during HS2 OpenSession
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/17363 )
Change subject: IMPALA-10684: Report invalid parameter setting errors during HS2 OpenSession
......................................................................
Patch Set 1:
(3 comments)
Let me see if I understand this:
A session logs on with bad query options. Right now, nothing is returned to the client at OpenSession. If that session runs a query, what happens? Does it throw an error about the invalid query options? Or does it silently try to run with invalid query options?
I'm trying to decide whether it makes sense to enforce this at OpenSession or whether it would be better to wait until running a SQL. If existing users currently open a session with invalid parameters, then this would be a behavior change.
http://gerrit.cloudera.org:8080/#/c/17363/1/be/src/common/status.h
File be/src/common/status.h:
http://gerrit.cloudera.org:8080/#/c/17363/1/be/src/common/status.h@164
PS1, Line 164: // Same as Status(string) but can supress logging
: Status(const std::string& error_msg, bool silent);
The "Expected" constructors are equivalent to the usual constructors with silent=true. So, let's switch to using that and leaving this private.
http://gerrit.cloudera.org:8080/#/c/17363/1/be/src/common/status.h@183
PS1, Line 183: static Status Expected(const std::string& error_msg);
This is equivalent to silent=true.
http://gerrit.cloudera.org:8080/#/c/17363/1/be/src/service/impala-hs2-server.cc
File be/src/service/impala-hs2-server.cc:
http://gerrit.cloudera.org:8080/#/c/17363/1/be/src/service/impala-hs2-server.cc@364
PS1, Line 364: // Ignore failure (failures will be logged in SetQueryOption()).
Update this comment.
--
To view, visit http://gerrit.cloudera.org:8080/17363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I19a859bf5226feb903017a66f199ca5da916f217
Gerrit-Change-Number: 17363
Gerrit-PatchSet: 1
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Comment-Date: Mon, 10 May 2021 17:16:26 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10684: Report invalid parameter setting errors during HS2 OpenSession
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/17363 )
Change subject: IMPALA-10684: Report invalid parameter setting errors during HS2 OpenSession
......................................................................
Patch Set 2:
> (3 comments)
>
> Currently with hs2, any parameters supplied during OpenSession that
> fail the parsing check are logged and discarded. This is bad
> because the user may have made a simple mistake such as quoting the
> parameter value. The user would be able to determine that the
> parameter was not set using the set command or looking for the
> error in the log. Further confusing matters, the set statement does
> not run this check values either - parameters can be set to invalid
> values and are not reported until a query is executed.
I agree that silently dropping the invalid query options is a bad behavior.
My concern is that this has been the behavior for 5+ years, and I have to assume that some users may rely on it. That reliance could be embedded in code, and it is not always easy to modify immediately. Returning an error here could cause existing use cases to be unable to open a session.
One option is to use the SUCCESS_WITH_INFO status to return a message saying that some query options were ignored. Another option is we could have a startup parameter that enables the strict mode, though I don't know how we would stage enabling it.
--
To view, visit http://gerrit.cloudera.org:8080/17363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I19a859bf5226feb903017a66f199ca5da916f217
Gerrit-Change-Number: 17363
Gerrit-PatchSet: 2
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 May 2021 19:25:16 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10684: Report invalid parameter setting errors during HS2 OpenSession
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17363 )
Change subject: IMPALA-10684: Report invalid parameter setting errors during HS2 OpenSession
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/8697/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/17363
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I19a859bf5226feb903017a66f199ca5da916f217
Gerrit-Change-Number: 17363
Gerrit-PatchSet: 2
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 May 2021 00:29:42 +0000
Gerrit-HasComments: No