----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26581/#review57303 -----------------------------------------------------------
Nice feature. Have tons of comments. Please feel free to ping me in person if you need clarifications. connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseConnector.java <https://reviews.apache.org/r/26581/#comment97952> Please remove todo: seems ok to have a exception until we add support for FROM connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseConnector.java <https://reviews.apache.org/r/26581/#comment97953> same as above connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseConnector.java <https://reviews.apache.org/r/26581/#comment97954> heads up : upgrade api has changed. connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseConnectorConstants.java <https://reviews.apache.org/r/26581/#comment97955> please call this configs, helps to be consistent with other 2 conenctors and more readable. connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseConnectorError.java <https://reviews.apache.org/r/26581/#comment97956> a thought. why we have Errorcode has a interface, can we just make it class and make it simple for developers not to have to duplicate this in every conenctor? private final String message; 40 41 private HBaseConnectorError(String message) { 42 this.message = message; 43 } 44 45 public String getCode() { 46 return name(); 47 } 48 49 public String getMessage() { 50 return message; 51 } connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseConnectorRepositoryUpgrader.java <https://reviews.apache.org/r/26581/#comment97947> Please see the latest code, the api has changed. There is util class to avoid the copy paste. I ahve refactored common logic used by all configurables to a util class connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseExecutor.java <https://reviews.apache.org/r/26581/#comment97957> nice annotation. I did not know this:) connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseExecutor.java <https://reviews.apache.org/r/26581/#comment97958> are not these configs values a must? if the answer is yes, they are required. can we not throw an exception right here? Or is this checked in validators. I have not seen a way yet that enforces required/ optiona. Might be a good annotation feature to have connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseExecutor.java <https://reviews.apache.org/r/26581/#comment97962> I am curious if we can store the connection here. Do we need to create a new one evertime we call getTables, getColumnFamilies? why cant we reuse? I might be missing something, but I felt it is worth asking connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseExecutor.java <https://reviews.apache.org/r/26581/#comment97959> its nice to use Apache common StringUtils in cases like this for length and null check. connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseExecutor.java <https://reviews.apache.org/r/26581/#comment97960> http://stackoverflow.com/questions/3052442/what-is-the-difference-between-text-and-new-stringtext-in-java new String is a overkill :) connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseExecutor.java <https://reviews.apache.org/r/26581/#comment97961> can see many cases of NPE here. I hope the unit tests cover it. connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseLoader.java <https://reviews.apache.org/r/26581/#comment97963> nitpick we can have default access for methods used in testing only, it does not need to be protected. Only trick is of course have the test in the same package. Please remove protected connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseToDestroyer.java <https://reviews.apache.org/r/26581/#comment97964> This seems like a case where we could provide a common abstract class for connector developers, they just need not even add this class, since there is nothing they are even doing. connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseToInitializer.java <https://reviews.apache.org/r/26581/#comment97965> please use linkConfig, LinkConfig and Link are 2 different things, just makes code much readable . connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/configuration/LinkConfiguration.java <https://reviews.apache.org/r/26581/#comment97948> nit pick, can this be renamed to linkConfig. connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/configuration/LinkConfiguration.java <https://reviews.apache.org/r/26581/#comment97966> Please see if we can move these classes to connecto sdk so we can reuse them across all connectors.? Since I dont see any specific config validators either Again some of these comments are not related to HBase, but in general reducing the burden for connector developers. connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/configuration/ToJobConfiguration.java <https://reviews.apache.org/r/26581/#comment97949> toTable, please rename might be a artifact of c&p connector/connector-hbase/src/main/resources/hbase-connector-resources.properties <https://reviews.apache.org/r/26581/#comment97950> Please cleanup usages of forms to configs and would be nice to have linkConfig, toJobConfig and fromJobConfig as the properties. Also, to be consistent with other 2 connectors, kindly request you rename this to *configs.properties connector/connector-hbase/src/main/resources/sqoopconnector.properties <https://reviews.apache.org/r/26581/#comment97951> JDBC? please fix the comment connector/connector-hbase/src/test/java/org/apache/sqoop/connector/hbase/TestExecutor.java <https://reviews.apache.org/r/26581/#comment97967> call it HBaseExecutor, please. Consistent naming helps. connector/connector-hbase/src/test/java/org/apache/sqoop/connector/hbase/TestExecutor.java <https://reviews.apache.org/r/26581/#comment97968> nit pick, suffixing with mock, makes code redable as well. connector/connector-hbase/src/test/java/org/apache/sqoop/connector/hbase/TestLoader.java <https://reviews.apache.org/r/26581/#comment97969> is this intentional to have extractor table in the test loader ? - Veena Basavaraj On Oct. 16, 2014, 11:37 p.m., Abraham Elmahrek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26581/ > ----------------------------------------------------------- > > (Updated Oct. 16, 2014, 11:37 p.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-1156 > https://issues.apache.org/jira/browse/SQOOP-1156 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > commit b9af5c1556e2c88a3c861950a6a296733fa1e4e7 > Author: Abraham Elmahrek <[email protected]> > Date: Thu Oct 9 22:56:34 2014 -0700 > > SQOOP-1156: HBase connector > > :000000 100644 0000000... 61dd408... A connector/connector-hbase/pom.xml > :000000 100644 0000000... b0e4ea0... A > connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseConnector.java > :000000 100644 0000000... 975cb40... A > connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseConnectorConstants.java > :000000 100644 0000000... 2e08bfd... A > connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseConnectorError.java > :000000 100644 0000000... cf632c9... A > connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseConnectorRepositoryUpgrader.java > :000000 100644 0000000... bc61993... A > connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseExecutor.java > :000000 100644 0000000... 991846f... A > connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseLoader.java > :000000 100644 0000000... 325591c... A > connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseToDestroyer.java > :000000 100644 0000000... 51fd885... A > connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseToInitializer.java > :000000 100644 0000000... 6e306b1... A > connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/configuration/LinkConfig.java > :000000 100644 0000000... a4b825c... A > connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/configuration/LinkConfiguration.java > :000000 100644 0000000... 15feb54... A > connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/configuration/ToJobConfig.java > :000000 100644 0000000... cc71b78... A > connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/configuration/ToJobConfiguration.java > :000000 100644 0000000... 15a9425... A > connector/connector-hbase/src/main/resources/hbase-connector-resources.properties > :000000 100644 0000000... 1fc360e... A > connector/connector-hbase/src/main/resources/sqoopconnector.properties > :000000 100644 0000000... c78042a... A > connector/connector-hbase/src/test/java/org/apache/sqoop/connector/hbase/TestExecutor.java > :000000 100644 0000000... 60f8217... A > connector/connector-hbase/src/test/java/org/apache/sqoop/connector/hbase/TestLoader.java > :000000 100644 0000000... 44ffced... A > connector/connector-hbase/src/test/resources/log4j.properties > :100644 100644 e98a0fc... 35c665e... M connector/pom.xml > :100644 100644 f25a29f... a556bcf... M pom.xml > :100644 100644 67baaa5... 21a1fa9... M server/pom.xml > :100644 100644 7a80710... fbd4e84... M test/pom.xml > > > Diffs > ----- > > connector/connector-hbase/pom.xml PRE-CREATION > > connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseConnector.java > PRE-CREATION > > connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseConnectorConstants.java > PRE-CREATION > > connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseConnectorError.java > PRE-CREATION > > connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseConnectorRepositoryUpgrader.java > PRE-CREATION > > connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseExecutor.java > PRE-CREATION > > connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseLoader.java > PRE-CREATION > > connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseToDestroyer.java > PRE-CREATION > > connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseToInitializer.java > PRE-CREATION > > connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/configuration/LinkConfig.java > PRE-CREATION > > connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/configuration/LinkConfiguration.java > PRE-CREATION > > connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/configuration/ToJobConfig.java > PRE-CREATION > > connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/configuration/ToJobConfiguration.java > PRE-CREATION > > connector/connector-hbase/src/main/resources/hbase-connector-resources.properties > PRE-CREATION > connector/connector-hbase/src/main/resources/sqoopconnector.properties > PRE-CREATION > > connector/connector-hbase/src/test/java/org/apache/sqoop/connector/hbase/TestExecutor.java > PRE-CREATION > > connector/connector-hbase/src/test/java/org/apache/sqoop/connector/hbase/TestLoader.java > PRE-CREATION > connector/connector-hbase/src/test/resources/log4j.properties PRE-CREATION > connector/pom.xml e98a0fc > pom.xml f25a29f > server/pom.xml 67baaa5 > test/pom.xml 7a80710 > > Diff: https://reviews.apache.org/r/26581/diff/ > > > Testing > ------- > > mvn clean verify + can transfer from mysql to hbase. > > > Thanks, > > Abraham Elmahrek > >
