You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org> on 2017/10/26 11:57:18 UTC
[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 <db>" 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 <db>" command.
Also, the command processed by precmd can entirely skip the cmdqueue,
therefore it is not enough to insert the "use <db>" 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 <bo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-2235: Fix current db when shell auto-reconnects
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Philip Zeyliger 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: Code-Review+1
(1 comment)
Thanks for the commit message. Please see my comment about the sleep you added. If you think it won't work, I'm fine with this as is.
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: time.sleep(1)
Would "p.send_cmd("show tables"); p.get_result()" make this stable, without doing a sleep?
I always worry a bit about sleeps in tests, as they have a tendency to show up at annoying times as problems.
--
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 <bo...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 14 Nov 2017 16:58:04 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2235: Fix current db when shell auto-reconnects
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 6: Verified+1
--
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: 6
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 15 Nov 2017 22:42:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2235: Fix current db when shell auto-reconnects
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Philip Zeyliger 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: Code-Review+1
Thanks. Tim: I'm good with this; you'll have to +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: comment
Gerrit-Change-Id: I40dfa00ba0314d356fe8617446f516505c925e5e
Gerrit-Change-Number: 8368
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 15 Nov 2017 00:12:38 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2235: Fix current db when shell auto-reconnects
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 6:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1477/
--
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: 6
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 15 Nov 2017 19:24:52 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2235: Fix current db when shell auto-reconnects
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( 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
The ImpalaShell didn't issue the 'USE <current-db>' 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 <db>" 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 <db>" 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
Reviewed-on: http://gerrit.cloudera.org:8080/8368
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Impala Public Jenkins
---
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(-)
Approvals:
Tim Armstrong: Looks good to me, approved
Impala Public Jenkins: Verified
--
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: merged
Gerrit-Change-Id: I40dfa00ba0314d356fe8617446f516505c925e5e
Gerrit-Change-Number: 8368
Gerrit-PatchSet: 7
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-2235: Fix current db when shell auto-reconnects
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 14 Nov 2017 23:27:55 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2235: Fix current db when shell auto-reconnects
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 03 Nov 2017 15:03:27 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2235: Fix current db when shell auto-reconnects
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong 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:
Phil, did you want to take another look at this?
--
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 <bo...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 10 Nov 2017 01:08:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2235: Fix current db when shell auto-reconnects
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Philip Zeyliger 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:
(7 comments)
Thanks for tackling this! As a user, I hate that this was broken.
I think your changes to impala_shell.py look good. I'm a bit more wary of having another superclass for tests with helper functions.
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 rid of the two paths here and converge to just one?
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@33
PS2, Line 33: class ClusterController(CustomClusterTestSuite):
I'm generally not fond of the inheritance and multiple-inheritance in these tests. Is this actually distinct from CustomClusterTestSuite?
http://gerrit.cloudera.org:8080/#/c/8368/2/tests/common/cluster_controller.py@64
PS2, Line 64: def start_cluster_with_args(self, **kwargs):
This could go straight into the super class. It's tightly coupled with _start_impala_cluster.
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: TMP_HISTORY_FILE = os.path.expanduser("~/.impalahistorytmp")
Perhaps use an actual tempfile created with tempfile.[something]
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?
I think because you need CustomCluster to do the restart? If so, please add a comment about this.
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 be parallel.
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@116
PS2, Line 116: """ Moves history file to given filepath """
The underlying bug here is that the shell doesn't have a historypath option, but I think this is fine...
--
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 <bo...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Oct 2017 17:58:48 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2235: Fix current db when shell auto-reconnects
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong 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: Code-Review+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: comment
Gerrit-Change-Id: I40dfa00ba0314d356fe8617446f516505c925e5e
Gerrit-Change-Number: 8368
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 15 Nov 2017 19:24:34 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2235: Fix current db when shell auto-reconnects
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong 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)
Thanks for doing this refactoring, I had some questions about whether we could improve it further, but I think the direction looks good.
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@33
PS2, Line 33: class ClusterController(CustomClusterTestSuite):
Woh, nice.
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 that kill processes and restart the cluster?
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 additional layer of inheritance is needed. I think a lot of the below utility functions for starting the cluster and killing processes could be moved into CustomClusterTestSuite.
Other aspects seem specific to the breakpad tests - disabling core dumps, the temporary directory manipulation. Maybe it makes sense to have those in a Breakpad test base class.
What do you think?
http://gerrit.cloudera.org:8080/#/c/8368/2/tests/common/cluster_controller.py@85
PS2, Line 85: def kill_processes(self, processes, signal):
kill_processes() and wait_for_all_processes_dead() I think could just be standalone utility functions - they aren't specific to cluster tests.
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_history
maybe move_shell_history()/restore_shell_history()? It's more verbose but I think is easier to understand at the callsites.
--
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 <bo...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Oct 2017 17:58:15 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2235: Fix current db when shell auto-reconnects
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
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 <bo...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 02 Nov 2017 19:52:28 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2235: Fix current db when shell auto-reconnects
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
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 <current-db>' 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 <db>" 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 <db>" 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 <bo...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-2235: Fix current db when shell auto-reconnects
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Philip Zeyliger 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: Code-Review+1
(3 comments)
Thanks for the updates. This looks fine to me, with 2 minor issues:
* Please re-work the first line of the commit message; it doesn't make sense to me. (Or maybe I'm missing something, in which case just let me know.)
* If you're using NamedTemporaryFile to just get a filename, I suggested a shorter solution. There are two cases in this diff where you could take advantage of that.
I'm fine with both of those changes going in without further review; they're very minor.
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: When precmd tested the connection it didn't validate that if we are
This sentence didn't parse for me. Specifically, I'm not sure what "that" in "it didn't validate that" is referring to.
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: tempfile = NamedTemporaryFile(delete=False)
: tempfile.close()
: cls.tempfile_name = tempfile.name
Minor: I think this is just:
cls.tempfile_name = tempfile.mktemp()
Or did you use NamedTemporaryFile to create the file too, and that's important somehow?
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: """ Moves back history file from given filepath """
mention that this is a no-op if filepath doesn't exist.
--
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 <bo...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 13 Nov 2017 19:14:30 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2235: Fix current db when shell auto-reconnects
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
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 <current-db>' 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 <db>" 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 <db>" 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 <bo...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-2235: Fix current db when shell auto-reconnects
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
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 <db>" 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 <db>" command.
Also, the command processed by precmd can entirely skip the cmdqueue,
therefore it is not enough to insert the "use <db>" 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 <bo...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-2235: Fix current db when shell auto-reconnects
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong 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: Code-Review+1
Will give phil another chance to look
--
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 <bo...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Fri, 03 Nov 2017 18:21:30 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2235: Fix current db when shell auto-reconnects
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
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 <current-db>' 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 <bo...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 14 Nov 2017 15:34:03 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2235: Fix current db when shell auto-reconnects
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong 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 6: Code-Review+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: comment
Gerrit-Change-Id: I40dfa00ba0314d356fe8617446f516505c925e5e
Gerrit-Change-Number: 8368
Gerrit-PatchSet: 6
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 15 Nov 2017 19:24:43 +0000
Gerrit-HasComments: No