Michael Brown has posted comments on this change. ( http://gerrit.cloudera.org:8080/9921 )
Change subject: IMPALA-6793: Fix empty metadata after statestore restarts ...................................................................... Patch Set 7: (5 comments) I'm kind of in the same mode as Alex, and I'll to avoid rehashing his concerns. http://gerrit.cloudera.org:8080/#/c/9921/7/tests/custom_cluster/test_restart_services.py File tests/custom_cluster/test_restart_services.py: http://gerrit.cloudera.org:8080/#/c/9921/7/tests/custom_cluster/test_restart_services.py@38 PS7, Line 38: sleep(60) If this is expected to differ greatly in ASAN builds, consider using specific_build_type_timeout() . It's also unfortunate you're not able to poll here. Can you at least add a comment that says why you can't do so? From my reading of the long discussion about this test, it seems that there's no metric suitable. http://gerrit.cloudera.org:8080/#/c/9921/7/tests/custom_cluster/test_restart_services.py@39 PS7, Line 39: range(0, 10) Python trivia that's for your info that you do or don't have to fix, up to you. 1. This can just be range(10). 2. You can also switch to using xrange(10) which does lazy evaluation of each element. In this case, it's a micro-optimization. http://gerrit.cloudera.org:8080/#/c/9921/7/tests/custom_cluster/test_restart_services.py@43 PS7, Line 43: except HiveServer2Error: Is any HiveServer2Error acceptable, or just an AnalysisException in which functional does not exist? If the latter, maybe inspect the exception to make sure that's what you got, and fail the test if you got something else. http://gerrit.cloudera.org:8080/#/c/9921/7/tests/custom_cluster/test_restart_services.py@44 PS7, Line 44: sleep(10) Where does 10s come from? Why not 1? http://gerrit.cloudera.org:8080/#/c/9921/7/tests/custom_cluster/test_restart_services.py@45 PS7, Line 45: assert False How about something a tad more descriptive, like: assert False, "Coordinator never received metadata from restarted statestore after {0} seconds or iterations".format(calculation_seconds_or_iterations_or_similar) -- To view, visit http://gerrit.cloudera.org:8080/9921 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I332a60e172af84b93b3544373fe363cdced5e8d0 Gerrit-Change-Number: 9921 Gerrit-PatchSet: 7 Gerrit-Owner: Tianyi Wang <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Tianyi Wang <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Wed, 11 Apr 2018 21:58:17 +0000 Gerrit-HasComments: Yes
