Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/11225 )
Change subject: tests: ensure consistent logging format across tests ...................................................................... Patch Set 1: Code-Review+2 (2 comments) Thanks for doing this. I think it's strictly an improvement as is, so +2. Consider adding a timestamp too. The other weird thing is that maybe we should be doing this in run-all-tests.py. E.g. the following, which I had around. Ultimately, I was trying to understand if I fixed IMPALA-5219, and I wasn't able to convincingly reproduce IMPALA-5219. I didn't try terribly hard, but the issue is that run-all-tests.py calls py.test, so anything happening and logging in run-all-tests.py is delaying when basicConfig is run. (In my experience, first call to basicConfig wins.) >From a testing perspective, I assume you have, but please spot check some >tests that they still have logging in their junit XMLs and stuff. commit 7377eda61ec116993a4dd4774c2fc413eaf70e6f Author: Philip Zeyliger <[email protected]> Date: Mon Apr 30 11:36:54 2018 -0700 IMPALA-5219: Enable logging for e2e tests. Change-Id: Id991b66db3d637e92e1b69b5f3c54e57fa7ca74e diff --git tests/run-tests.py tests/run-tests.py index b1a9fdd..1733d9f 100755 --- tests/run-tests.py +++ tests/run-tests.py @@ -26,6 +26,7 @@ from tests.common.impala_cluster import ImpalaCluster from tests.common.impala_service import ImpaladService import itertools import json +import logging import multiprocessing import os import pytest @@ -224,6 +225,8 @@ def print_metrics(substring): if __name__ == "__main__": + logging.basicConfig(level=logging.INFO, + format='%(asctime)s %(threadName)s %(levelname)s: %(message)s') exit_on_error = '-x' in sys.argv or '--exitfirst' in sys.argv skip_serial = '--skip-serial' in sys.argv if skip_serial: http://gerrit.cloudera.org:8080/#/c/11225/1/tests/common/impala_cluster.py File tests/common/impala_cluster.py: http://gerrit.cloudera.org:8080/#/c/11225/1/tests/common/impala_cluster.py@36 PS1, Line 36: LOG.setLevel(level=logging.DEBUG) This line is "global" too, though I think it's intentional. http://gerrit.cloudera.org:8080/#/c/11225/1/tests/conftest.py File tests/conftest.py: http://gerrit.cloudera.org:8080/#/c/11225/1/tests/conftest.py@56 PS1, Line 56: logging.basicConfig(level=logging.INFO, format='-- %(threadName)s: %(message)s') Should we put a timestamp in here? We have a variety of these that do have timestamps. logging.basicConfig(stream=sys.stdout, level=logging.DEBUG, datefmt="%Y-%m-%d %H:%M:%S", format="%(asctime)s %(levelname)-8s %(message)s") -- To view, visit http://gerrit.cloudera.org:8080/11225 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I55ef0214b43f87da2d71804913ba4caa964f789f Gerrit-Change-Number: 11225 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 15 Aug 2018 15:50:44 +0000 Gerrit-HasComments: Yes
