[Impala-ASF-CR] IMPALA-2235: Fix current db when shell auto-reconnects
Zoltan Borok-Nagy has posted comments on this change. (
http://gerrit.cloudera.org:8080/8368 )
Change subject: IMPALA-2235: Fix current db when shell auto-reconnects
..
Patch Set 5:
(1 comment)
Thanks for the review
http://gerrit.cloudera.org:8080/#/c/8368/4/tests/custom_cluster/test_shell_interactive_reconnect.py
File tests/custom_cluster/test_shell_interactive_reconnect.py:
http://gerrit.cloudera.org:8080/#/c/8368/4/tests/custom_cluster/test_shell_interactive_reconnect.py@60
PS4, Line 60: start_num_queries = impalad.get_metric_value(NUM_QUERIES)
> Would "p.send_cmd("show tables"); p.get_result()" make this stable, without
Yeah I also felt wrong about the sleep for the same reason, but didn't know any
better.
Unfortunately get_result() closes STDIN and that exits the Impala shell process.
But, looking at other test codes I figured out how I can use the ImpaladService
class to wait for my USE command.
--
To view, visit http://gerrit.cloudera.org:8080/8368
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I40dfa00ba0314d356fe8617446f516505c925e5e
Gerrit-Change-Number: 8368
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Borok-Nagy
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Zoltan Borok-Nagy
Gerrit-Comment-Date: Tue, 14 Nov 2017 23:27:55 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2235: Fix current db when shell auto-reconnects
Hello Philip Zeyliger, Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8368 to look at the new patch set (#5). Change subject: IMPALA-2235: Fix current db when shell auto-reconnects .. IMPALA-2235: Fix current db when shell auto-reconnects The ImpalaShell didn't issue the 'USE ' command after reconnecting to the Impala daemon. Therefore the client session used the default DB after reconnection, not the previously selected DB. Setting the current DB is done by the _validate_database method. Before this commit it appended the "use " command to the command queue of the Cmd class. But, at this point we might already have commands in the command queue that will run before the "use " command. In case of reconnection, we want to invoke the USE command right away. Also, the command processed by the precmd() method can entirely skip the command queue, therefore it is not enough to insert the USE command to the front of the command queue. We need to issue the USE command with the onecmd() method to execute it immediately. I extended the _validate_database method with an "immediately" flag. If this flag is true, _validate_database will use the onecmd() method. Otherwise, it will append the USE command to the command queue to maintain the previous behaviour. I added a new automated test suite named test_shell_interactive_reconnect.py to the "custom cluster" tests. It sets the default database, and after reconnection it checks if the shell set it again automatically. One test case checks if the shell set the DB after manually reconnecting to the impala daemon by issuing the CONNECT command. The other test case checks if the shell set the DB after automatic reconnection due to cluster restart. I needed to backup the impala shell history file because I didn't want to pollute it by the test cases (just like the way it is done in tests/shell/test_shell_interactive.py). I created utility functions for this in tests/shell/util.py and now test_shell_interactive.py and the newly created test suite are using these utility functions. Change-Id: I40dfa00ba0314d356fe8617446f516505c925e5e --- M shell/impala_shell.py A tests/custom_cluster/test_shell_interactive_reconnect.py M tests/shell/test_shell_interactive.py M tests/shell/util.py 4 files changed, 106 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/8368/5 -- To view, visit http://gerrit.cloudera.org:8080/8368 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I40dfa00ba0314d356fe8617446f516505c925e5e Gerrit-Change-Number: 8368 Gerrit-PatchSet: 5 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-2235: Fix current db when shell auto-reconnects
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8368 ) Change subject: IMPALA-2235: Fix current db when shell auto-reconnects .. Patch Set 4: (3 comments) Thanks for the review. In addition I also added a time.sleep(1) to the test case test_auto_reconnect because I noticed that it wasn't stable. This sleep gives time for the ImpalaShell to successfully start and connect to the Impala daemon before we restart the cluster. http://gerrit.cloudera.org:8080/#/c/8368/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8368/3//COMMIT_MSG@9 PS3, Line 9: The ImpalaShell didn't issue the 'USE ' command after > This sentence didn't parse for me. Specifically, I'm not sure what "that" i I rephrased the beginning of the commit. I hope it is easier to understand now. http://gerrit.cloudera.org:8080/#/c/8368/3/tests/custom_cluster/test_shell_interactive_reconnect.py File tests/custom_cluster/test_shell_interactive_reconnect.py: http://gerrit.cloudera.org:8080/#/c/8368/3/tests/custom_cluster/test_shell_interactive_reconnect.py@36 PS3, Line 36: : cls.tempfile_name = tempfile.mktemp() : move_shell_history(cls.tempfile_n > Minor: I think this is just: I only used NamedTemporaryFile because mktemp is marked as deprecated: https://docs.python.org/2/library/tempfile.html#tempfile.mktemp tempfile.mkstemp() could have also been used, but it also creates a file, and returns an open file descriptor for that. Anyway, since it is in a test case, I don't see that mktemp() would cause a vulnerability issue, so now I use it for brevity and better readability. Done. http://gerrit.cloudera.org:8080/#/c/8368/3/tests/shell/util.py File tests/shell/util.py: http://gerrit.cloudera.org:8080/#/c/8368/3/tests/shell/util.py@121 PS3, Line 121: def restore_shell_history(filepath): > mention that this is a no-op if filepath doesn't exist. Done. -- To view, visit http://gerrit.cloudera.org:8080/8368 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I40dfa00ba0314d356fe8617446f516505c925e5e Gerrit-Change-Number: 8368 Gerrit-PatchSet: 4 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Tue, 14 Nov 2017 15:34:03 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2235: Fix current db when shell auto-reconnects
Hello Philip Zeyliger, Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8368 to look at the new patch set (#4). Change subject: IMPALA-2235: Fix current db when shell auto-reconnects .. IMPALA-2235: Fix current db when shell auto-reconnects The ImpalaShell didn't issue the 'USE ' command after reconnecting to the Impala daemon. Therefore the client session used the default DB after reconnection, not the previously selected DB. Setting the current DB is done by the _validate_database method. Before this commit it appended the "use " command to the command queue of the Cmd class. But, at this point we might already have commands in the command queue that will run before the "use " command. In case of reconnection, we want to invoke the USE command right away. Also, the command processed by the precmd() method can entirely skip the command queue, therefore it is not enough to insert the USE command to the front of the command queue. We need to issue the USE command with the onecmd() method to execute it immediately. I extended the _validate_database method with an "immediately" flag. If this flag is true, _validate_database will use the onecmd() method. Otherwise, it will append the USE command to the command queue to maintain the previous behaviour. I added a new automated test suite named test_shell_interactive_reconnect.py to the "custom cluster" tests. It sets the default database, and after reconnection it checks if the shell set it again automatically. One test case checks if the shell set the DB after manually reconnecting to the impala daemon by issuing the CONNECT command. The other test case checks if the shell set the DB after automatic reconnection due to cluster restart. I needed to backup the impala shell history file because I didn't want to pollute it by the test cases (just like the way it is done in tests/shell/test_shell_interactive.py). I created utility functions for this in tests/shell/util.py and now test_shell_interactive.py and the newly created test suite are using these utility functions. Change-Id: I40dfa00ba0314d356fe8617446f516505c925e5e --- M shell/impala_shell.py A tests/custom_cluster/test_shell_interactive_reconnect.py M tests/shell/test_shell_interactive.py M tests/shell/util.py 4 files changed, 100 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/8368/4 -- To view, visit http://gerrit.cloudera.org:8080/8368 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I40dfa00ba0314d356fe8617446f516505c925e5e Gerrit-Change-Number: 8368 Gerrit-PatchSet: 4 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option
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 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/8490/3/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/3/fe/src/test/java/org/apache/impala/service/JdbcTest.java@657 PS3, Line 657: } > Maybe you can be more explicit here about the expected end-state, e.g.: 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: 6 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer 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, 10 Nov 2017 15:58:30 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option
Hello Michael Ho, Thomas Tauber-Marshall, Laszlo Gaal, Gabor Kaszab, Attila Jeges, Tim Armstrong, Csaba Ringhofer, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8490 to look at the new patch set (#6). 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, 262 insertions(+), 32 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/8490/6 -- 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: 6 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer 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
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 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/8490/3/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:
http://gerrit.cloudera.org:8080/#/c/8490/3/be/src/service/client-request-state.cc@211
PS3, Line 211: SetResultSet({}, {});
: }
> single line
Done
http://gerrit.cloudera.org:8080/#/c/8490/3/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/8490/3/be/src/service/impala-server.cc@1119
PS3, Line 1119:discard_result(UnregisterQuery(query_id, false, &status));
: }
: // Reconfigure the poll period of session_timeout_thread_ if
necessary.
: UnregisterSessionTimeout(session_state->session_timeout);
: return Status::OK();
: }
:
> Can we call UnregisterSessionTimeout() instead here?
Nice catch! Yes, and we should! Done.
http://gerrit.cloudera.org:8080/#/c/8490/3/be/src/service/impala-server.cc@1236
PS3, Line 1236:
> nit: 4 character indents on line continuations
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: 5
Gerrit-Owner: Zoltan Borok-Nagy
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Csaba Ringhofer
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, 08 Nov 2017 13:25:05 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option
Hello Michael Ho, Thomas Tauber-Marshall, Laszlo Gaal, Gabor Kaszab, Attila Jeges, Tim Armstrong, Csaba Ringhofer, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8490 to look at the new patch set (#5). 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, 256 insertions(+), 32 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/8490/5 -- 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: 5 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer 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
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 4:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/8490/3/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:
http://gerrit.cloudera.org:8080/#/c/8490/3/be/src/service/client-request-state.cc@204
PS3, Line 204: if (iequals(key, "idle_session_timeout")) {
> Its unfortunate this is special cased here. Could you add a comment explain
Done
http://gerrit.cloudera.org:8080/#/c/8490/3/be/src/service/client-request-state.cc@205
PS3, Line 205: // IMPALA-2248: For some client, we need to enable to
set
> Could you extract the body of the 'if' to a new function in order to keep t
Done
http://gerrit.cloudera.org:8080/#/c/8490/3/be/src/service/client-request-state.cc@213
PS3, Line 213:
: } else {
> single line
Done
http://gerrit.cloudera.org:8080/#/c/8490/3/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/8490/3/be/src/service/impala-server.cc@189
PS3, Line 189: DEFINE_int32(idle_session_timeout, 0, "The time, in seconds,
that a session may be idle"
> Note how this interacts with the query option.
Done
http://gerrit.cloudera.org:8080/#/c/8490/3/common/thrift/ImpalaService.thrift
File common/thrift/ImpalaService.thrift:
http://gerrit.cloudera.org:8080/#/c/8490/3/common/thrift/ImpalaService.thrift@292
PS3, Line 292: // The time, in seconds, that a session may be idle for before
it is closed (and all
> Note how this interacts with the flag.
Done
http://gerrit.cloudera.org:8080/#/c/8490/3/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/3/fe/src/test/java/org/apache/impala/service/JdbcTest.java@594
PS3, Line 594:
> "0, 5, 10"'s and "timeout * 1.5"'s relation could be more explicit, for exa
Partly done.
I don't feel that the asList part is necessary, but I introduced a
timeoutTolerance variable.
http://gerrit.cloudera.org:8080/#/c/8490/3/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/3/fe/src/test/java/org/apache/impala/util/Metrics.java@26
PS3, Line 26: /**
> Could you please write a short comment to describe the purpose of this clas
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: 4
Gerrit-Owner: Zoltan Borok-Nagy
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Csaba Ringhofer
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, 08 Nov 2017 11:49:11 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option
Hello Michael Ho, Thomas Tauber-Marshall, Laszlo Gaal, Gabor Kaszab, Attila Jeges, Tim Armstrong, Csaba Ringhofer, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8490 to look at the new patch set (#4). 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, 253 insertions(+), 24 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/8490/4 -- 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: 4 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Laszlo Gaal Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-2181: Add query option levels for display
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8447 ) Change subject: IMPALA-2181: Add query option levels for display .. Patch Set 8: (1 comment) http://gerrit.cloudera.org:8080/#/c/8447/8/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/8447/8/be/src/service/impala-server.h@516 PS8, Line 516: /// Given a query option name this function gets the option level for it to use when : /// displaying from Impala shell and adds the level to the ConfigVariable parameter : /// in numeric format. For values see TQueryOptionLevel enum. The old comment is still here. -- To view, visit http://gerrit.cloudera.org:8080/8447 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1 Gerrit-Change-Number: 8447 Gerrit-PatchSet: 8 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Laszlo Gaal Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Wed, 08 Nov 2017 09:50:04 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2181: Add query option levels for display
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8447 ) Change subject: IMPALA-2181: Add query option levels for display .. Patch Set 7: (3 comments) http://gerrit.cloudera.org:8080/#/c/8447/7/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/8447/7/be/src/service/impala-server.h@516 PS7, Line 516: /// Given a query option name this function gets the option level for it to use when : /// displaying from Impala shell and adds the level to the ConfigVariable parameter : /// in numeric format. For values see TQueryOptionLevel enum. nit: For me, this comment is too complex and tells too much implementation details. How about something like this: This function sets the option level for parameter 'option' based on the mapping stored in 'query_option_levels_'. The option level is used by the Impala shell when it displays the options. http://gerrit.cloudera.org:8080/#/c/8447/7/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/8447/7/be/src/service/impala-server.cc@1228 PS7, Line 1228: const string& option_key Why do we pass option_key here? It is already there in config, isn't it? http://gerrit.cloudera.org:8080/#/c/8447/7/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/8447/7/shell/impala_shell.py@620 PS7, Line 620: self.imp_client.default_query_options, : self.set_query_options, : self.imp_client.query_option_levels These members are already available in _print_with_set through self. And as far as I can see the same members are passed on every callsites. I think it would be easier to read if _print_with_set didn't take these params. -- To view, visit http://gerrit.cloudera.org:8080/8447 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1 Gerrit-Change-Number: 8447 Gerrit-PatchSet: 7 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Laszlo Gaal Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Tue, 07 Nov 2017 16:25:06 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option
Hello Michael Ho, Laszlo Gaal, Gabor Kaszab, Attila Jeges, Tim Armstrong, Csaba Ringhofer, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8490 to look at the new patch set (#3). 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/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 10 files changed, 236 insertions(+), 23 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/8490/3 -- 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: 3 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Laszlo Gaal Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-6134: Update code base to use impala::ConditionVariable
Hello Philip Zeyliger, Tim Armstrong, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8428 to look at the new patch set (#3). Change subject: IMPALA-6134: Update code base to use impala::ConditionVariable .. IMPALA-6134: Update code base to use impala::ConditionVariable boost::condtion_variable supports thread interruption which has some overhead. In some places we already use impala::ConditionVariable which is a very thin layer around pthread, therefore it has less overhead. This commit substitues every boost::condition_variable in the codebase (except under kudu/) to impala::ConditionVariable. It also extends impala::ConditionVariable class to support waiting with a given timeout. The WaitFor function takes a duration as parameter. The WaitUntil function takes an absolute time as parameter. Change-Id: I3085c6dcb42350b61244df6e7f091a1e7db356c9 --- M be/src/catalog/catalog-server.cc M be/src/catalog/catalog-server.h M be/src/exec/plan-root-sink.cc M be/src/exec/plan-root-sink.h M be/src/rpc/thrift-server.cc M be/src/runtime/coordinator.cc M be/src/runtime/coordinator.h M be/src/runtime/data-stream-mgr.h M be/src/runtime/data-stream-recvr.cc M be/src/runtime/data-stream-sender.cc M be/src/runtime/disk-io-mgr-internal.h M be/src/runtime/disk-io-mgr-reader-context.cc M be/src/runtime/disk-io-mgr-scan-range.cc M be/src/runtime/disk-io-mgr-stress.cc M be/src/runtime/disk-io-mgr-stress.h M be/src/runtime/disk-io-mgr-test.cc M be/src/runtime/disk-io-mgr.cc M be/src/runtime/disk-io-mgr.h M be/src/runtime/fragment-instance-state.cc M be/src/runtime/fragment-instance-state.h M be/src/scheduling/admission-controller.cc M be/src/scheduling/admission-controller.h M be/src/service/client-request-state.cc M be/src/service/client-request-state.h M be/src/service/impala-server.cc M be/src/service/impala-server.h M be/src/statestore/statestore.h M be/src/util/blocking-queue.h M be/src/util/condition-variable.h M be/src/util/promise.h M be/src/util/thread-pool.h 31 files changed, 137 insertions(+), 122 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/28/8428/3 -- To view, visit http://gerrit.cloudera.org:8080/8428 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3085c6dcb42350b61244df6e7f091a1e7db356c9 Gerrit-Change-Number: 8428 Gerrit-PatchSet: 3 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-6134: Update code base to use impala::ConditionVariable
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8428 ) Change subject: IMPALA-6134: Update code base to use impala::ConditionVariable .. Patch Set 2: (4 comments) Thanks, updated the code and hopefully resolved the merge conflict. http://gerrit.cloudera.org:8080/#/c/8428/2/be/src/runtime/data-stream-recvr.cc File be/src/runtime/data-stream-recvr.cc: http://gerrit.cloudera.org:8080/#/c/8428/2/be/src/runtime/data-stream-recvr.cc@220 PS2, Line 220: unique_lock > This change is harmless but doesn't seem necessary. Oops, I rewrote it when I started to work on "the mutex should be released by the time we call notify". This shouldn't be in this commit. Done. http://gerrit.cloudera.org:8080/#/c/8428/2/be/src/runtime/fragment-instance-state.cc File be/src/runtime/fragment-instance-state.cc: http://gerrit.cloudera.org:8080/#/c/8428/2/be/src/runtime/fragment-instance-state.cc@332 PS2, Line 332: auto wait_duration = boost::posix_time::seconds(report_fragment_offset); > The use of "auto" here is probably marginal as far as our coding idioms go. Done http://gerrit.cloudera.org:8080/#/c/8428/2/be/src/runtime/fragment-instance-state.cc@337 PS2, Line 337: auto wait_duration = boost::posix_time::seconds(FLAGS_status_report_interval); > This is pre-existing but we should avoid shadowing variables. Done http://gerrit.cloudera.org:8080/#/c/8428/2/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/8428/2/be/src/service/impala-server.cc@1800 PS2, Line 1800: session_timeout_cv_.WaitFor(timeout_lock, wait_duration); > nit: could just be a one liner. Done -- To view, visit http://gerrit.cloudera.org:8080/8428 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3085c6dcb42350b61244df6e7f091a1e7db356c9 Gerrit-Change-Number: 8428 Gerrit-PatchSet: 2 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Tue, 07 Nov 2017 14:58:17 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option
Zoltan Borok-Nagy has uploaded a new patch set (#2). ( 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. I created an automated test case in JdbcTest.java. 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 10 files changed, 236 insertions(+), 23 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/8490/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: newpatchset Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e Gerrit-Change-Number: 8490 Gerrit-PatchSet: 2 Gerrit-Owner: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option
Zoltan Borok-Nagy has uploaded this change for review. ( 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. I created an automated test case in JdbcTest.java. 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 10 files changed, 238 insertions(+), 23 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/90/8490/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: newchange Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e Gerrit-Change-Number: 8490 Gerrit-PatchSet: 1 Gerrit-Owner: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-6134: Update code base to use impala::ConditionVariable
Hello Philip Zeyliger, Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8428 to look at the new patch set (#2). Change subject: IMPALA-6134: Update code base to use impala::ConditionVariable .. IMPALA-6134: Update code base to use impala::ConditionVariable boost::condtion_variable supports thread interruption which has some overhead. In some places we already use impala::ConditionVariable which is a very thin layer around pthread, therefore it has less overhead. This commit substitues every boost::condition_variable in the codebase (except under kudu/) to impala::ConditionVariable. It also extends impala::ConditionVariable class to support waiting with a given timeout. The WaitFor function takes a duration as parameter. The WaitUntil function takes an absolute time as parameter. Change-Id: I3085c6dcb42350b61244df6e7f091a1e7db356c9 --- M be/src/catalog/catalog-server.cc M be/src/catalog/catalog-server.h M be/src/exec/plan-root-sink.cc M be/src/exec/plan-root-sink.h M be/src/rpc/thrift-server.cc M be/src/runtime/coordinator.cc M be/src/runtime/coordinator.h M be/src/runtime/data-stream-mgr.h M be/src/runtime/data-stream-recvr.cc M be/src/runtime/data-stream-sender.cc M be/src/runtime/disk-io-mgr-internal.h M be/src/runtime/disk-io-mgr-reader-context.cc M be/src/runtime/disk-io-mgr-scan-range.cc M be/src/runtime/disk-io-mgr-stress.cc M be/src/runtime/disk-io-mgr-stress.h M be/src/runtime/disk-io-mgr-test.cc M be/src/runtime/disk-io-mgr.cc M be/src/runtime/disk-io-mgr.h M be/src/runtime/fragment-instance-state.cc M be/src/runtime/fragment-instance-state.h M be/src/scheduling/admission-controller.cc M be/src/scheduling/admission-controller.h M be/src/service/client-request-state.cc M be/src/service/client-request-state.h M be/src/service/impala-server.cc M be/src/service/impala-server.h M be/src/statestore/statestore.h M be/src/util/blocking-queue.h M be/src/util/condition-variable.h M be/src/util/promise.h M be/src/util/thread-pool.h 31 files changed, 139 insertions(+), 122 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/28/8428/2 -- To view, visit http://gerrit.cloudera.org:8080/8428 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3085c6dcb42350b61244df6e7f091a1e7db356c9 Gerrit-Change-Number: 8428 Gerrit-PatchSet: 2 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-2235: Fix current db when shell auto-reconnects
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8368 ) Change subject: IMPALA-2235: Fix current db when shell auto-reconnects .. Patch Set 3: (2 comments) I removed ClusterController and left test_breakpad.py untouched. The functionality available in CustomClusterTestSuite was enough for my test cases. http://gerrit.cloudera.org:8080/#/c/8368/2/tests/custom_cluster/test_shell_interactive_reconnect.py File tests/custom_cluster/test_shell_interactive_reconnect.py: http://gerrit.cloudera.org:8080/#/c/8368/2/tests/custom_cluster/test_shell_interactive_reconnect.py@25 PS2, Line 25: > Perhaps use an actual tempfile created with tempfile.[something] Done http://gerrit.cloudera.org:8080/#/c/8368/2/tests/shell/util.py File tests/shell/util.py: http://gerrit.cloudera.org:8080/#/c/8368/2/tests/shell/util.py@115 PS2, Line 115: move_shell_h > maybe move_shell_history()/restore_shell_history()? It's more verbose but I Done -- To view, visit http://gerrit.cloudera.org:8080/8368 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I40dfa00ba0314d356fe8617446f516505c925e5e Gerrit-Change-Number: 8368 Gerrit-PatchSet: 3 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Fri, 03 Nov 2017 15:03:27 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2235: Fix current db when shell auto-reconnects
Hello Philip Zeyliger, Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8368 to look at the new patch set (#3). Change subject: IMPALA-2235: Fix current db when shell auto-reconnects .. IMPALA-2235: Fix current db when shell auto-reconnects When precmd tested the connection it didn't validate that if we are using the previously selected DB. The _validate_database method is responsible for that, but it only appended the "use " command to the cmdqueue (command queue of Cmd class). But, at this point we might already have commands in the command queue that will run before the "use " command. Also, the command processed by precmd can entirely skip the cmdqueue, therefore it is not enough to insert the "use " command to the front of cmdqueue. We need to issue the USE command with the onecmd() method to execute it immediately. I extended the _validate_database method with an "immediately" flag. If this is true, _validate_database will use the onecmd() method. Otherwise, it will append the USE command to the cmdqueue to maintain the previous behaviour. I added a new automated test suite named test_shell_interactive_reconnect.py to the "custom cluster" tests. It sets the default database, and after reconnection it checks if the shell set it again automatically. One test case checks if the shell set the default db after manually reconnecting to the impala daemon by issuing the CONNECT command. The other test case checks if the shell set the default db after automatic reconnection due to cluster restart. I needed to backup the impala shell history file because I didn't want to pollute it by the test cases (just like the way it is done in tests/shell/test_shell_interactive.py). I created utility functions for this in tests/shell/util.py and now test_shell_interactive.py and the newly created test suite are using these utility functions. Change-Id: I40dfa00ba0314d356fe8617446f516505c925e5e --- M shell/impala_shell.py A tests/custom_cluster/test_shell_interactive_reconnect.py M tests/shell/test_shell_interactive.py M tests/shell/util.py 4 files changed, 102 insertions(+), 8 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/8368/3 -- To view, visit http://gerrit.cloudera.org:8080/8368 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I40dfa00ba0314d356fe8617446f516505c925e5e Gerrit-Change-Number: 8368 Gerrit-PatchSet: 3 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-6134: Update code base to use impala::ConditionVariable
Zoltan Borok-Nagy has posted comments on this change. (
http://gerrit.cloudera.org:8080/8428 )
Change subject: IMPALA-6134: Update code base to use impala::ConditionVariable
..
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8428/1/be/src/util/condition-variable.h
File be/src/util/condition-variable.h:
http://gerrit.cloudera.org:8080/#/c/8428/1/be/src/util/condition-variable.h@48
PS1, Line 48: const timespec* timeout
> I feel like this should be a const&, since it has to be non-null. That woul
OK, will do it.
http://gerrit.cloudera.org:8080/#/c/8428/1/be/src/util/condition-variable.h@65
PS1, Line 65: bool TimedWait(boost::unique_lock& lock,
> This looks like it only has one callsite. There's another callsite in Block
Now that I looked into the callsites more carefully, I realized that there are
two use cases for TimedWait. In Impala both of them are used. One use case is
when we wait until a point in time:
auto deadline = calculate_deadline();
while (! condition) {
if (!cv.TimedWait(lock, deadline) {
// deadline reached without notification
return Status::ERROR;
}
}
The other use case is waiting in a loop, and signaling some status periodically:
while (! condition) {
cv.TimedWait(lock, seconds(1));
if (condition) break;
else SendReport();
}
Both API can be expressed in terms of the other, however it is a bit awkward
(expressing deadline in terms of duration is more awkward, so if we have to
choose, I think we should go with supporting deadlines).
There are examples for both in the code base:
FragmentInstanceState::ReportProfileThread: wait duration in terms of wait
until deadline.
Promise::Get: wait until deadline in terms of wait duration.
Now I am on the side of support both use case with the two APIs, like
std::condition_variable has wait_for and wait_until member functions. Maybe we
can use these names also to be more explicit.
What do you think?
--
To view, visit http://gerrit.cloudera.org:8080/8428
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3085c6dcb42350b61244df6e7f091a1e7db356c9
Gerrit-Change-Number: 8428
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Zoltan Borok-Nagy
Gerrit-Comment-Date: Fri, 03 Nov 2017 10:05:26 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2235: Fix current db when shell auto-reconnects
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8368 ) Change subject: IMPALA-2235: Fix current db when shell auto-reconnects .. Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/8368/2/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/8368/2/shell/impala_shell.py@729 PS2, Line 729: def _validate_database(self, immediately=False): > Is there a case where you ever want immediately=True? i.e., could we get ri Yes, in precmd after reconnecting to the cluster (this commit). Every other _validate_database invocation use the default value which maintains the old behaviour of the method. http://gerrit.cloudera.org:8080/#/c/8368/2/tests/common/cluster_controller.py File tests/common/cluster_controller.py: http://gerrit.cloudera.org:8080/#/c/8368/2/tests/common/cluster_controller.py@34 PS2, Line 34: breakpad > Comment needs update. I guess now it's used for all custom cluster tests th Yeah, that's the idea. I'll remove the comment. http://gerrit.cloudera.org:8080/#/c/8368/2/tests/common/cluster_controller.py@42 PS2, Line 42: self.tmp_dir = tempfile.mkdtemp() > I'm thinking about whether this is factored in the best way and whether the Seems reasonable, will do it http://gerrit.cloudera.org:8080/#/c/8368/2/tests/custom_cluster/test_shell_interactive_reconnect.py File tests/custom_cluster/test_shell_interactive_reconnect.py: http://gerrit.cloudera.org:8080/#/c/8368/2/tests/custom_cluster/test_shell_interactive_reconnect.py@27 PS2, Line 27: class TestShellInteractiveReconnect(ClusterController): > Any reason not to add this simply to tests/shell/test_shell_interactive.py? Yes, it is in the custom cluster test suite because I am starting/restarting the cluster. http://gerrit.cloudera.org:8080/#/c/8368/2/tests/custom_cluster/test_shell_interactive_reconnect.py@40 PS2, Line 40: @pytest.mark.execute_serially > I think all CustomCluster tests run serially? This one seems like it could I guess you are right, my idea was what if another test case restarts the cluster during this test, so this impala shell automatically reconnects to the cluster and it makes this test fail or pass. What do you think? -- To view, visit http://gerrit.cloudera.org:8080/8368 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I40dfa00ba0314d356fe8617446f516505c925e5e Gerrit-Change-Number: 8368 Gerrit-PatchSet: 2 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Thu, 02 Nov 2017 19:52:28 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6134: Update code base to use impala::ConditionVariable
Zoltan Borok-Nagy has posted comments on this change. (
http://gerrit.cloudera.org:8080/8428 )
Change subject: IMPALA-6134: Update code base to use impala::ConditionVariable
..
Patch Set 1:
(3 comments)
> (1 comment)
>
> Most of this looks mechanical, and it looked fine. You changed some
> 1-line if's into 3-line ifs, which may be against house style.
>
> We don't seem to have any tests for condition-variable.h. How did
> you test this?
OK, I'll change the 3-line ifs back to 1-lines.
After the modifications, I ran the backend test suites on my local desktop.
Should I create tests for condition-variable.h? We don't seem to have unit
tests yet.
http://gerrit.cloudera.org:8080/#/c/8428/1/be/src/util/condition-variable.h
File be/src/util/condition-variable.h:
http://gerrit.cloudera.org:8080/#/c/8428/1/be/src/util/condition-variable.h@38
PS1, Line 38: void Wait(boost::unique_lock& lock) {
> I'm not yet familiar with this part of C++; why is 'inline' getting removed
About the 'struct' keyword: it is something that C requires, but in C++ it can
be omitted. If there were a function named 'timespec', then we would have to
use the 'struct' keyword in C++ as well to resolve ambiguity.
An example for that is 'stat' in POSIX, which is also a function and a struct.
http://gerrit.cloudera.org:8080/#/c/8428/1/be/src/util/condition-variable.h@38
PS1, Line 38: void Wait(boost::unique_lock& lock) {
> "inline" is implied by the function being defined within the class body: ht
Done
http://gerrit.cloudera.org:8080/#/c/8428/1/be/src/util/condition-variable.h@64
PS1, Line 64: template
> It looks like this is only ever used with microseconds - maybe we should na
IMHO it is easier for the callers to specify a time duration than forcing them
to calculate a time point in the future. At least I think this is how it is
used most of the time, so I would choose this direction.
We also have calls with milliseconds(thrift-server.cc, impala-server.cc), and
seconds (fragment-instance-state.cc) as well.
We can also narrow the scope by templates (though it is a bit ugly):
template
... subsecond_duration ...
Or, we can fix the callsites to always use microseconds.
What do you think of it?
--
To view, visit http://gerrit.cloudera.org:8080/8428
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3085c6dcb42350b61244df6e7f091a1e7db356c9
Gerrit-Change-Number: 8428
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Zoltan Borok-Nagy
Gerrit-Comment-Date: Thu, 02 Nov 2017 18:02:26 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6134: Update code base to use impala::ConditionVariable
Zoltan Borok-Nagy has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8428 Change subject: IMPALA-6134: Update code base to use impala::ConditionVariable .. IMPALA-6134: Update code base to use impala::ConditionVariable boost::condtion_variable supports thread interruption which has some overhead. In some places we already use impala::ConditionVariable which is a very thin layer around pthread, therefore it has less overhead. This commit substitues every boost::condtion_variable in the codebase (except under kudu/) to impala::ConditionVariable. It also extends impala::ConditionVariable class to support TimedWait based on boost::system_time and boost::time_duration. Change-Id: I3085c6dcb42350b61244df6e7f091a1e7db356c9 --- M be/src/catalog/catalog-server.cc M be/src/catalog/catalog-server.h M be/src/exec/plan-root-sink.cc M be/src/exec/plan-root-sink.h M be/src/rpc/thrift-server.cc M be/src/runtime/coordinator.cc M be/src/runtime/coordinator.h M be/src/runtime/data-stream-mgr.h M be/src/runtime/data-stream-recvr.cc M be/src/runtime/data-stream-sender.cc M be/src/runtime/disk-io-mgr-internal.h M be/src/runtime/disk-io-mgr-reader-context.cc M be/src/runtime/disk-io-mgr-scan-range.cc M be/src/runtime/disk-io-mgr-stress.cc M be/src/runtime/disk-io-mgr-stress.h M be/src/runtime/disk-io-mgr-test.cc M be/src/runtime/disk-io-mgr.cc M be/src/runtime/disk-io-mgr.h M be/src/runtime/fragment-instance-state.cc M be/src/runtime/fragment-instance-state.h M be/src/scheduling/admission-controller.cc M be/src/scheduling/admission-controller.h M be/src/service/client-request-state.cc M be/src/service/client-request-state.h M be/src/service/impala-server.cc M be/src/service/impala-server.h M be/src/statestore/statestore.h M be/src/util/blocking-queue.h M be/src/util/condition-variable.h M be/src/util/promise.h M be/src/util/thread-pool.h 31 files changed, 134 insertions(+), 114 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/28/8428/1 -- To view, visit http://gerrit.cloudera.org:8080/8428 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I3085c6dcb42350b61244df6e7f091a1e7db356c9 Gerrit-Change-Number: 8428 Gerrit-PatchSet: 1 Gerrit-Owner: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-1291: Parquet read fails if io buffer size is less than the footer size
Hello Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8366 to look at the new patch set (#3). Change subject: IMPALA-1291: Parquet read fails if io buffer size is less than the footer size .. IMPALA-1291: Parquet read fails if io buffer size is less than the footer size This commit introduces the compile-time constant READ_SIZE_MIN_VALUE in the newly created common/global-flags.h A static_assert checks if READ_SIZE_MIN_VALUE is greater than or equal to HdfsParquetScanner::FOOTER_SIZE. Also, moved the definition of read_size flag to global-flags.cc, and declared it in global-flags.h. Runtime checks test if read_size is greater than or eaqual to READ_SIZE_MIN_VALUE. If not, the process is terminated. Change-Id: Ib7364147e7daf5380f11368871f8479646b448da --- M be/src/common/global-flags.cc A be/src/common/global-flags.h M be/src/common/init.cc M be/src/exec/hdfs-parquet-scanner.h M be/src/runtime/disk-io-mgr.cc M be/src/util/backend-gflag-util.cc 6 files changed, 57 insertions(+), 9 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/66/8366/3 -- To view, visit http://gerrit.cloudera.org:8080/8366 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib7364147e7daf5380f11368871f8479646b448da Gerrit-Change-Number: 8366 Gerrit-PatchSet: 3 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-2235: Fix current db when shell auto-reconnects
Zoltan Borok-Nagy has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8368 Change subject: IMPALA-2235: Fix current db when shell auto-reconnects .. IMPALA-2235: Fix current db when shell auto-reconnects When precmd tested the connection it didn't validate that if we are using the previously selected DB. The _validate_database method is responsible for that, but it only appended the "use " command to the cmdqueue (command queue of Cmd class). But, at this point we might already have commands in the command queue that will run before the "use " command. Also, the command processed by precmd can entirely skip the cmdqueue, therefore it is not enough to insert the "use " command to the front of cmdqueue. We need to issue the USE command with the onecmd() method to execute it immediately. I extended the _validate_database method with an "immediately" flag. If this is true, _validate_database will use the onecmd() method. Otherwise, it will append the USE command to the cmdqueue to maintain the previous behaviour. I added a new automated test suite named test_shell_interactive_reconnect.py to the "custom cluster" tests. It sets the default database, and after reconnection it checks if the shell set it again automatically. One test case checks if the shell set the default db after manually reconnecting to the impala daemon by issuing the CONNECT command. The other test case checks if the shell set the default db after automatic reconnection due to cluster restart. I needed to start/restart the cluster in these tests. That functionality was already implemented in class TestBreakpadBase, but I didn't want the new tests to depend on code from an other test suite, therefore I moved TestBreakpadBase class to tests/common/cluster_controller.py and renamed it to ClusterController. I also needed to backup the impala shell history file because I didn't want to pollute it by the test cases (just like the way it is done in tests/shell/test_shell_interactive.py). I created utility functions for this in tests/shell/util.py and now test_shell_interactive.py and the newly created test suite are using these utility functions. Change-Id: I40dfa00ba0314d356fe8617446f516505c925e5e --- M shell/impala_shell.py A tests/common/cluster_controller.py M tests/custom_cluster/test_breakpad.py A tests/custom_cluster/test_shell_interactive_reconnect.py M tests/shell/test_shell_interactive.py M tests/shell/util.py 6 files changed, 243 insertions(+), 121 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/8368/2 -- To view, visit http://gerrit.cloudera.org:8080/8368 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I40dfa00ba0314d356fe8617446f516505c925e5e Gerrit-Change-Number: 8368 Gerrit-PatchSet: 2 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-1291: Parquet read fails if io buffer size is less than the footer size
Zoltan Borok-Nagy has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8366 Change subject: IMPALA-1291: Parquet read fails if io buffer size is less than the footer size .. IMPALA-1291: Parquet read fails if io buffer size is less than the footer size This commit introduces the compile-time constant READ_SIZE_MIN_VALUE in the newly created common/global-flags.h A static_assert checks if READ_SIZE_MIN_VALUE is greater than or equal to HdfsParquetScanner::FOOTER_SIZE. Also, moved the definition of read_size flag to global-flags.cc, and declared it in global-flags.h. Runtime checks test if read_size is greater than or eaqual to READ_SIZE_MIN_VALUE. If not, the process is terminated. Change-Id: Ib7364147e7daf5380f11368871f8479646b448da --- M be/src/common/global-flags.cc A be/src/common/global-flags.h M be/src/common/init.cc M be/src/exec/hdfs-parquet-scanner.h M be/src/runtime/disk-io-mgr.cc M be/src/util/backend-gflag-util.cc 6 files changed, 57 insertions(+), 9 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/66/8366/2 -- To view, visit http://gerrit.cloudera.org:8080/8366 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ib7364147e7daf5380f11368871f8479646b448da Gerrit-Change-Number: 8366 Gerrit-PatchSet: 2 Gerrit-Owner: Zoltan Borok-Nagy Gerrit-Reviewer: Tim Armstrong
