Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201705220528.v4M5SEb5027114@ip-10-146-233-104.ec2.internal>
Date: Mon, 22 May 2017 05:28:13 +0000
From: "David Knupp (Code Review)" <gerrit@cloudera.org>
To: Sailesh Mukil <sailesh@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Henry Robinson <henry@cloudera.com>, Alex Behm <alex.behm@cloudera.com>,
        Matthew Jacobs <mj@cloudera.com>, Dan Hecht <dhecht@cloudera.com>
Reply-To: dknupp@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-5331=3A_Use_new_libHDFS_API_to_address_=22Unknown_Error_255=22=0A?=
In-Reply-To: <gerrit.1494916804000.I181e316ed63b70b94d4f7a7557d398a931bb171d@gerrit.cloudera.org>
References: <gerrit.1494916804000.I181e316ed63b70b94d4f7a7557d398a931bb171d@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.7
archived-at: Mon, 22 May 2017 05:28:18 -0000

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