Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/10319 )
Change subject: IMPALA-6070: Adding ASAN, --tail to test-with-docker. ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/10319/1/docker/entrypoint.sh File docker/entrypoint.sh: http://gerrit.cloudera.org:8080/#/c/10319/1/docker/entrypoint.sh@262 PS1, Line 262: noclean shouldn't we clean up before building with asan? http://gerrit.cloudera.org:8080/#/c/10319/1/docker/test-with-docker.py File docker/test-with-docker.py: http://gerrit.cloudera.org:8080/#/c/10319/1/docker/test-with-docker.py@409 PS1, Line 409: self.lock = threading.Lock() Can you add a comment explaining what this protects? http://gerrit.cloudera.org:8080/#/c/10319/1/docker/test-with-docker.py@565 PS1, Line 565: def _rm_container(container): You could also add a synchronized decorator and use it to synchronize the method, but that might be overkill. http://gerrit.cloudera.org:8080/#/c/10319/1/docker/test-with-docker.py@573 PS1, Line 573: with container.lock: Wouldn't it be safer to grab the lock before the if above? That way you'd only call docker once in total, vs several times in series. -- To view, visit http://gerrit.cloudera.org:8080/10319 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I51451cdf1352fc0f9516d729b9a77700488d993f Gerrit-Change-Number: 10319 Gerrit-PatchSet: 1 Gerrit-Owner: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Comment-Date: Sat, 05 May 2018 18:14:48 +0000 Gerrit-HasComments: Yes
