> On Oct. 26, 2014, 10:39 p.m., Gwen Shapira wrote: > > Hate to contradict Veena, but I don't think this is ready yet. For one > > thing, the integration tests this patch touches are failing on my machine, > > and the validator looks broken as well. > > Veena Basavaraj wrote: > hehe, I did not run the patch, I proabably should have. I am glad you did > not have to agree:)
Indeed, the validator wasn't originally running. This is also why the integration tests are now failing. Fixed. > On Oct. 26, 2014, 10:39 p.m., Gwen Shapira wrote: > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsUtils.java, > > line 25 > > <https://reviews.apache.org/r/27110/diff/3/?file=733150#file733150line25> > > > > Not happy about a super generic name for a method that only handles > > URIs (and not compression codecs for example). Is the plan to use this for > > all configurations in the future? If so, please leave a comment to that > > effect (and perhaps double check if its even feasible). Good point. Changing to configureURI. It would be cool to have a single place where we provide properties for a Configuration object. It would make troubleshooting and testing easier. The codec is separate though... Sqoop loads the codec based on its own configuration and the filename. It's not yet used in the Configuration object other than producing a set of supported codecs (which we don't configure). > On Oct. 26, 2014, 10:39 p.m., Gwen Shapira wrote: > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/configuration/LinkConfig.java, > > line 34 > > <https://reviews.apache.org/r/27110/diff/3/?file=733151#file733151line34> > > > > I love the idea of verifying the URI, but something looks wrong here. > > > > 1. The regex assumes two ":" are mandatory, i.e. we'll always have a > > port number. But hdfs:///tmp and hdfs://ns1/tmp are both valid. > > 2. It looks like we only allow a single character between the ":"? > > i.e. hdfs://a:8920 matches but hdfs://abcd:8020 does not. This is obviously > > broken. > > > > So, please add unit tests for this. > > Veena Basavaraj wrote: > very good catch. Good point. For instance, file:/// should be supported as well. Maybe http://blog.dieweltistgarnichtso.net/constructing-a-regular-expression-that-matches-uris would work out for us. > On Oct. 26, 2014, 10:39 p.m., Gwen Shapira wrote: > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/configuration/LinkConfig.java, > > lines 41-43 > > <https://reviews.apache.org/r/27110/diff/3/?file=733151#file733151line41> > > > > Users will love it if we give an example for a legal URI in the error > > message. i.e: "... for example the URI hdfs://nameservice1:8020/sqoop is > > valid". It gives them something to copy paste. > > Veena Basavaraj wrote: > love this comment too Gwen! I love making anything that makes users life > happy. Good point! - Abraham ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27110/#review58574 ----------------------------------------------------------- On Oct. 25, 2014, 12:12 a.m., Abraham Elmahrek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27110/ > ----------------------------------------------------------- > > (Updated Oct. 25, 2014, 12:12 a.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-1620 > https://issues.apache.org/jira/browse/SQOOP-1620 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > commit ef7c1e58b115fec6c6d5a84345ca1e3c0cc93164 > Author: Abraham Elmahrek <[email protected]> > Date: Fri Oct 10 19:02:27 2014 -0700 > > SQOOP-1620: Sqoop2: FileSystem should be configurable in HDFS connector > > :100644 100644 cce0e29... 7ad66f8... M > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsConnector.java > :100644 100644 31b0a99... 0a2c98b... M > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsExtractor.java > :100644 100644 c7d35f7... 6d79db7... M > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsFromDestroyer.java > :100644 100644 0752510... 4c6f566... M > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsFromInitializer.java > :100644 100644 682349c... 0753f9d... M > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsLoader.java > :100644 100644 daa7fe2... 12bcd53... M > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsPartitioner.java > :100644 100644 8bfd727... 3c85be8... M > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsToDestroyer.java > :100644 100644 e3d54b8... bce72b5... M > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsToInitializer.java > :000000 100644 0000000... 0d77427... A > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsUtils.java > :000000 100644 0000000... 54768ea... A > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/configuration/LinkConfig.java > :000000 100644 0000000... 29063a8... A > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/configuration/LinkConfiguration.java > :100644 100644 90bc8bc... 3d088d0... M > connector/connector-hdfs/src/main/resources/hdfs-connector-config.properties > :100644 100644 124c3df... 0a6369f... M > connector/connector-hdfs/src/test/java/org/apache/sqoop/connector/hdfs/TestExtractor.java > :000000 100644 0000000... 846b565... A > connector/connector-hdfs/src/test/java/org/apache/sqoop/connector/hdfs/TestHdfsUtils.java > :100644 100644 b1b3b16... f88424b... M > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/FromHDFSToRDBMSTest.java > :100644 100644 36f7443... b89424f... M > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/FromRDBMSToHDFSTest.java > > > Diffs > ----- > > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsConnector.java > cce0e29 > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsExtractor.java > 31b0a99 > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsFromDestroyer.java > c7d35f7 > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsFromInitializer.java > 0752510 > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsLoader.java > 682349c > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsPartitioner.java > daa7fe2 > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsToDestroyer.java > 8bfd727 > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsToInitializer.java > e3d54b8 > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsUtils.java > PRE-CREATION > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/configuration/LinkConfig.java > PRE-CREATION > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/configuration/LinkConfiguration.java > PRE-CREATION > > connector/connector-hdfs/src/main/resources/hdfs-connector-config.properties > 90bc8bc > > connector/connector-hdfs/src/test/java/org/apache/sqoop/connector/hdfs/TestExtractor.java > 124c3df > > connector/connector-hdfs/src/test/java/org/apache/sqoop/connector/hdfs/TestHdfsUtils.java > PRE-CREATION > > connector/connector-hdfs/src/test/java/org/apache/sqoop/connector/hdfs/TestLoader.java > 8429e15 > > connector/connector-hdfs/src/test/java/org/apache/sqoop/connector/hdfs/TestPartitioner.java > bef1984 > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/FromHDFSToRDBMSTest.java > b1b3b16 > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/FromRDBMSToHDFSTest.java > 36f7443 > > Diff: https://reviews.apache.org/r/27110/diff/ > > > Testing > ------- > > mvn test and manually tested working order. > > > Thanks, > > Abraham Elmahrek > >
