Michael Brown has posted comments on this change. Change subject: Preview: IMPALA-4467: Add support for CRUD operations in stress test ......................................................................
Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/5093/1/tests/stress/concurrent_select.py File tests/stress/concurrent_select.py: PS1, Line 695: self.MIN_INT = 20 You've defined self.MIN_INT twice. PS1, Line 698: def sql(self): This whole set of methods could use docstrings to explain to someone roughly how the interface works. It's more of a burden on the reader to understand the interface by reading how it is called. PS1, Line 701: return str(randint(10, 20)) I would like for us to make our randomized tests more repeatable. To that end, I think we should, at least in new code, do the following: - provide a seed command line option in all entry points. This could probably go into cli_options since I'd like to use this for the random query generator in new code - if the seed needs to be "randomly chosen" for more chaos, or is seeded based on the timer, that's fine. However we should print the seed chosen so the queries can be repeated. - no longer use the module-level functions and instead initialize a random.Random() object. See https://docs.python.org/2/library/random.html. PS1, Line 713: result = re.sub(pattern, "10", self._sql) Was the hard coding of 10 here meant to actually be self.MIN_INT? PS1, Line 1379: query.modifies_table = False Why is this False? PS1, Line 1392: def generate_delete_queries(cursor): Lots of duplicated logic here and in generate_upsert_queries(). I think addressing Alex's point about about breaking abstractions should also consider this: the code is mostly the same except for the "setup" SQL that's needed before. PS1, Line 1664: if query._sql in queries_with_runtime_info_by_db_and_sql[query.db_name]: It doesn't seem right to be examining a private attribute from the outside. We should use the public interface and the public interface should be providing what callers need. -- To view, visit http://gerrit.cloudera.org:8080/5093 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia2aafdc6851cc0e1677a3c668d3350e47c4bfe40 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Taras Bobrovytsky <tbobrovyt...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Michael Brown <mi...@cloudera.com> Gerrit-HasComments: Yes
