Tim Armstrong has posted comments on this change. Change subject: IMPALA-3764: fuzz test HDFS scanners ......................................................................
Patch Set 3: (8 comments) Thanks for the comment, sorry for the slow turnaround http://gerrit.cloudera.org:8080/#/c/3448/3/tests/query_test/test_scanners.py File tests/query_test/test_scanners.py: Line 642: rng = random.Random() > I don't think we need to create a Random object, because it's inconvenient I find it much easier to reason about using the explicit rng. The problem with using the builtin global one is that to guarantee that the results are deterministic I have to convince myself that no function I'm calling will call into the RNG. Passing it explicitly means a little more code but it's way easier to reason about. I could make it a member of the class if you feel that's better. Line 644: if random_seed is None: random_seed = time.time() > how about: Done PS3, Line 676: not > nit: I think it's a little easier to read if you put "not" right before "in Done. Good point, forgot about that syntax Line 701: """ > Start the comment on this line. Here and elsewhere. Done PS3, Line 721: copy > nit: how about copy_num? Done PS3, Line 722: copy%d_%s > I think here it's easier to use format here: Done PS3, Line 726: self.corrupt_file(filepath, rng) > How about moving this to the loop above under shutil.copyfile? That doesn't quite work since we need to corrupt the original file too. PS3, Line 750: 0, 254 > Maybe leave a comment why only 255 different values are generated (instead Ah, should be 255 -- To view, visit http://gerrit.cloudera.org:8080/3448 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I50cf43195a7c582caa02c85ae400ea2256fa3a3b Gerrit-PatchSet: 3 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
