From dev-return-14210-apmail-sqoop-dev-archive=sqoop.apache.org@sqoop.apache.org Tue Oct 21 20:47:30 2014 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 0955F17AA7 for ; Tue, 21 Oct 2014 20:47:30 +0000 (UTC) Received: (qmail 26202 invoked by uid 500); 21 Oct 2014 20:47:29 -0000 Delivered-To: apmail-sqoop-dev-archive@sqoop.apache.org Received: (qmail 26170 invoked by uid 500); 21 Oct 2014 20:47:29 -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 26151 invoked by uid 500); 21 Oct 2014 20:47:29 -0000 Delivered-To: apmail-incubator-sqoop-dev@incubator.apache.org Received: (qmail 26144 invoked by uid 99); 21 Oct 2014 20:47:29 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 21 Oct 2014 20:47:29 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id 6E63F1DF4DF; Tue, 21 Oct 2014 20:47:34 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============8165146866417820784==" MIME-Version: 1.0 Subject: Re: Review Request 26941: SQOOP-1557: SQ_CONFIGURABLE ( for entities who own configs) From: "Jarek Cecho" To: "Veena Basavaraj" , "Sqoop" , "Jarek Cecho" Date: Tue, 21 Oct 2014 20:47:34 -0000 Message-ID: <20141021204734.1283.91405@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org Auto-Submitted: auto-generated Sender: "Jarek Cecho" X-ReviewGroup: Sqoop X-ReviewRequest-URL: https://reviews.apache.org/r/26941/ X-Sender: "Jarek Cecho" References: <20141021191945.1282.75225@reviews.apache.org> In-Reply-To: <20141021191945.1282.75225@reviews.apache.org> Reply-To: "Jarek Cecho" X-ReviewRequest-Repository: sqoop-sqoop2 --===============8165146866417820784== 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/26941/#review57652 ----------------------------------------------------------- repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoError.java Super nit: Can we also put the semicolon on it's own line? :) My goal here is to have ability to add new exception code by just adding a new line without need to touch other lines which will make it easier for "git cherrypick" or "git blame". common/src/main/java/org/apache/sqoop/model/MConnector.java Seems as still the same invalid import? :) common/src/main/java/org/apache/sqoop/model/MDriver.java Thank you for cleaning this one! core/src/main/java/org/apache/sqoop/driver/Driver.java Do you think that it would make sense to change this call to Driver.getClass().getSimpleName() so that we don't have to change it if at some point in the future we decides to change the package or class name? core/src/main/java/org/apache/sqoop/repository/JdbcRepository.java I'm thinking if it would be claner to call here mDriver.getUniqueName() instead of going to the Driver class directly? It's more a nitpick at this point, but this methods assumes that the mDriver will be derived from Driver. Which right now will definitely be the case, but I'm wondering whether we want to have such assumption here. core/src/main/java/org/apache/sqoop/repository/Repository.java Seems as unused import? repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java Can we keep the comment here so that it's obvious what we're doing here? repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java Nit: Trailing whitespace repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java Nit: Trailing whitespace repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java Nit: Trailing whitespace repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java Nit: Trailing whitespace Jarcec - Jarek Cecho On Oct. 21, 2014, 7:19 p.m., Veena Basavaraj wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26941/ > ----------------------------------------------------------- > > (Updated Oct. 21, 2014, 7:19 p.m.) > > > Review request for Sqoop. > > > Repository: sqoop-sqoop2 > > > Description > ------- > > see JIRA for details > > there is whitespace, that will be addressed once the reviews for the functionality > > > Diffs > ----- > > common/src/main/java/org/apache/sqoop/model/MConnector.java 174d0b9 > common/src/main/java/org/apache/sqoop/model/MDriver.java 4241a31 > core/src/main/java/org/apache/sqoop/driver/Driver.java 46a16ac > core/src/main/java/org/apache/sqoop/repository/JdbcRepository.java 476830d > core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java 4c5229f > core/src/main/java/org/apache/sqoop/repository/Repository.java 8f78052 > core/src/test/java/org/apache/sqoop/repository/TestJdbcRepository.java ff9e0c3 > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoConstants.java 40dcc49 > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoError.java 3e4a4a9 > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java aa58850 > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaConstants.java 59773e1 > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java 44ec2e3 > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java 366e4ee > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestConnectorHandling.java 68a173b > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestDriverHandling.java bbf721f > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestJobHandling.java a15bda9 > tools/src/main/java/org/apache/sqoop/tools/tool/RepositoryLoadTool.java 8cf9cf1 > > Diff: https://reviews.apache.org/r/26941/diff/ > > > Testing > ------- > > yes > > > Thanks, > > Veena Basavaraj > > --===============8165146866417820784==--