> On Oct. 20, 2014, 1:27 a.m., Veena Basavaraj wrote: > > connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseConnectorError.java, > > line 24 > > <https://reviews.apache.org/r/26581/diff/2/?file=724133#file724133line24> > > > > 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 > > }
Possibly. Addressable in a separate Jira. > On Oct. 20, 2014, 1:27 a.m., Veena Basavaraj wrote: > > connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseExecutor.java, > > line 52 > > <https://reviews.apache.org/r/26581/diff/2/?file=724135#file724135line52> > > > > 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 Hmm like Preconditions for methods? In general seems like a good idea. Not implemented yet though, so let's log a ticket and do it later? > On Oct. 20, 2014, 1:27 a.m., Veena Basavaraj wrote: > > connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseExecutor.java, > > line 71 > > <https://reviews.apache.org/r/26581/diff/2/?file=724135#file724135line71> > > > > 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 I wanted to make the executor self contained. By sharing a connection, we will have to close the connection outside of the executor. Or, we'll have to add "start" and "stop" methods to the executor. This seemed easier. > On Oct. 20, 2014, 1:27 a.m., Veena Basavaraj wrote: > > connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseExecutor.java, > > line 108 > > <https://reviews.apache.org/r/26581/diff/2/?file=724135#file724135line108> > > > > its nice to use Apache common StringUtils in cases like this for length > > and null check. This is an array of HColumnDescriptor? > On Oct. 20, 2014, 1:27 a.m., Veena Basavaraj wrote: > > connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseExecutor.java, > > line 111 > > <https://reviews.apache.org/r/26581/diff/2/?file=724135#file724135line111> > > > > > > http://stackoverflow.com/questions/3052442/what-is-the-difference-between-text-and-new-stringtext-in-java > > > > new String is a overkill :) ``getName()`` returns an array of bytes. > On Oct. 20, 2014, 1:27 a.m., Veena Basavaraj wrote: > > connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseLoader.java, > > line 32 > > <https://reviews.apache.org/r/26581/diff/2/?file=724136#file724136line32> > > > > 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 Hmm protected scope is on purpose. This forces test case writers to extend this class and override these methods. What test purpose does package scope serve? > On Oct. 20, 2014, 1:27 a.m., Veena Basavaraj wrote: > > connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/HBaseToDestroyer.java, > > line 31 > > <https://reviews.apache.org/r/26581/diff/2/?file=724137#file724137line31> > > > > 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. Agreed. Future Jiras. > On Oct. 20, 2014, 1:27 a.m., Veena Basavaraj wrote: > > connector/connector-hbase/src/main/java/org/apache/sqoop/connector/hbase/configuration/LinkConfiguration.java, > > line 33 > > <https://reviews.apache.org/r/26581/diff/2/?file=724140#file724140line33> > > > > 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. Let's address generalizations in future jiras? - Abraham ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26581/#review57303 ----------------------------------------------------------- On Oct. 17, 2014, 6:37 a.m., Abraham Elmahrek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26581/ > ----------------------------------------------------------- > > (Updated Oct. 17, 2014, 6:37 a.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 > >
