You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "David Knupp (Code Review)" <ge...@cloudera.org> on 2016/11/18 04:45:19 UTC

[Impala-ASF-CR] IMPALA-4510: Selectively filter args for metric verification tests 

David Knupp has uploaded a new change for review.

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

Change subject: IMPALA-4510: Selectively filter args for metric verification tests
......................................................................

IMPALA-4510: Selectively filter args for metric verification tests

run-tests.py is a wrapper around impala-pytest. It abstracts away
the need to invoke separate individual runs for serial tests,
parallel tests, and metric verification tests.

Because it's possible for a user to specify certain test suites,
or even specific tests, on the command line when calling
run-tests.py, it had been necessary to override the command line
args when it came time to run the metric verification tests --
otherwise those other tests/suites would be rerun. Before this
patch, we had simply been stripping away all command line args.

However, that blanket approach causes problems when running tests
against a remote cluster, because we need to retain those command
line args that pertain to the remote cluster.

This patch selectively prunes unwanted command line args for the
last metric verification test stage, keeping the ones that we
need, and also adds extensive documentation for explaining why we
have to go through this fairly odd and elaborate step.

Change-Id: I069172f44c1307d55f85779cdb01fecc0ba1799e
---
M tests/run-tests.py
1 file changed, 55 insertions(+), 11 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/5135/1
-- 
To view, visit http://gerrit.cloudera.org:8080/5135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I069172f44c1307d55f85779cdb01fecc0ba1799e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: David Knupp <dk...@cloudera.com>

[Impala-ASF-CR] IMPALA-4510: Selectively filter args for metric verification tests

Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has submitted this change and it was merged.

Change subject: IMPALA-4510: Selectively filter args for metric verification tests
......................................................................


IMPALA-4510: Selectively filter args for metric verification tests

run-tests.py is a wrapper around impala-py.test. It abstracts away
the need to invoke separate runs for serial tests, parallel tests,
and metric verification tests.

Because it's possible for a user to specify certain test suites,
or even specific tests, on the command line when calling
run-tests.py, it had been necessary to override the command line
args when it came time to run the metric verification tests --
otherwise those other tests/suites would be rerun. Before this
patch, we had simply been stripping away all command line args.

However, that blanket approach causes problems when running tests
against a remote cluster, because we need to retain those command
line args that pertain to the remote cluster.

This patch selectively prunes unwanted command line args for the
last metric verification test stage, keeping the ones that we
need, and also adds extensive documentation for explaining why we
have to go through this fairly odd and elaborate step.

This patch was tested by running a sample test suite locally,
and against a remote cluster. Previously, the metric verification
stage had been failing for remote cluster tests (since they were
defaulting to localhost for services that were only available
remotely.) With the patch, the remote verfification tests were
passing.

Also, while I'm here, add a small change that exits immediately
if the user calls for --help. Before this, we actually still ran
the tests.

Change-Id: I069172f44c1307d55f85779cdb01fecc0ba1799e
Reviewed-on: http://gerrit.cloudera.org:8080/5135
Reviewed-by: Michael Brown <mi...@cloudera.com>
Reviewed-by: Ishaan Joshi <is...@cloudera.com>
Tested-by: Internal Jenkins
---
M tests/run-tests.py
1 file changed, 77 insertions(+), 14 deletions(-)

Approvals:
  Ishaan Joshi: Looks good to me, approved
  Michael Brown: Looks good to me, but someone else must approve
  Internal Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I069172f44c1307d55f85779cdb01fecc0ba1799e
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Ishaan Joshi <is...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>

[Impala-ASF-CR] IMPALA-4510: Selectively filter args for metric verification tests

Posted by "David Knupp (Code Review)" <ge...@cloudera.org>.
Hello Michael Brown,

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

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

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

Change subject: IMPALA-4510: Selectively filter args for metric verification tests
......................................................................

IMPALA-4510: Selectively filter args for metric verification tests

run-tests.py is a wrapper around impala-py.test. It abstracts away
the need to invoke separate runs for serial tests, parallel tests,
and metric verification tests.

Because it's possible for a user to specify certain test suites,
or even specific tests, on the command line when calling
run-tests.py, it had been necessary to override the command line
args when it came time to run the metric verification tests --
otherwise those other tests/suites would be rerun. Before this
patch, we had simply been stripping away all command line args.

However, that blanket approach causes problems when running tests
against a remote cluster, because we need to retain those command
line args that pertain to the remote cluster.

This patch selectively prunes unwanted command line args for the
last metric verification test stage, keeping the ones that we
need, and also adds extensive documentation for explaining why we
have to go through this fairly odd and elaborate step.

This patch was tested by running a sample test suite locally,
and against a remote cluster. Previously, the metric verification
stage had been failing for remote cluster tests (since they were
defaulting to localhost for services that were only available
remotely.) With the patch, the remote verfification tests were
passing.

Also, while I'm here, add a small change that exits immediately
if the user calls for --help. Before this, we actually still ran
the tests.

Change-Id: I069172f44c1307d55f85779cdb01fecc0ba1799e
---
M tests/run-tests.py
1 file changed, 77 insertions(+), 14 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/5135/5
-- 
To view, visit http://gerrit.cloudera.org:8080/5135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I069172f44c1307d55f85779cdb01fecc0ba1799e
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>

[Impala-ASF-CR] IMPALA-4510: Selectively filter args for metric verification tests

Posted by "David Knupp (Code Review)" <ge...@cloudera.org>.
David Knupp has posted comments on this change.

Change subject: IMPALA-4510: Selectively filter args for metric verification tests
......................................................................


Patch Set 4:

While I'm in this file, make a quick fix to not run tests if the user just passes in --help.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I069172f44c1307d55f85779cdb01fecc0ba1799e
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4510: Selectively filter args for metric verification tests

Posted by "David Knupp (Code Review)" <ge...@cloudera.org>.
David Knupp has posted comments on this change.

Change subject: IMPALA-4510: Selectively filter args for metric verification tests
......................................................................


Patch Set 2:

(7 comments)

Thanks for the review.

http://gerrit.cloudera.org:8080/#/c/5135/2//COMMIT_MSG
Commit Message:

PS2, Line 9: impala-pytest
> impala-py.test
Done


PS2, Line 10:  separate individual
> You can just use one of these two words. :)
Done


Line 28: 
> Please mention the testing you performed for this patch.
Done


http://gerrit.cloudera.org:8080/#/c/5135/2/tests/run-tests.py
File tests/run-tests.py:

Line 73:   Modify and return the command line arguments that will be passed to pytest.
> Can you also talk about what log_base_name and valid_dirs need to be for a 
Done


PS2, Line 107:     # We need to account for the fact that '--foo bar' and '--foo=bar' might
             :     # be supplied by the user.
             :     raw_args = itertools.chain(*[arg.split('=') for arg in sys.argv[1:]])
> Crafty. It would help a reader to show a simple before/after example of wha
Done


PS2, Line 110:     filtered_args = []
> The name here is strange to me. These args aren't being filtered, but in fa
Done


PS2, Line 113:       try:
             :         pytest.config.getvalue(arg.strip('-'))  # Raises ValueError if invalid arg
             :         filtered_args += [arg, str(raw_args.next())]
             :       except ValueError:
             :         continue
> This is related to the L75 docstring comment above: it might make more sens
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I069172f44c1307d55f85779cdb01fecc0ba1799e
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4510: Selectively filter args for metric verification tests

Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has posted comments on this change.

Change subject: IMPALA-4510: Selectively filter args for metric verification tests
......................................................................


Patch Set 5: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I069172f44c1307d55f85779cdb01fecc0ba1799e
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Ishaan Joshi <is...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4510: Selectively filter args for metric verification tests

Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
Michael Brown has posted comments on this change.

Change subject: IMPALA-4510: Selectively filter args for metric verification tests
......................................................................


Patch Set 4: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I069172f44c1307d55f85779cdb01fecc0ba1799e
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4510: Selectively filter args for metric verification tests

Posted by "Ishaan Joshi (Code Review)" <ge...@cloudera.org>.
Ishaan Joshi has posted comments on this change.

Change subject: IMPALA-4510: Selectively filter args for metric verification tests
......................................................................


Patch Set 5: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I069172f44c1307d55f85779cdb01fecc0ba1799e
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Ishaan Joshi <is...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4510: Selectively filter args for metric verification tests

Posted by "David Knupp (Code Review)" <ge...@cloudera.org>.
David Knupp has uploaded a new patch set (#3).

Change subject: IMPALA-4510: Selectively filter args for metric verification tests
......................................................................

IMPALA-4510: Selectively filter args for metric verification tests

run-tests.py is a wrapper around impala-py.test. It abstracts away
the need to invoke separate runs for serial tests, parallel tests,
and metric verification tests.

Because it's possible for a user to specify certain test suites,
or even specific tests, on the command line when calling
run-tests.py, it had been necessary to override the command line
args when it came time to run the metric verification tests --
otherwise those other tests/suites would be rerun. Before this
patch, we had simply been stripping away all command line args.

However, that blanket approach causes problems when running tests
against a remote cluster, because we need to retain those command
line args that pertain to the remote cluster.

This patch selectively prunes unwanted command line args for the
last metric verification test stage, keeping the ones that we
need, and also adds extensive documentation for explaining why we
have to go through this fairly odd and elaborate step.

This patch was tested by running a sample test suite locally,
and against a remote cluster. Previously, the metric verification
stage had been failing for remote cluster tests (since they were
defaulting to localhost for services that were only available
remotely.) With the patch, the remote verfification tests were
passing.

Change-Id: I069172f44c1307d55f85779cdb01fecc0ba1799e
---
M tests/run-tests.py
1 file changed, 69 insertions(+), 11 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/5135/3
-- 
To view, visit http://gerrit.cloudera.org:8080/5135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I069172f44c1307d55f85779cdb01fecc0ba1799e
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>

[Impala-ASF-CR] IMPALA-4510: Selectively filter args for metric verification tests

Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
Michael Brown has posted comments on this change.

Change subject: IMPALA-4510: Selectively filter args for metric verification tests
......................................................................


Patch Set 2:

(8 comments)

I understand the problem, and I think I see what you're doing to fix it, but it might make things a tad clearer if you address my comments below. Thanks!

http://gerrit.cloudera.org:8080/#/c/5135/2//COMMIT_MSG
Commit Message:

PS2, Line 9: impala-pytest
impala-py.test


PS2, Line 10:  separate individual
You can just use one of these two words. :)


Line 28: 
Please mention the testing you performed for this patch.


http://gerrit.cloudera.org:8080/#/c/5135/2/tests/run-tests.py
File tests/run-tests.py:

Line 73:   Modify and return the command line arguments that will be passed to pytest.
Can you also talk about what log_base_name and valid_dirs need to be for a caller?


Line 75:   The raw command line arguments need to be modified because of the way our
Should all of this be in the docstring? It seems like you could instead inline some of this information in relevant places below.


PS2, Line 107:     # We need to account for the fact that '--foo bar' and '--foo=bar' might
             :     # be supplied by the user.
             :     raw_args = itertools.chain(*[arg.split('=') for arg in sys.argv[1:]])
Crafty. It would help a reader to show a simple before/after example of what sys.argv[1:] starts as, and then what the resulting raw_args is.


PS2, Line 110:     filtered_args = []
The name here is strange to me. These args aren't being filtered, but in fact they are getting through your filter, right? What's being filtered actually the options that raise ValueError below, right?


PS2, Line 113:       try:
             :         pytest.config.getvalue(arg.strip('-'))  # Raises ValueError if invalid arg
             :         filtered_args += [arg, str(raw_args.next())]
             :       except ValueError:
             :         continue
This is related to the L75 docstring comment above: it might make more sense for a reader to see here that you're saying "If this is a pytest config argh, it should be carried along; otherwise it should be filtered" .... or something similar.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I069172f44c1307d55f85779cdb01fecc0ba1799e
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4510: Selectively filter args for metric verification tests

Posted by "David Knupp (Code Review)" <ge...@cloudera.org>.
David Knupp has uploaded a new patch set (#4).

Change subject: IMPALA-4510: Selectively filter args for metric verification tests
......................................................................

IMPALA-4510: Selectively filter args for metric verification tests

run-tests.py is a wrapper around impala-py.test. It abstracts away
the need to invoke separate runs for serial tests, parallel tests,
and metric verification tests.

Because it's possible for a user to specify certain test suites,
or even specific tests, on the command line when calling
run-tests.py, it had been necessary to override the command line
args when it came time to run the metric verification tests --
otherwise those other tests/suites would be rerun. Before this
patch, we had simply been stripping away all command line args.

However, that blanket approach causes problems when running tests
against a remote cluster, because we need to retain those command
line args that pertain to the remote cluster.

This patch selectively prunes unwanted command line args for the
last metric verification test stage, keeping the ones that we
need, and also adds extensive documentation for explaining why we
have to go through this fairly odd and elaborate step.

This patch was tested by running a sample test suite locally,
and against a remote cluster. Previously, the metric verification
stage had been failing for remote cluster tests (since they were
defaulting to localhost for services that were only available
remotely.) With the patch, the remote verfification tests were
passing.

Change-Id: I069172f44c1307d55f85779cdb01fecc0ba1799e
---
M tests/run-tests.py
1 file changed, 69 insertions(+), 11 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/5135/4
-- 
To view, visit http://gerrit.cloudera.org:8080/5135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I069172f44c1307d55f85779cdb01fecc0ba1799e
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>

[Impala-ASF-CR] IMPALA-4510: Selectively filter args for metric verification tests

Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
Michael Brown has posted comments on this change.

Change subject: IMPALA-4510: Selectively filter args for metric verification tests
......................................................................


Patch Set 5: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I069172f44c1307d55f85779cdb01fecc0ba1799e
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4510: Selectively filter args for metric verification tests

Posted by "David Knupp (Code Review)" <ge...@cloudera.org>.
David Knupp has uploaded a new patch set (#2).

Change subject: IMPALA-4510: Selectively filter args for metric verification tests
......................................................................

IMPALA-4510: Selectively filter args for metric verification tests

run-tests.py is a wrapper around impala-pytest. It abstracts away
the need to invoke separate individual runs for serial tests,
parallel tests, and metric verification tests.

Because it's possible for a user to specify certain test suites,
or even specific tests, on the command line when calling
run-tests.py, it had been necessary to override the command line
args when it came time to run the metric verification tests --
otherwise those other tests/suites would be rerun. Before this
patch, we had simply been stripping away all command line args.

However, that blanket approach causes problems when running tests
against a remote cluster, because we need to retain those command
line args that pertain to the remote cluster.

This patch selectively prunes unwanted command line args for the
last metric verification test stage, keeping the ones that we
need, and also adds extensive documentation for explaining why we
have to go through this fairly odd and elaborate step.

Change-Id: I069172f44c1307d55f85779cdb01fecc0ba1799e
---
M tests/run-tests.py
1 file changed, 55 insertions(+), 11 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/5135/2
-- 
To view, visit http://gerrit.cloudera.org:8080/5135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I069172f44c1307d55f85779cdb01fecc0ba1799e
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: David Knupp <dk...@cloudera.com>