impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Brown (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4510: Selectively filter args for metric verification tests
Date Mon, 21 Nov 2016 19:26:14 GMT
Michael Brown has posted comments on this change.

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

Patch Set 2:


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!
Commit Message:

PS2, Line 9: impala-pytest

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.
File tests/

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
             :         filtered_args += [arg, str(]
             :       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
To unsubscribe, visit

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

View raw message