Return-Path: X-Original-To: apmail-sqoop-dev-archive@www.apache.org Delivered-To: apmail-sqoop-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id D91D517C44 for ; Sat, 21 Mar 2015 02:16:04 +0000 (UTC) Received: (qmail 7411 invoked by uid 500); 21 Mar 2015 02:16:04 -0000 Delivered-To: apmail-sqoop-dev-archive@sqoop.apache.org Received: (qmail 7374 invoked by uid 500); 21 Mar 2015 02:16:04 -0000 Mailing-List: contact dev-help@sqoop.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@sqoop.apache.org Delivered-To: mailing list dev@sqoop.apache.org Received: (qmail 7360 invoked by uid 500); 21 Mar 2015 02:16:04 -0000 Delivered-To: apmail-incubator-sqoop-dev@incubator.apache.org Received: (qmail 7350 invoked by uid 99); 21 Mar 2015 02:16:04 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Sat, 21 Mar 2015 02:16:04 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id 357601D46D4; Sat, 21 Mar 2015 02:16:04 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============8514056573630523632==" MIME-Version: 1.0 Subject: Re: Review Request 31657: SENTRY-644: Sentry Sqoop binding framework for role-based authorization From: "Prasad Mujumdar" To: "Abraham Elmahrek" , "Dapeng Sun" , "Prasad Mujumdar" , "Colin Ma" , "Xiaomeng Huang" Cc: "shen guoquan" , "sentry" , "Sqoop" Date: Sat, 21 Mar 2015 02:16:04 -0000 Message-ID: <20150321021604.1490.51978@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org/ Auto-Submitted: auto-generated Sender: "Prasad Mujumdar" X-ReviewGroup: sentry, Sqoop X-ReviewRequest-URL: https://reviews.apache.org/r/31657/ X-Sender: "Prasad Mujumdar" References: <20150318021415.391.81898@reviews.apache.org> In-Reply-To: <20150318021415.391.81898@reviews.apache.org> Reply-To: "Prasad Mujumdar" X-ReviewRequest-Repository: sentry --===============8514056573630523632== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31657/#review77299 ----------------------------------------------------------- Looks fine. Some minor comments below. sentry-binding/sentry-binding-sqoop/src/main/java/org/apache/sentry/sqoop/authz/SentryAuthorizationHander.java Does this need to be info ? I am not sure how often sqoop calls this. It might be too much noise in the log file. sentry-binding/sentry-binding-sqoop/src/main/java/org/apache/sentry/sqoop/authz/SentryAuthorizationHander.java Should this be InstantiationException ? sentry-binding/sentry-binding-sqoop/src/main/java/org/apache/sentry/sqoop/authz/SentryAuthorizationHander.java It would be great if SqoopConfig could directly indentify if strong authorization is enabled rather than Sentry figuring out that from sqoop config. Let's work with Sqoop comminuty to make this into an API in future. sentry-binding/sentry-binding-sqoop/src/main/java/org/apache/sentry/sqoop/authz/SentryAuthorizationValidator.java " does not have privilege for action " ? sentry-binding/sentry-binding-sqoop/src/main/java/org/apache/sentry/sqoop/binding/SqoopAuthBinding.java Nit: formatting. sentry-binding/sentry-binding-sqoop/src/main/java/org/apache/sentry/sqoop/binding/SqoopAuthBinding.java Is there a reason we don't pass the full exception to SqoopException() ? - Prasad Mujumdar On March 18, 2015, 2:14 a.m., shen guoquan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31657/ > ----------------------------------------------------------- > > (Updated March 18, 2015, 2:14 a.m.) > > > Review request for sentry, Sqoop, Abraham Elmahrek, Xiaomeng Huang, Colin Ma, Dapeng Sun, and Prasad Mujumdar. > > > Repository: sentry > > > Description > ------- > > Sentry Sqoop binding framework for role-based authorization > > > Diffs > ----- > > pom.xml 4c80916 > sentry-binding/pom.xml 7428aa5 > sentry-binding/sentry-binding-sqoop/pom.xml PRE-CREATION > sentry-binding/sentry-binding-sqoop/src/main/java/org/apache/sentry/sqoop/PrincipalDesc.java PRE-CREATION > sentry-binding/sentry-binding-sqoop/src/main/java/org/apache/sentry/sqoop/SentrySqoopError.java PRE-CREATION > sentry-binding/sentry-binding-sqoop/src/main/java/org/apache/sentry/sqoop/authz/NoopSentryAccessController.java PRE-CREATION > sentry-binding/sentry-binding-sqoop/src/main/java/org/apache/sentry/sqoop/authz/SentryAccessController.java PRE-CREATION > sentry-binding/sentry-binding-sqoop/src/main/java/org/apache/sentry/sqoop/authz/SentryAuthorizationHander.java PRE-CREATION > sentry-binding/sentry-binding-sqoop/src/main/java/org/apache/sentry/sqoop/authz/SentryAuthorizationValidator.java PRE-CREATION > sentry-binding/sentry-binding-sqoop/src/main/java/org/apache/sentry/sqoop/binding/SqoopAuthBinding.java PRE-CREATION > sentry-binding/sentry-binding-sqoop/src/main/java/org/apache/sentry/sqoop/conf/SqoopAuthConf.java PRE-CREATION > sentry-binding/sentry-binding-sqoop/src/test/java/org/apache/sentry/sqoop/MockAuthenticationProvider.java PRE-CREATION > sentry-binding/sentry-binding-sqoop/src/test/java/org/apache/sentry/sqoop/TestSentryAuthorizationHander.java PRE-CREATION > sentry-binding/sentry-binding-sqoop/src/test/java/org/apache/sentry/sqoop/TestSqoopAuthConf.java PRE-CREATION > sentry-binding/sentry-binding-sqoop/src/test/resources/no-configure-sentry-site.xml PRE-CREATION > sentry-binding/sentry-binding-sqoop/src/test/resources/sentry-site.xml PRE-CREATION > sentry-binding/sentry-binding-sqoop/src/test/resources/test-authz-provider.ini PRE-CREATION > sentry-dist/pom.xml f63b33b > sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/AuthorizationComponent.java def3486 > > Diff: https://reviews.apache.org/r/31657/diff/ > > > Testing > ------- > > Ran unit tests > > > Thanks, > > shen guoquan > > --===============8514056573630523632==--