impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Brown (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] Preview: IMPALA-4467: Add support for CRUD operations in stress test
Date Fri, 18 Nov 2016 17:34:50 GMT
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 <tbobrovytsky@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Michael Brown <mikeb@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message