Michael Brown 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: (7 comments) http://gerrit.cloudera.org:8080/#/c/9770/1/tests/stress/concurrent_select.py File tests/stress/concurrent_select.py: http://gerrit.cloudera.org:8080/#/c/9770/1/tests/stress/concurrent_select.py@174 PS1, Line 174: LOG.warn( > docstring Done http://gerrit.cloudera.org:8080/#/c/9770/1/tests/stress/concurrent_select.py@176 PS1, Line 176: "The stress test algorithm needs control of this option.".format( : query_option=query_option, opt=args_common_quer > Ideally we should also prefix query.logical_id. QueryReport doesn't own tha Done http://gerrit.cloudera.org:8080/#/c/9770/1/tests/stress/concurrent_select.py@708 PS1, Line 708: ry") > Stale var of some older designs. Doesn't seem needed. Nevermind, actually used now. http://gerrit.cloudera.org:8080/#/c/9770/1/tests/stress/concurrent_select.py@1475 PS1, Line 1475: if old_required_mem_mb_with_spilling: : mem_limit = old_required_mem_mb_with_spilling : old_required_mem_mb_with_spilling = None : else: > Getting a little untenable. Maybe instead just save what we want instead of Eh, withdrawn. The save list is still longer. 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 Done 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, t Nice idea. I tend to think __getattr__ is just as magical, and I tend to avoid using it, but I think the more magical one is __getattribute__. But you raise a good point and using __getattr__ means I don't need to do the dispatch. Since you said you'll be reviewing in batches, I'm going to think on this one a while before addressing it one way or another. 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 docst Done. I tried to convey this L116-122, but it wasn't enough. I've tried again. Please let me know if it can be made clearer. -- 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 <mi...@cloudera.com> Gerrit-Reviewer: David Knupp <dkn...@cloudera.com> Gerrit-Reviewer: Michael Brown <mi...@cloudera.com> Gerrit-Reviewer: Nithya Janarthanan <njanartha...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Tue, 27 Mar 2018 21:28:29 +0000 Gerrit-HasComments: Yes
