David Knupp has posted comments on this change. Change subject: IMPALA-5331: Use new libHDFS API to address "Unknown Error 255" ......................................................................
Patch Set 6: (2 comments) http://gerrit.cloudera.org:8080/#/c/6894/6/tests/data_errors/test_data_errors.py File tests/data_errors/test_data_errors.py: Line 78: try: I wonder about stacking all of these asserts in one block. If any of them fails, the test will obviously fail, but not before trying to "leave" safe mode in the finally: clause. Is that the right thing to do in every situation? (E.g., maybe it makes more sense to leave the state as-is for triage? I don't know, I'm just asking.) Also, presumably the hdfs commands could fail in multiple ways. The first check to "get" the safemode, for example -- either the hdfs command succeeds but safe mode is "ON", or the command itself fails for some other reason. To differentiate the behavior and provide better assistance for someone triaging test failures, you might consider examining stderr as well, using something like: proc = subprocess.Popen(['hdfs', 'dfsadmin', '-safemode', 'get'], stdout=subprocess.PIPE, stderr=subprocess.PIPE) output, error = proc.communicate() If "output" contains "Safe mode is ON" and "error" is an empty string, you know the command succeeded, but the expected state is incorrect. If the reverse is true, and "error" contains the meaningful string, then the command itself has failed. Perhaps you want to take different actions in each case. In the unlikely case that "hdfs" is not a valid command, an OSError will be raised. So maybe that's a third failure mode. Line 97: assert "Safe mode is OFF" in subprocess.Popen( If this is the happy path, this is fine. If the assert fails though, you might want to add a warning for the user that their safemode setting might be incorrect -- e.g., assert <leaving safe mode succeeded>, "Command failed: system may still be in safemode" ... or something like that? -- To view, visit http://gerrit.cloudera.org:8080/6894 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I181e316ed63b70b94d4f7a7557d398a931bb171d Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
