subject:"\[Impala\-ASF\-CR\] IMPALA\-2248\: Make idle session timeout a query option"

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2018-01-06 Thread Zoltan Borok-Nagy (Code Review)

Zoltan Borok-Nagy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8490 )

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..


Patch Set 22:

Thank you all for the comments!


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 22
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Sat, 06 Jan 2018 10:00:26 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2018-01-05 Thread Impala Public Jenkins (Code Review)

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

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..

IMPALA-2248: Make idle_session_timeout a query option

This commit makes idle_session_timeout a query option.

idle_session_timeout currently can be set as a command line
option, which will be the default timeout for sessions.
HS2 sessions can override it with a smaller value by setting
it in the configuration overlay of HS2 OpenSession().

However, we can't override idle_session_timeout for JDBC/ODBC
connections, because we cannot put this in the connection string.

This commit is a workaround for this problem, it allows JDBC/ODBC
connections to set the session timeout as a query option
with the SET statement.

After this commit, the session timeout can be overridden to
any value, i.e. the command line flag idle_session_timeout
doesn't limit this option anymore.

I created an automated test case in JdbcTest.java based on
test_hs2.py::test_concurrent_session_mixed_idle_timeout. I also
extended the test_session_expiration and test_set_and_unset
test suites.

Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Reviewed-on: http://gerrit.cloudera.org:8080/8490
Reviewed-by: Tim Armstrong 
Tested-by: Impala Public Jenkins
---
M be/src/service/client-request-state.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.cc
M be/src/service/query-options.h
M common/thrift/ImpalaInternalService.thrift
M common/thrift/ImpalaService.thrift
M fe/src/test/java/org/apache/impala/service/JdbcTest.java
A fe/src/test/java/org/apache/impala/util/Metrics.java
M tests/custom_cluster/test_session_expiration.py
M tests/custom_cluster/test_set_and_unset.py
M tests/hs2/hs2_test_suite.py
M tests/hs2/test_hs2.py
14 files changed, 391 insertions(+), 60 deletions(-)

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

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 22
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2018-01-05 Thread Impala Public Jenkins (Code Review)

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

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..


Patch Set 21: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 21
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Sat, 06 Jan 2018 01:47:46 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2018-01-05 Thread Tim Armstrong (Code Review)

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

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..


Patch Set 21: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 21
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Fri, 05 Jan 2018 21:55:33 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2018-01-05 Thread Impala Public Jenkins (Code Review)

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

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..


Patch Set 21:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 21
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Fri, 05 Jan 2018 21:56:09 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2018-01-05 Thread Tim Armstrong (Code Review)

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

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..


Patch Set 21:

This solution and the behaviour seem reasonable to me. It's a bit unfortunate 
that we have to special-case this but it seems unavoidable.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 21
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Fri, 05 Jan 2018 21:56:02 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2018-01-05 Thread Zoltan Borok-Nagy (Code Review)

Zoltan Borok-Nagy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8490 )

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..


Patch Set 21:

There are several ways to fix that test case.
I chose the one that involves the least code modifications.

Other way of fixing it would be to add an extra param to 
impala::DebugQueryOptions(). This param would contain the default query 
options. Callers could pass ImpalaServer::default_query_options. After that the 
result of DebugQueryOptions() would change, because 
ImpalaServer::default_query_options is not equal to the default constructed 
TQueryOptions. Therefore, we would also need to modify some test cases that 
check the result of DebugQueryOptions(), e.g. 
TestAdmissionController::test_set_request_pool.

Other fix could be to not set 
ImpalaServer::default_query_options.idle_session_timeout. With this solution 
idle_session_timeout would be more aligned with TQueryOptions::query_timeout_s 
/ FLAGS_idle_query_timeout, but the result of the SET command would not show 
the effective value of the session timeout when it's not set explicitly.

Please tell me if you prefer the other solutions, or if I'm missing some point.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 21
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Fri, 05 Jan 2018 15:27:04 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2018-01-05 Thread Zoltan Borok-Nagy (Code Review)

Hello Michael Ho, Thomas Tauber-Marshall, Laszlo Gaal, Gabor Kaszab, Attila 
Jeges, Tim Armstrong, Csaba Ringhofer, Dan Hecht, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..

IMPALA-2248: Make idle_session_timeout a query option

This commit makes idle_session_timeout a query option.

idle_session_timeout currently can be set as a command line
option, which will be the default timeout for sessions.
HS2 sessions can override it with a smaller value by setting
it in the configuration overlay of HS2 OpenSession().

However, we can't override idle_session_timeout for JDBC/ODBC
connections, because we cannot put this in the connection string.

This commit is a workaround for this problem, it allows JDBC/ODBC
connections to set the session timeout as a query option
with the SET statement.

After this commit, the session timeout can be overridden to
any value, i.e. the command line flag idle_session_timeout
doesn't limit this option anymore.

I created an automated test case in JdbcTest.java based on
test_hs2.py::test_concurrent_session_mixed_idle_timeout. I also
extended the test_session_expiration and test_set_and_unset
test suites.

Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
---
M be/src/service/client-request-state.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.cc
M be/src/service/query-options.h
M common/thrift/ImpalaInternalService.thrift
M common/thrift/ImpalaService.thrift
M fe/src/test/java/org/apache/impala/service/JdbcTest.java
A fe/src/test/java/org/apache/impala/util/Metrics.java
M tests/custom_cluster/test_session_expiration.py
M tests/custom_cluster/test_set_and_unset.py
M tests/hs2/hs2_test_suite.py
M tests/hs2/test_hs2.py
14 files changed, 391 insertions(+), 60 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/8490/20
--
To view, visit http://gerrit.cloudera.org:8080/8490
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 20
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2018-01-03 Thread Michael Ho (Code Review)

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8490 )

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..


Patch Set 19:

Build failed due to failed test:

 21:45:15 ] FAIL 
custom_cluster/test_admission_controller.py::TestAdmissionController::()::test_set_request_pool
21:45:15 ] === FAILURES 
===
21:45:15 ]  TestAdmissionController.test_set_request_pool 
_
21:45:15 ] hs2/hs2_test_suite.py:48: in add_session
21:45:15 ] fn(self)
21:45:15 ] custom_cluster/test_admission_controller.py:274: in 
test_set_request_pool
21:45:15 ] ['MEM_LIMIT=2', 'REQUEST_POOL=root.queueB'])
21:45:15 ] custom_cluster/test_admission_controller.py:204: in 
__check_query_options
21:45:15 ] assert len(confs) == len(expected_query_options)
21:45:15 ] E   assert 3 == 2
21:45:15 ] E+  where 3 = len(['MEM_LIMIT=2', 
'REQUEST_POOL=root.queueB', 'IDLE_SESSION_TIMEOUT=0'])
21:45:15 ] E+  and   2 = len(['MEM_LIMIT=2', 
'REQUEST_POOL=root.queueB'])
21:45:15 ]  Captured stdout setup 
-


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 19
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Wed, 03 Jan 2018 21:53:28 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2018-01-03 Thread Impala Public Jenkins (Code Review)

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

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..


Patch Set 19: Verified-1

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 19
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Wed, 03 Jan 2018 21:45:16 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2018-01-03 Thread Impala Public Jenkins (Code Review)

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

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..


Patch Set 19:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 19
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Wed, 03 Jan 2018 18:09:55 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2018-01-03 Thread Zoltan Borok-Nagy (Code Review)

Hello Michael Ho, Thomas Tauber-Marshall, Laszlo Gaal, Gabor Kaszab, Attila 
Jeges, Tim Armstrong, Csaba Ringhofer, Dan Hecht,

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

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

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

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..

IMPALA-2248: Make idle_session_timeout a query option

This commit makes idle_session_timeout a query option.

idle_session_timeout currently can be set as a command line
option, which will be the default timeout for sessions.
HS2 sessions can override it with a smaller value by setting
it in the configuration overlay of HS2 OpenSession().

However, we can't override idle_session_timeout for JDBC/ODBC
connections, because we cannot put this in the connection string.

This commit is a workaround for this problem, it allows JDBC/ODBC
connections to set the session timeout as a query option
with the SET statement.

After this commit, the session timeout can be overridden to
any value, i.e. the command line flag idle_session_timeout
doesn't limit this option anymore.

I created an automated test case in JdbcTest.java based on
test_hs2.py::test_concurrent_session_mixed_idle_timeout. I also
extended the test_session_expiration and test_set_and_unset
test suites.

Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
---
M be/src/service/client-request-state.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.cc
M be/src/service/query-options.h
M common/thrift/ImpalaInternalService.thrift
M common/thrift/ImpalaService.thrift
M fe/src/test/java/org/apache/impala/service/JdbcTest.java
A fe/src/test/java/org/apache/impala/util/Metrics.java
M tests/custom_cluster/test_session_expiration.py
M tests/custom_cluster/test_set_and_unset.py
M tests/hs2/hs2_test_suite.py
M tests/hs2/test_hs2.py
14 files changed, 381 insertions(+), 59 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/8490/19
--
To view, visit http://gerrit.cloudera.org:8080/8490
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 19
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2018-01-03 Thread Zoltan Borok-Nagy (Code Review)

Zoltan Borok-Nagy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8490 )

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..


Patch Set 18:

(3 comments)

Thanks!

http://gerrit.cloudera.org:8080/#/c/8490/17/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

http://gerrit.cloudera.org:8080/#/c/8490/17/be/src/service/impala-server.cc@1837
PS17, Line 1837: ile (true) {
> I suppose the reason why we can get rid of the notification here is that be
Right, that's the logic behind it.


http://gerrit.cloudera.org:8080/#/c/8490/18/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

http://gerrit.cloudera.org:8080/#/c/8490/18/be/src/service/impala-server.cc@1829
PS18, Line 1829:
> nit: indent off by 2
Oops, done.


http://gerrit.cloudera.org:8080/#/c/8490/18/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:

http://gerrit.cloudera.org:8080/#/c/8490/18/common/thrift/ImpalaInternalService.thrift@292
PS18, Line 292: are never expired.
> nit: never expire.
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 18
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Wed, 03 Jan 2018 10:20:14 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2018-01-02 Thread Michael Ho (Code Review)

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8490 )

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..


Patch Set 18: Code-Review+2

(4 comments)

http://gerrit.cloudera.org:8080/#/c/8490/17/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

http://gerrit.cloudera.org:8080/#/c/8490/17/be/src/service/impala-server.cc@1837
PS17, Line 1837: ile (true) {
I suppose the reason why we can get rid of the notification here is that 
because the thread should wake up once a second if session_timeout_set_ is not 
empty, right ?


http://gerrit.cloudera.org:8080/#/c/8490/18/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

http://gerrit.cloudera.org:8080/#/c/8490/18/be/src/service/impala-server.cc@1829
PS18, Line 1829:
nit: indent off by 2


http://gerrit.cloudera.org:8080/#/c/8490/18/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:

http://gerrit.cloudera.org:8080/#/c/8490/18/common/thrift/ImpalaInternalService.thrift@292
PS18, Line 292: are never expired.
nit: never expire.


http://gerrit.cloudera.org:8080/#/c/8490/17/fe/src/test/java/org/apache/impala/service/JdbcTest.java
File fe/src/test/java/org/apache/impala/service/JdbcTest.java:

http://gerrit.cloudera.org:8080/#/c/8490/17/fe/src/test/java/org/apache/impala/service/JdbcTest.java@629
PS17, Line 629:  + timeoutTolera
> I ported the e2e test "test_concurrent_session_mixed_idle_timeout" which us
Thanks for double checking and fixing other places !



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 18
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Wed, 03 Jan 2018 03:03:52 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2018-01-02 Thread Zoltan Borok-Nagy (Code Review)

Zoltan Borok-Nagy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8490 )

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..


Patch Set 17:

(11 comments)

Thanks for your comments.

It turned that the way Impala expires sessions has changed over the time, and 
some tests/comments were inconsistent with the new behavior.

See the details in my comments.

http://gerrit.cloudera.org:8080/#/c/8490/17/be/src/service/impala-hs2-server.cc
File be/src/service/impala-hs2-server.cc:

http://gerrit.cloudera.org:8080/#/c/8490/17/be/src/service/impala-hs2-server.cc@340
PS17, Line 340: state->UpdateTimeout();
> This line is a bit subtle. The session hasn't yet registered any timeout so
I chose to use the old way when we are creating a new session.


http://gerrit.cloudera.org:8080/#/c/8490/17/be/src/service/impala-hs2-server.cc@341
PS17, Line 341:   VLOG_QUERY << "OpenSession(): idle_session_timeout="
  :  << PrettyPrinter::Print(state->session_timeout, 
TUnit::TIME_S);
> Is it worth logging this every time a session is opened ? What you did in P
Done


http://gerrit.cloudera.org:8080/#/c/8490/17/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

http://gerrit.cloudera.org:8080/#/c/8490/17/be/src/service/impala-server.cc@1229
PS17, Line 1229:
> nit: blank space here seems unnecessary.
Done


http://gerrit.cloudera.org:8080/#/c/8490/17/be/src/service/impala-server.cc@1231
PS17, Line 1231:
> nit: blank space here seems unnecessary.
Done


http://gerrit.cloudera.org:8080/#/c/8490/17/be/src/service/impala-server.cc@1237
PS17, Line 1237:
> nit: blank space here seems unnecessary.
Done


http://gerrit.cloudera.org:8080/#/c/8490/17/be/src/service/impala-server.cc@1768
PS17, Line 1768: session_state->UpdateTimeout();
> IMHO, the old way of explicitly calling RegisterSessionTimeout() when a ses
OK, I can agree with that. Reverted it back to the old way for session starts.


http://gerrit.cloudera.org:8080/#/c/8490/17/common/thrift/ImpalaService.thrift
File common/thrift/ImpalaService.thrift:

http://gerrit.cloudera.org:8080/#/c/8490/17/common/thrift/ImpalaService.thrift@293
PS17, Line 293: are never expired
> nit: never expire.
Done


http://gerrit.cloudera.org:8080/#/c/8490/17/fe/src/test/java/org/apache/impala/service/JdbcTest.java
File fe/src/test/java/org/apache/impala/service/JdbcTest.java:

http://gerrit.cloudera.org:8080/#/c/8490/17/fe/src/test/java/org/apache/impala/service/JdbcTest.java@629
PS17, Line 629: sleepPeriod
> sleepPeriodMs.
Done


http://gerrit.cloudera.org:8080/#/c/8490/17/fe/src/test/java/org/apache/impala/service/JdbcTest.java@629
PS17, Line 629: timeoutTolerance
> So, the larger the timeout, the larger the tolerance is ?
I ported the e2e test "test_concurrent_session_mixed_idle_timeout" which uses a 
tolerance that is proportional to the timeout.

However, I looked at ImpalaServer::ExpireSessions(), and it turned out that the 
way we expire sessions have changed.

Before IMPALA-5108, ExpireSessions() checked the sessions in every (MIN(session 
timeouts) / 2) seconds. After IMPALA-5108, it checks the sessions in every 
second.

I updated the tests and comments with this in mind. Not just the ones that I 
introduced in this commit, but some existing tests/comments as well.


http://gerrit.cloudera.org:8080/#/c/8490/17/fe/src/test/java/org/apache/impala/service/JdbcTest.java@629
PS17, Line 629: + 500
> Just curious on reasoning for + 500 ?
Whenever my tests involve timeouts I'm afraid of making it flaky, so I tend to 
make the timeouts bigger than needed.

Please see my next comment.


http://gerrit.cloudera.org:8080/#/c/8490/17/fe/src/test/java/org/apache/impala/service/JdbcTest.java@637
PS17, Line 637: lastExecStatementTime
> "now" seems to be a more appropriate name esp. if you are comparing it agai
I agree, done.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 17
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Tue, 02 Jan 2018 17:25:22 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2018-01-02 Thread Zoltan Borok-Nagy (Code Review)

Hello Michael Ho, Thomas Tauber-Marshall, Laszlo Gaal, Gabor Kaszab, Attila 
Jeges, Tim Armstrong, Csaba Ringhofer, Dan Hecht,

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

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

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

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..

IMPALA-2248: Make idle_session_timeout a query option

This commit makes idle_session_timeout a query option.

idle_session_timeout currently can be set as a command line
option, which will be the default timeout for sessions.
HS2 sessions can override it with a smaller value by setting
it in the configuration overlay of HS2 OpenSession().

However, we can't override idle_session_timeout for JDBC/ODBC
connections, because we cannot put this in the connection string.

This commit is a workaround for this problem, it allows JDBC/ODBC
connections to set the session timeout as a query option
with the SET statement.

After this commit, the session timeout can be overridden to
any value, i.e. the command line flag idle_session_timeout
doesn't limit this option anymore.

I created an automated test case in JdbcTest.java based on
test_hs2.py::test_concurrent_session_mixed_idle_timeout. I also
extended the test_session_expiration and test_set_and_unset
test suites.

Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
---
M be/src/service/client-request-state.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.cc
M be/src/service/query-options.h
M common/thrift/ImpalaInternalService.thrift
M common/thrift/ImpalaService.thrift
M fe/src/test/java/org/apache/impala/service/JdbcTest.java
A fe/src/test/java/org/apache/impala/util/Metrics.java
M tests/custom_cluster/test_session_expiration.py
M tests/custom_cluster/test_set_and_unset.py
M tests/hs2/hs2_test_suite.py
M tests/hs2/test_hs2.py
14 files changed, 381 insertions(+), 59 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/8490/18
--
To view, visit http://gerrit.cloudera.org:8080/8490
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 18
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2017-12-18 Thread Zoltan Borok-Nagy (Code Review)

Zoltan Borok-Nagy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8490 )

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..


Patch Set 15:

After an offline discussion with Greg and Michael, the following conclusion was 
reached: FLAGS_idle_session_timeout should not limit the value of query option 
idle_session_timeout. From now on, any timeout value is allowed to be set by 
this query option.

I updated my commit with this in mind.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 15
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Mon, 18 Dec 2017 17:31:57 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2017-12-18 Thread Zoltan Borok-Nagy (Code Review)

Hello Michael Ho, Thomas Tauber-Marshall, Laszlo Gaal, Gabor Kaszab, Attila 
Jeges, Tim Armstrong, Csaba Ringhofer, Dan Hecht,

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

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

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

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..

IMPALA-2248: Make idle_session_timeout a query option

This commit makes idle_session_timeout a query option.

idle_session_timeout currently can be set as a command line
option, which will be the default timeout for sessions.
HS2 sessions can override it with a smaller value by setting
it in the configuration overlay of HS2 OpenSession().

However, we can't override idle_session_timeout for JDBC/ODBC
connections, because we cannot put this in the connection string.

This commit is a workaround for this problem, it allows JDBC/ODBC
connections to set the session timeout as a query option
with the SET statement.

After this commit, the session timeout can be overridden to
any value, i.e. the command line flag idle_session_timeout
doesn't limit this option anymore.

I created an automated test case in JdbcTest.java based on
test_hs2.py::test_concurrent_session_mixed_idle_timeout. I also
extended the test_session_expiration and test_set_and_unset
test suites.

Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
---
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
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.cc
M be/src/service/query-options.h
M common/thrift/ImpalaInternalService.thrift
M common/thrift/ImpalaService.thrift
M fe/src/test/java/org/apache/impala/service/JdbcTest.java
A fe/src/test/java/org/apache/impala/util/Metrics.java
M tests/custom_cluster/test_session_expiration.py
M tests/custom_cluster/test_set_and_unset.py
M tests/hs2/hs2_test_suite.py
14 files changed, 397 insertions(+), 53 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/8490/16
--
To view, visit http://gerrit.cloudera.org:8080/8490
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 16
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2017-12-15 Thread Zoltan Borok-Nagy (Code Review)

Zoltan Borok-Nagy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8490 )

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..


Patch Set 15:

(6 comments)

Thanks for the comments

http://gerrit.cloudera.org:8080/#/c/8490/14/be/src/service/client-request-state.h
File be/src/service/client-request-state.h:

http://gerrit.cloudera.org:8080/#/c/8490/14/be/src/service/client-request-state.h@428
PS14, Line 428: The timeout cannot be set to a higher
  :   /// value than FLAGS_idle_session_timeout.
> Can you please clarify the reasoning behind this constraint ?
I think the rationale behind it is to have a cap on the session timeout. So 
clients cannot set it arbitrary long either by mistake or malevolence.

It was the original behavior as well, and the JIRA says it explicitly:
"This could be set to a lower value by some clients".


http://gerrit.cloudera.org:8080/#/c/8490/14/be/src/service/impala-hs2-server.cc
File be/src/service/impala-hs2-server.cc:

http://gerrit.cloudera.org:8080/#/c/8490/14/be/src/service/impala-hs2-server.cc@338
PS14, Line 338:  (status.ok() && iequals(v.first, "idle_session_timeout")) {
> Instead of calling state->SetTimeout() here and below, may be we should jus
Done


http://gerrit.cloudera.org:8080/#/c/8490/14/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

http://gerrit.cloudera.org:8080/#/c/8490/14/be/src/service/impala-server.cc@191
PS14, Line 191: It can be overridden by the query option"
  : " 'idle_session_timeout' for specific sessions, but they 
can only have shorter"
  : " timeouts.");
> Is there a reason for this ? Won't that limit the usefulness of "set idle_s
Please see my other comment in this reply.


http://gerrit.cloudera.org:8080/#/c/8490/14/be/src/service/impala-server.cc@1234
PS14, Line 1234:   if (requested_timeout == 0) {
   : session_timeout = FLAGS_idle_session_timeout;
   :   } else if (FLAGS_idle_session_timeout == 0) {
   : session_timeout = requested_timeout;
   :   } else {
   : session_timeout = min(FLAGS_idle_session_timeout, 
requested_timeout);
   :   }
> Sorry, I find this logic a bit confusing.
Zero means there is no timeout, i.e. the session can be idle for infinite time.
If FLAGS_idle_session_timeout is infinite (zero), we can set the session 
timeout to any value.
If FLAGS_idle_session_timeout is a positive number, then it puts a cap on the 
session timeout that can be set online.

At least that's how I explained it to myself. OK, I'll send an email about this 
to Greg and you.


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

http://gerrit.cloudera.org:8080/#/c/8490/14/be/src/service/query-options.cc@584
PS14, Line 584:   if (requested_timeout == 0) {
  : return Status(Substitute("Session timeout cannot be 
unlimited, "
  : "maximum value: $0 seconds", 
FLAGS_idle_session_timeout));
  :   }
  :   if (requested_timeout > FLAGS_idle_session_timeout) {
  : return Status(Substitute(
  : "Session timeout cannot be set longer than $0 
seconds",
  : FLAGS_idle_session_timeout));
  :   }
> Please see comments elsewhere. I don't quite understand the reasoning behin
I tried to answer this by other comments in this reply.


http://gerrit.cloudera.org:8080/#/c/8490/14/fe/src/test/java/org/apache/impala/util/Metrics.java
File fe/src/test/java/org/apache/impala/util/Metrics.java:

http://gerrit.cloudera.org:8080/#/c/8490/14/fe/src/test/java/org/apache/impala/util/Metrics.java@1
PS14, Line 1: // Licensed to the Apache Software Foundation (ASF) under one
: // or more contributor license agreements.  See the NOTICE file
: // distributed with this work for additional information
: // regarding copyright ownership.  The ASF licenses this file
: // to you under the Apache License, Version 2.0 (the
: // "License"); you may not use this file except in compliance
: // with the License.  You may obtain a copy of the License at
: //
: //   http://www.apache.org/licenses/LICENSE-2.0
: //
: // Unless required by applicable law or agreed to in writing,
: // software distributed under the License is distributed on an
: // "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
: // KIND, either express or implied.  See the License for the
: // specific language governing permissions and limitations
: // under the License
> Seems to be missing a blank space between // and comments.

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2017-12-15 Thread Zoltan Borok-Nagy (Code Review)

Hello Michael Ho, Thomas Tauber-Marshall, Laszlo Gaal, Gabor Kaszab, Attila 
Jeges, Tim Armstrong, Csaba Ringhofer, Dan Hecht,

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

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

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

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..

IMPALA-2248: Make idle_session_timeout a query option

This commit makes idle_session_timeout a query option.
idle_session_timeout currently can be set as a command line
option, which will be the default timeout for sessions.
HS2 sessions can override it with a smaller value by setting
it in the configuration overlay of HS2 OpenSession().

However, we can't override idle_session_timeout for JDBC/ODBC
connections, because we cannot put this in the connection string.

This commit is a workaround for this problem, it allows JDBC/ODBC
connections to set the session timeout as a query option
with the SET statement.

I created an automated test case in JdbcTest.java based on
test_hs2.py::test_concurrent_session_mixed_idle_timeout. I also
extended the test_session_expiration and test_set_and_unset
test suites.

Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
---
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
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.cc
M be/src/service/query-options.h
M common/thrift/ImpalaInternalService.thrift
M common/thrift/ImpalaService.thrift
M fe/src/test/java/org/apache/impala/service/JdbcTest.java
A fe/src/test/java/org/apache/impala/util/Metrics.java
M tests/custom_cluster/test_session_expiration.py
M tests/custom_cluster/test_set_and_unset.py
M tests/hs2/hs2_test_suite.py
14 files changed, 426 insertions(+), 52 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/8490/15
--
To view, visit http://gerrit.cloudera.org:8080/8490
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 15
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2017-12-14 Thread Michael Ho (Code Review)

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8490 )

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..


Patch Set 14:

(6 comments)

Thanks for updating the interface. Looks much better now but I do have some 
questions about the reasoning about some of the constraints imposed in the code.

http://gerrit.cloudera.org:8080/#/c/8490/14/be/src/service/client-request-state.h
File be/src/service/client-request-state.h:

http://gerrit.cloudera.org:8080/#/c/8490/14/be/src/service/client-request-state.h@428
PS14, Line 428: The timeout cannot be set to a higher
  :   /// value than FLAGS_idle_session_timeout.
Can you please clarify the reasoning behind this constraint ?


http://gerrit.cloudera.org:8080/#/c/8490/14/be/src/service/impala-hs2-server.cc
File be/src/service/impala-hs2-server.cc:

http://gerrit.cloudera.org:8080/#/c/8490/14/be/src/service/impala-hs2-server.cc@338
PS14, Line 338: 
state->SetTimeout(state->set_query_options.idle_session_timeout);
Instead of calling state->SetTimeout() here and below, may be we should just  
one call site at line 346. We can store the target timeout value in a local 
variable on at line 319 and set it to FLAGS_idle_session_timeout by default. If 
it's overridden, we can update the local variable here.


http://gerrit.cloudera.org:8080/#/c/8490/14/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

http://gerrit.cloudera.org:8080/#/c/8490/14/be/src/service/impala-server.cc@191
PS14, Line 191: It can be overridden by the query option"
  : " 'idle_session_timeout' for specific sessions, but they 
can only have shorter"
  : " timeouts.");
Is there a reason for this ? Won't that limit the usefulness of "set 
idle_session_timeout" as a user cannot dynamically extend the timeout period ?


http://gerrit.cloudera.org:8080/#/c/8490/14/be/src/service/impala-server.cc@1234
PS14, Line 1234:   if (requested_timeout == 0) {
   : session_timeout = FLAGS_idle_session_timeout;
   :   } else if (FLAGS_idle_session_timeout == 0) {
   : session_timeout = requested_timeout;
   :   } else {
   : session_timeout = min(FLAGS_idle_session_timeout, 
requested_timeout);
   :   }
Sorry, I find this logic a bit confusing.

If FLAGS_idle_session_timeout is positive to begin with, setting it to zero 
will fail.

If FLAGS_idle_session_timeout is zero to begin with, I can set it to arbitrary 
non-negative number.

Otherwise, I can set it to any number between [1, FLAGS_idle_session_timeout].

What's the reasoning behind the above ? One thing which I am not so sure is 
okay is that I cannot disable idle session timeout if 
FLAGS_idle_session_timeout is originally set to a non-negative integer. In 
addition, I cannot increase the idle_session_timeout online, which seems to 
limit its usefulness to be able to be overridden per-session.

May be it helps if Greg can chime in on the legitimate expected behavior.


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

http://gerrit.cloudera.org:8080/#/c/8490/14/be/src/service/query-options.cc@584
PS14, Line 584:   if (requested_timeout == 0) {
  : return Status(Substitute("Session timeout cannot be 
unlimited, "
  : "maximum value: $0 seconds", 
FLAGS_idle_session_timeout));
  :   }
  :   if (requested_timeout > FLAGS_idle_session_timeout) {
  : return Status(Substitute(
  : "Session timeout cannot be set longer than $0 
seconds",
  : FLAGS_idle_session_timeout));
  :   }
Please see comments elsewhere. I don't quite understand the reasoning behind 
these constraints.


http://gerrit.cloudera.org:8080/#/c/8490/14/fe/src/test/java/org/apache/impala/util/Metrics.java
File fe/src/test/java/org/apache/impala/util/Metrics.java:

http://gerrit.cloudera.org:8080/#/c/8490/14/fe/src/test/java/org/apache/impala/util/Metrics.java@1
PS14, Line 1: //Licensed to the Apache Software Foundation (ASF) under one
: //or more contributor license agreements.  See the NOTICE file
: //distributed with this work for additional information
: //regarding copyright ownership.  The ASF licenses this file
: //to you under the Apache License, Version 2.0 (the
: //"License"); you may not use this file except in compliance
: //with the License.  You may obtain a copy of the License at
: //
: //http://www.apache.org/licenses/LICENSE-2.0
: //
: //Unless required by applicable law or agreed to in writing,
: //software distributed under the

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2017-12-14 Thread Gabor Kaszab (Code Review)

Gabor Kaszab has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8490 )

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..


Patch Set 14: Code-Review+1

Thanks for sharing your view, Zoli!


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 14
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Thu, 14 Dec 2017 19:16:04 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2017-12-14 Thread Zoltan Borok-Nagy (Code Review)

Zoltan Borok-Nagy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8490 )

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..


Patch Set 14:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8490/14/be/src/service/impala-server.h@452
PS14, Line 452:   friend class SessionState;
> Well, I feel here that making SessionState a friend class so that the Regis
I actually made the register/unregister functions private in response to 
Michael's comment.

I think it's reasonable that timeouts can only be registered via 
SessionState::SetTimeout().

I agree that it is unfortunate that SessionState now accesses all members of 
ImpalaServer. However, friend declarations only extend the access of private 
fields, but do not break the access boundary.
Making a member public allows everybody to depend on that member. But, it might 
be not a huge issue in application code.

SessionState is defined inside the class definition of ImpalaServer, therefore 
making it a friend is not a huge problem either in my opinion.

It is aligned with the google style guide as well:
https://google.github.io/styleguide/cppguide.html#Friends



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 14
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Thu, 14 Dec 2017 14:06:41 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2017-12-13 Thread Zoltan Borok-Nagy (Code Review)

Hello Michael Ho, Thomas Tauber-Marshall, Laszlo Gaal, Gabor Kaszab, Attila 
Jeges, Tim Armstrong, Csaba Ringhofer, Dan Hecht,

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

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

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

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..

IMPALA-2248: Make idle_session_timeout a query option

This commit makes idle_session_timeout a query option.
idle_session_timeout currently can be set as a command line
option, which will be the default timeout for sessions.
HS2 sessions can override it with a smaller value by setting
it in the configuration overlay of HS2 OpenSession().

However, we can't override idle_session_timeout for JDBC/ODBC
connections, because we cannot put this in the connection string.

This commit is a workaround for this problem, it allows JDBC/ODBC
connections to set the session timeout as a query option
with the SET statement.

I created an automated test case in JdbcTest.java based on
test_hs2.py::test_concurrent_session_mixed_idle_timeout. I also
extended the test_session_expiration and test_set_and_unset
test suites.

Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
---
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
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.cc
M be/src/service/query-options.h
M common/thrift/ImpalaInternalService.thrift
M common/thrift/ImpalaService.thrift
M fe/src/test/java/org/apache/impala/service/JdbcTest.java
A fe/src/test/java/org/apache/impala/util/Metrics.java
M tests/custom_cluster/test_session_expiration.py
M tests/custom_cluster/test_set_and_unset.py
M tests/hs2/hs2_test_suite.py
14 files changed, 427 insertions(+), 52 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/8490/14
--
To view, visit http://gerrit.cloudera.org:8080/8490
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 14
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2017-12-12 Thread Michael Ho (Code Review)

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8490 )

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..


Patch Set 13:

(4 comments)

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

http://gerrit.cloudera.org:8080/#/c/8490/13/be/src/service/impala-server.h@342
PS13, Line 342:   /// Register timeout value upon opening a new session. This 
will wake up
  :   /// session_timeout_thread_ to update its poll period.
  :   void RegisterSessionTimeout(int32_t timeout);
  :
  :   /// Unregister timeout value. This will wake up 
session_timeout_thread_
  :   /// to update its poll period.
  :   void UnregisterSessionTimeout(int32_t timeout);
Should these functions be private ?

Actually, it seems that the current code separates the setting of the the 
session's idle timeout (i.e. SetTimeout) from the actual registration / 
unregistration with session_timeout_thread_. Is there a reason for this 
separation ?

In other words, can we have SetTimeout() as the only exposed interface for 
changing idle session timeout value ? It will be responsible for determining 
the new timeout value based on the requested timeout value and 
FLAGS_idle_session_timeout and call UnregisterSessionTimeout() and 
RegisterSessionTimeout() if the timeout value changes.


http://gerrit.cloudera.org:8080/#/c/8490/13/be/src/service/impala-server.h@437
PS13, Line 437: FLAGS_idle_session_timeout is used
Also, it also seems to use FLAGS_idle_session_timeout as the value if 
requested_timeout is 0.


http://gerrit.cloudera.org:8080/#/c/8490/13/be/src/service/impala-server.h@438
PS13, Line 438: This method also sets the query option 'idle_session_timeout'
Is this still true ?


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

http://gerrit.cloudera.org:8080/#/c/8490/13/be/src/service/query-options.cc@582
PS13, Line 582: else if (requested_timeout == 0 && FLAGS_idle_session_timeout 
!= 0) {
  :   return Status(
  :   Substitute("Session timeout cannot be unlimited, "
  :  "maximum value: $0 seconds", 
FLAGS_idle_session_timeout));
  : } else if (requested_timeout > 
FLAGS_idle_session_timeout &&
  :FLAGS_idle_session_timeout != 0) {
  :   return Status(Substitute("Session timeout cannot be 
set longer than $0 seconds",
  :   FLAGS_idle_session_timeout));
  : }
Just a minor suggestion:

 if (FLAGS_idle_session_timeout != 0) {
 if (request_timeout == 0) {
 return ...
 }
 if (request_timeout > FLAGS_idle_session_timeout) {
 return 
 }
 }



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 13
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Tue, 12 Dec 2017 20:17:01 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2017-12-12 Thread Gabor Kaszab (Code Review)

Gabor Kaszab has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8490 )

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..


Patch Set 13: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 13
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Tue, 12 Dec 2017 11:54:04 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2017-12-07 Thread Zoltan Borok-Nagy (Code Review)

Zoltan Borok-Nagy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8490 )

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..


Patch Set 13:

(3 comments)

Thanks!

http://gerrit.cloudera.org:8080/#/c/8490/10/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:

http://gerrit.cloudera.org:8080/#/c/8490/10/be/src/service/client-request-state.cc@207
PS10, Line 207: RETURN_IF_ERROR(SetQueryOption(key, value, 
_->set_query_options,
> Can we also add a couple of test cases to "set.test" to test the validation
I added couple of extra checks to test_set_and_unset_session_timeout


http://gerrit.cloudera.org:8080/#/c/8490/11/be/src/service/impala-hs2-server.cc
File be/src/service/impala-hs2-server.cc:

http://gerrit.cloudera.org:8080/#/c/8490/11/be/src/service/impala-hs2-server.cc@333
PS11, Line 333:   } else {
> This seems overall better, thanks!
Yeah, I also think it's better this way. You're welcome!


http://gerrit.cloudera.org:8080/#/c/8490/11/fe/src/test/java/org/apache/impala/service/JdbcTest.java
File fe/src/test/java/org/apache/impala/service/JdbcTest.java:

http://gerrit.cloudera.org:8080/#/c/8490/11/fe/src/test/java/org/apache/impala/service/JdbcTest.java@626
PS11, Line 626:   // while renewing the remainders by issuing a query
> Yeah I like the test case, just took a while to understand it initially.
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 13
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Thu, 07 Dec 2017 13:55:59 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2017-12-06 Thread Zoltan Borok-Nagy (Code Review)

Zoltan Borok-Nagy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8490 )

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..


Patch Set 11:

(10 comments)

Thanks!

http://gerrit.cloudera.org:8080/#/c/8490/10//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8490/10//COMMIT_MSG@7
PS10, Line 7: IMPALA-2248: Make idle_session_timeout a query option
> Sounds reasonable. We'll have to make sure to document the limitations clea
I'll need some help in filing the Doc JIRA, but sure!


http://gerrit.cloudera.org:8080/#/c/8490/11//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8490/11//COMMIT_MSG@20
PS11, Line 20: SET statement.
> Quick question about the behavior of setting this query option: if the sess
I updated the code not to expire anything with the SET statement. The last 
accessed time of the session is updated immediately, therefore the session 
won't expire for at least the newly set timeout.
It shouldn't affect the queries either, they have their own timeouts.


http://gerrit.cloudera.org:8080/#/c/8490/10/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:

http://gerrit.cloudera.org:8080/#/c/8490/10/be/src/service/client-request-state.cc@207
PS10, Line 207: if (iequals(key, "idle_session_timeout")) {
> Right, but if the client provides an invalid value, I think it will just ge
OK, I restructured my code. Now the validation mainly happens in 
SetQueryOption(), and the client will get an error if they want to set it to an 
invalid value.
(An exception to this is HS2 OpenSession(), which used to ignore the errors 
related to query options, so I didn't change the behavior there.)


http://gerrit.cloudera.org:8080/#/c/8490/11/be/src/service/impala-hs2-server.cc
File be/src/service/impala-hs2-server.cc:

http://gerrit.cloudera.org:8080/#/c/8490/11/be/src/service/impala-hs2-server.cc@333
PS11, Line 333:   } else if (iequals(v.first, "idle_session_timeout")) {
> Can this fall through to SetQueryOption() below now that it's supported ?
I modified the code structure, now some part of the validation is done in 
SetQueryTimeout(), but some special casing is still needed.


http://gerrit.cloudera.org:8080/#/c/8490/11/fe/src/test/java/org/apache/impala/service/JdbcTest.java
File fe/src/test/java/org/apache/impala/service/JdbcTest.java:

http://gerrit.cloudera.org:8080/#/c/8490/11/fe/src/test/java/org/apache/impala/service/JdbcTest.java@616
PS11, Line 616: ew Long(
> nit: is the new Long() necessary? Seems like it should be automatically cas
Right, done.


http://gerrit.cloudera.org:8080/#/c/8490/11/fe/src/test/java/org/apache/impala/service/JdbcTest.java@626
PS11, Line 626:   int sleepPeriod = (int)(timeout * timeoutTolerance * 
1000) + 500;
> I think the wakeup timing could do with a brief explanation. Is the idea is
Right, that's the idea. I implemented this test case based on 
test_concurrent_session_mixed_idle_timeout in tests/hs2/test_hs2.py. Maybe it's 
redundant, but it is a thorough test of the jdbc client regarding to timeouts.
I added some description to the beginning of the loop.


http://gerrit.cloudera.org:8080/#/c/8490/11/fe/src/test/java/org/apache/impala/service/JdbcTest.java@634
PS11, Line 634: new Long
> same here
Done


http://gerrit.cloudera.org:8080/#/c/8490/11/fe/src/test/java/org/apache/impala/service/JdbcTest.java@636
PS11, Line 636: Boolean
> bool? Not sure why we need to use the boxed type
Done


http://gerrit.cloudera.org:8080/#/c/8490/11/fe/src/test/java/org/apache/impala/service/JdbcTest.java@639
PS11, Line 639:   try(ResultSet rs = 
connection.createStatement().executeQuery("SELECT 1+2")) {
> nit: whitespace
Done


http://gerrit.cloudera.org:8080/#/c/8490/11/fe/src/test/java/org/apache/impala/util/Metrics.java
File fe/src/test/java/org/apache/impala/util/Metrics.java:

http://gerrit.cloudera.org:8080/#/c/8490/11/fe/src/test/java/org/apache/impala/util/Metrics.java@43
PS11, Line 43:   public Object getMetric(String metric) throws Exception {
> A brief comment would be helpful, mainly to explain what it returns - when
Added a javadoc comment to the method.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 11
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2017-12-06 Thread Zoltan Borok-Nagy (Code Review)

Hello Michael Ho, Thomas Tauber-Marshall, Laszlo Gaal, Gabor Kaszab, Attila 
Jeges, Tim Armstrong, Csaba Ringhofer, Dan Hecht,

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

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

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

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..

IMPALA-2248: Make idle_session_timeout a query option

This commit makes idle_session_timeout a query option.
idle_session_timeout currently can be set as a command line
option, which will be the default timeout for sessions.
HS2 sessions can override it with a smaller value by setting
it in the configuration overlay of HS2 OpenSession().

However, we can't override idle_session_timeout for JDBC/ODBC
connections, because we cannot put this in the connection string.

This commit is a workaround for this problem, it allows JDBC/ODBC
connections to set the session timeout as a query option
with the SET statement.

I created an automated test case in JdbcTest.java based on
test_hs2.py::test_concurrent_session_mixed_idle_timeout. I also
extended the test_session_expiration and test_set_and_unset
test suites.

Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
---
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
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.cc
M be/src/service/query-options.h
M common/thrift/ImpalaInternalService.thrift
M common/thrift/ImpalaService.thrift
M fe/src/test/java/org/apache/impala/service/JdbcTest.java
A fe/src/test/java/org/apache/impala/util/Metrics.java
M tests/custom_cluster/test_session_expiration.py
M tests/custom_cluster/test_set_and_unset.py
13 files changed, 375 insertions(+), 45 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/8490/12
--
To view, visit http://gerrit.cloudera.org:8080/8490
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 12
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2017-12-05 Thread Michael Ho (Code Review)

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8490 )

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..


Patch Set 11:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8490/11//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8490/11//COMMIT_MSG@20
PS11, Line 20: SET statement.
Quick question about the behavior of setting this query option: if the session 
timeout is changed to a smaller value in the middle of a session, some existing 
idle queries in the same session can now suddenly time out, right ?


http://gerrit.cloudera.org:8080/#/c/8490/11/be/src/service/impala-hs2-server.cc
File be/src/service/impala-hs2-server.cc:

http://gerrit.cloudera.org:8080/#/c/8490/11/be/src/service/impala-hs2-server.cc@333
PS11, Line 333:   } else if (iequals(v.first, "idle_session_timeout")) {
Can this fall through to SetQueryOption() below now that it's supported ?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 11
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Wed, 06 Dec 2017 02:55:40 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2017-12-04 Thread Tim Armstrong (Code Review)

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

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..


Patch Set 10:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/8490/10//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8490/10//COMMIT_MSG@7
PS10, Line 7: IMPALA-2248: Make idle_session_timeout a query option
> * Yeah, currently it doesn't have any effect for the shell. As you said, it
Sounds reasonable. We'll have to make sure to document the limitations clearly 
- we can file a Doc JIRA as a follow-up.


http://gerrit.cloudera.org:8080/#/c/8490/10/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:

http://gerrit.cloudera.org:8080/#/c/8490/10/be/src/service/client-request-state.cc@207
PS10, Line 207:   UpdateSessionTimeout(atoi(value.c_str()));
> At this point the value can be invalid, e.g. the client wants to set the ti
Right, but if the client provides an invalid value, I think it will just get 
swallowed up here. E.g. I tried set idle_session_timeout="foo"; and it seems to 
fail silently.

I'd expect it return the "Invalid idle session timeout: " error to the client. 
I think we should add tests to set.test  for validation of the option.


http://gerrit.cloudera.org:8080/#/c/8490/11/fe/src/test/java/org/apache/impala/service/JdbcTest.java
File fe/src/test/java/org/apache/impala/service/JdbcTest.java:

http://gerrit.cloudera.org:8080/#/c/8490/11/fe/src/test/java/org/apache/impala/service/JdbcTest.java@616
PS11, Line 616: ew Long(
nit: is the new Long() necessary? Seems like it should be automatically cast


http://gerrit.cloudera.org:8080/#/c/8490/11/fe/src/test/java/org/apache/impala/service/JdbcTest.java@626
PS11, Line 626:   int sleepPeriod = (int)(timeout * timeoutTolerance * 
1000) + 500;
I think the wakeup timing could do with a brief explanation. Is the idea is 
that each iteration might let one session expire and then renews the timeout 
for the remaining sessions?


http://gerrit.cloudera.org:8080/#/c/8490/11/fe/src/test/java/org/apache/impala/service/JdbcTest.java@634
PS11, Line 634: new Long
same here


http://gerrit.cloudera.org:8080/#/c/8490/11/fe/src/test/java/org/apache/impala/service/JdbcTest.java@636
PS11, Line 636: Boolean
bool? Not sure why we need to use the boxed type


http://gerrit.cloudera.org:8080/#/c/8490/11/fe/src/test/java/org/apache/impala/service/JdbcTest.java@639
PS11, Line 639:   try(ResultSet rs = 
connection.createStatement().executeQuery("SELECT 1+2")) {
nit: whitespace


http://gerrit.cloudera.org:8080/#/c/8490/11/fe/src/test/java/org/apache/impala/util/Metrics.java
File fe/src/test/java/org/apache/impala/util/Metrics.java:

http://gerrit.cloudera.org:8080/#/c/8490/11/fe/src/test/java/org/apache/impala/util/Metrics.java@43
PS11, Line 43:   public Object getMetric(String metric) throws Exception {
A brief comment would be helpful, mainly to explain what it returns - when 
reading the test I was initially confused about the casting to (Long).



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 10
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Tue, 05 Dec 2017 01:25:00 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2017-12-01 Thread Zoltan Borok-Nagy (Code Review)

Zoltan Borok-Nagy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8490 )

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..


Patch Set 10:

(5 comments)

Thanks for the review.

http://gerrit.cloudera.org:8080/#/c/8490/10//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8490/10//COMMIT_MSG@7
PS10, Line 7: IMPALA-2248: Make idle_session_timeout a query option
> I did an initial pass over the code and wanted to clarify my understanding
* Yeah, currently it doesn't have any effect for the shell. As you said, it 
only updates the client-side map, and this map is then sent with a query as 
config overlay. Options in this configuration overlay are only applied for that 
specific query, so my thought was that setting session timeout in a query 
config overlay doesn't make sense. Unfortunately, this means we cannot set the 
session timeout for the impala-shell.
* Yeah, I exactly thought the same. From a given session we can send queries to 
different pools, so the pool config should not affect the session timeout.

Yes, I think session timeout doesn't really belong to query options, since 
sessions are at a different level than queries. I only think of it as a 
workaround for JDBC clients that couldn't set it otherwise. Unfortunately it 
also makes some special case unavoidable I think.


http://gerrit.cloudera.org:8080/#/c/8490/10/be/src/service/client-request-state.h
File be/src/service/client-request-state.h:

http://gerrit.cloudera.org:8080/#/c/8490/10/be/src/service/client-request-state.h@423
PS10, Line 423:   void UpdateSessionTimeout(int32_t requested_timeout);
> I think this assumes that session_->lock_ is held. We should be careful to
Yes, I added a comment about it.


http://gerrit.cloudera.org:8080/#/c/8490/10/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:

http://gerrit.cloudera.org:8080/#/c/8490/10/be/src/service/client-request-state.cc@207
PS10, Line 207:   UpdateSessionTimeout(atoi(value.c_str()));
> It feels a bit weird that we're doing string parsing of a query option here
At this point the value can be invalid, e.g. the client wants to set the 
timeout for a longer period of time which is not permitted.
This validation now happens in SetTimeout(), which will set the timeout to a 
valid value, after that SetTimeout() will set the query option to this valid 
value to keep things in sync.


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

http://gerrit.cloudera.org:8080/#/c/8490/10/be/src/service/impala-server.h@436
PS10, Line 436: void SetTimeout(int32_t requested_timeout);
> The caller must hold 'lock', right?
Yes, I added a comment about it.
Unfortunately I cannot DCHECK if the lock is owned by the current thread.


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

http://gerrit.cloudera.org:8080/#/c/8490/10/be/src/service/query-options.h@99
PS10, Line 99:   QUERY_OPT_FN(idle_session_timeout, IDLE_SESSION_TIMEOUT)\
> Meanwhile, I managed to submit my query option level changes. So this line
I talked with Tim about it, and we agreed that it should be regular.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 10
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Fri, 01 Dec 2017 15:20:57 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2017-12-01 Thread Zoltan Borok-Nagy (Code Review)

Hello Michael Ho, Thomas Tauber-Marshall, Laszlo Gaal, Gabor Kaszab, Attila 
Jeges, Tim Armstrong, Csaba Ringhofer, Dan Hecht,

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

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

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

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..

IMPALA-2248: Make idle_session_timeout a query option

This commit makes idle_session_timeout a query option.
idle_session_timeout currently can be set as a command line
option, which will be the default timeout for sessions.
HS2 sessions can override it with a smaller value by setting
it in the configuration overlay of HS2 OpenSession().

However, we can't override idle_session_timeout for JDBC/ODBC
connections, because we cannot put this in the connection string.

This commit is a workaround for this problem, it allows JDBC/ODBC
connections to set the session timeout as a query option
with the SET statement.

I created an automated test case in JdbcTest.java based on
test_hs2.py::test_concurrent_session_mixed_idle_timeout. I also
extended the test_session_expiration and test_set_and_unset
test suites.

Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
---
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
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.cc
M be/src/service/query-options.h
M common/thrift/ImpalaInternalService.thrift
M common/thrift/ImpalaService.thrift
M fe/src/test/java/org/apache/impala/service/JdbcTest.java
A fe/src/test/java/org/apache/impala/util/Metrics.java
M tests/custom_cluster/test_session_expiration.py
M tests/custom_cluster/test_set_and_unset.py
13 files changed, 346 insertions(+), 39 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 11
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2017-11-30 Thread Tim Armstrong (Code Review)

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

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..


Patch Set 10:

(5 comments)

Did an initial pass. Still need to look at the tests in detail.

http://gerrit.cloudera.org:8080/#/c/8490/10//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8490/10//COMMIT_MSG@7
PS10, Line 7: IMPALA-2248: Make idle_session_timeout a query option
I did an initial pass over the code and wanted to clarify my understanding of 
the current behaviour:

* Does "set idle_session_timeout" in impala-shell have any effect? I wouldn't 
think so since it just updates a client-side map, right?
* The request pool query options overlay doesn't have an effect on this option, 
does it? Since a session doesn't belong to a request pool.

I think unfortunately this will end up being a bit of a special case in terms 
of behaviour, but I think this is expected.


http://gerrit.cloudera.org:8080/#/c/8490/10/be/src/service/client-request-state.h
File be/src/service/client-request-state.h:

http://gerrit.cloudera.org:8080/#/c/8490/10/be/src/service/client-request-state.h@423
PS10, Line 423:   void UpdateSessionTimeout(int32_t requested_timeout);
I think this assumes that session_->lock_ is held. We should be careful to 
document assumptions about which locks are and aren't held in this part of the 
code (there are unfortunately a lot of locks).


http://gerrit.cloudera.org:8080/#/c/8490/10/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:

http://gerrit.cloudera.org:8080/#/c/8490/10/be/src/service/client-request-state.cc@207
PS10, Line 207:   UpdateSessionTimeout(atoi(value.c_str()));
It feels a bit weird that we're doing string parsing of a query option here. 
Maybe we should set the query option first and then get the parsed value?


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

http://gerrit.cloudera.org:8080/#/c/8490/10/be/src/service/impala-server.h@436
PS10, Line 436: void SetTimeout(int32_t requested_timeout);
The caller must hold 'lock', right?


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

http://gerrit.cloudera.org:8080/#/c/8490/7/be/src/service/impala-server.h@423
PS7, Line 423: /// session may be correctly expired after a timeout (when 
ref_count == 0). Typically
Need to think about whether this is necessary - should we just use 
set_query_options?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 10
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Fri, 01 Dec 2017 02:13:08 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2017-11-27 Thread Gabor Kaszab (Code Review)

Gabor Kaszab has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8490 )

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..


Patch Set 10:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8490/10/be/src/service/query-options.h@99
PS10, Line 99:   QUERY_OPT_FN(idle_session_timeout, IDLE_SESSION_TIMEOUT)\
Meanwhile, I managed to submit my query option level changes. So this line has 
to be extended now with an option level. I'd say to make it regular, but could 
you confirm it with Tim/Dan?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 10
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Tue, 28 Nov 2017 05:15:20 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2017-11-27 Thread Zoltan Borok-Nagy (Code Review)

Zoltan Borok-Nagy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8490 )

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..


Patch Set 9:

(7 comments)

Thanks!

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

http://gerrit.cloudera.org:8080/#/c/8490/9/be/src/service/impala-server.h@441
PS9, Line 441:   friend class ClientRequestState;
> This is required so that ClientRequestState can call UnregisterSessionTimeo
Right. OK, I changed the visibility of the timeout methods.


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

http://gerrit.cloudera.org:8080/#/c/8490/9/be/src/service/impala-server.cc@1240
PS9, Line 1240:   std::to_string(this->session_timeout),
> I have no knowledge about how threading works around this part of the code.
SetTimeout() is only called at places that modified session_timeout and query 
options already.


http://gerrit.cloudera.org:8080/#/c/8490/9/be/src/service/impala-server.cc@1816
PS9, Line 1816: session_timeout_cv_.NotifyOne();
> In RegisterSessionTimeout() the notify_one call is also protected by the lo
It is not needed to hold the mutex during the notify call in 
RegisterSessionTimeout either, so I fixed it as well.
It is valid to call notify on a condition variable without holding the 
associated mutex. And it is not just valid, but more efficient also, because 
otherwise the notified thread would block on the mutex immediately. More on 
that:
https://stackoverflow.com/questions/17101922/do-i-have-to-acquire-lock-before-calling-condition-variable-notify-one/


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

http://gerrit.cloudera.org:8080/#/c/8490/9/be/src/service/query-options.h@99
PS9, Line 99:   QUERY_OPT_FN(idle_session_timeout, IDLE_SESSION_TIMEOUT)\
> For the sake of IMPALA-2181, could you find out which of the 4 new option g
Yeah, it's definitely regular or advanced. Maybe regular, but I don't have a 
strong opinion about it.


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

http://gerrit.cloudera.org:8080/#/c/8490/9/be/src/service/query-options.cc@578
PS9, Line 578: "Only positive numbers are allowed.", value));
> I think the '4 spaces for indentation' rule applies here as well. I would e
Done, I chose the second option.


http://gerrit.cloudera.org:8080/#/c/8490/9/tests/custom_cluster/test_session_expiration.py
File tests/custom_cluster/test_session_expiration.py:

http://gerrit.cloudera.org:8080/#/c/8490/9/tests/custom_cluster/test_session_expiration.py@62
PS9, Line 62: sleep(3)
> Could you please re-work these sleep times a little bit to reduce the runti
I reduced the timeouts, also in JdbcTest.java.


http://gerrit.cloudera.org:8080/#/c/8490/9/tests/custom_cluster/test_session_expiration.py@70
PS9, Line 70: sleep(8)
> I learnt this week that running the test suite on RELEASE and ASAN build mi
Tried those builds, there were no problems on my PC.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 9
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Mon, 27 Nov 2017 15:03:51 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2017-11-20 Thread Zoltan Borok-Nagy (Code Review)

Hello Michael Ho, Thomas Tauber-Marshall, Laszlo Gaal, Gabor Kaszab, Attila 
Jeges, Tim Armstrong, Csaba Ringhofer, Dan Hecht,

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

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

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

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..

IMPALA-2248: Make idle_session_timeout a query option

This commit makes idle_session_timeout a query option.
idle_session_timeout currently can be set as a command line
option, which will be the default timeout for sessions.
HS2 sessions can override it with a smaller value by setting
it in the configuration overlay of HS2 OpenSession().

However, we can't override idle_session_timeout for JDBC/ODBC
connections, because we cannot put this in the connection string.

This commit is a workaround for this problem, it allows JDBC/ODBC
connections to set the session timeout as a query option
with the SET statement.

I created an automated test case in JdbcTest.java based on
test_hs2.py::test_concurrent_session_mixed_idle_timeout. I also
extended the test_session_expiration and test_set_and_unset
test suites.

Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
---
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
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.cc
M be/src/service/query-options.h
M common/thrift/ImpalaInternalService.thrift
M common/thrift/ImpalaService.thrift
M fe/src/test/java/org/apache/impala/service/JdbcTest.java
A fe/src/test/java/org/apache/impala/util/Metrics.java
M tests/custom_cluster/test_session_expiration.py
M tests/custom_cluster/test_set_and_unset.py
13 files changed, 340 insertions(+), 34 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 9
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

2017-11-16 Thread Zoltan Borok-Nagy (Code Review)

Hello Michael Ho, Thomas Tauber-Marshall, Laszlo Gaal, Gabor Kaszab, Attila 
Jeges, Tim Armstrong, Csaba Ringhofer, Dan Hecht,

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

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

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

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..

IMPALA-2248: Make idle_session_timeout a query option

This commit makes idle_session_timeout a query option.
idle_session_timeout currently can be set as a command line
option, which will be the default timeout for sessions.
HS2 sessions can override it with a smaller value by setting
it in the configuration overlay of HS2 OpenSession().

However, we can't override idle_session_timeout for JDBC/ODBC
connections, because we cannot put this in the connection string.

This commit is a workaround for this problem, it allows JDBC/ODBC
connections to set the session timeout as a query option
with the SET statement.

I created an automated test case in JdbcTest.java based on
test_hs2.py::test_concurrent_session_mixed_idle_timeout.

Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
---
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
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.cc
M be/src/service/query-options.h
M common/thrift/ImpalaInternalService.thrift
M common/thrift/ImpalaService.thrift
M fe/src/test/java/org/apache/impala/service/JdbcTest.java
A fe/src/test/java/org/apache/impala/util/Metrics.java
11 files changed, 314 insertions(+), 33 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 7
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy

38 matches

Mail list logo