David Knupp has posted comments on this change. ( http://gerrit.cloudera.org:8080/9770 )
Change subject: IMPALA-5721,IMPALA-6717,IMPALA-6738: improve stress test binary search ...................................................................... Patch Set 7: (3 comments) I'm probably going to do this in passes. This initial small set of comments is only for the StressArgCurator class. http://gerrit.cloudera.org:8080/#/c/9770/7/tests/stress/concurrent_select.py File tests/stress/concurrent_select.py: http://gerrit.cloudera.org:8080/#/c/9770/7/tests/stress/concurrent_select.py@113 PS7, Line 113: class StressArgCurator(object): Nit: Is "curate" really the right word here? I think of the act of curating as filtering -- deciding what stays and what goes. This class does conversion or formatting more than curation. http://gerrit.cloudera.org:8080/#/c/9770/7/tests/stress/concurrent_select.py@141 PS7, Line 141: if hasattr(self, curate_method_name): So, for what it's worth, rather that having these magic _curate_ methods, the way I would have done this would have been to design a class that simply "wrapped" the args you got from parse_args(). Something like: class StressArgs(object): def __init__(self, raw_args): self.raw_args = raw_args @property def common_query_options(self): # do stuff to return formatted common_query_options @property def runtime_info_path(self): # do stuff to return formatted runtime_info_path def __getattr__(self, attr): """This should return other attrs not explicitly re-cast as properties.""" return getattr(self.raw_args, attr) Then you can just use it like: stress_args = StressArgs(parser.parse_args()) stress_args.common_query_options # Defers to common_query_options property stress_args.runtime_info_path # Defers to runtime_info_path property stress_args.results_dir # Returns stress_args.raw_args.results_dir etc... I'll leave it up to you if you'd care to change this. http://gerrit.cloudera.org:8080/#/c/9770/7/tests/stress/concurrent_select.py@145 PS7, Line 145: curated_args[k] = v Nit: Might want to explicitly mention in either the class or function docstring that only a portion of the args require conversion. Some are kept as is. That wasn't immediately clear to me. -- To view, visit http://gerrit.cloudera.org:8080/9770 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I33d036ec93df3016cd4703205078dbdba0168acb Gerrit-Change-Number: 9770 Gerrit-PatchSet: 7 Gerrit-Owner: Michael Brown <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-Reviewer: Nithya Janarthanan <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 27 Mar 2018 20:58:05 +0000 Gerrit-HasComments: Yes
