impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "David Knupp (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5331: Use new libHDFS API to address "Unknown Error 255"
Date Mon, 22 May 2017 05:28:13 GMT
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 <sailesh@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: David Knupp <dknupp@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message