impala-reviews mailing list archives

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

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

Patch Set 2:


Thanks for the review.
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.
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 

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

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

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 sens

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: David Knupp <>
Gerrit-Reviewer: Michael Brown <>
Gerrit-HasComments: Yes

View raw message