> On March 18, 2015, 9:28 p.m., Abraham Elmahrek wrote: > > Might be cool to make this configurable for HDFS encryption/ACLs?
I would prefer not to make it configurable but instead always write to a subdirectory of target directory because that will solve the issue with HDFS encryption/ACLs. I'm open to do it if we feel that it will be required. > On March 18, 2015, 9:28 p.m., Abraham Elmahrek wrote: > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsToDestroyer.java, > > lines 58-62 > > <https://reviews.apache.org/r/32216/diff/1/?file=899373#file899373line58> > > > > Why not move entire directory? instead of file at a time. Good question. There are several reasons why I've decided to go route of moving one file at the time: * The working directory is subdirectory of the target directory (e.g. I can't move entire directory to it's parent) * Also this is prepared for incremental import where I will need to append files to existing directory > On March 18, 2015, 9:28 p.m., Abraham Elmahrek wrote: > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsToDestroyer.java, > > line 60 > > <https://reviews.apache.org/r/32216/diff/1/?file=899373#file899373line60> > > > > Debug maybe. This code runs on mapreduce side where it's not "that easy" to apply our log4j configuration to enable debug mode. I would prefer to keep it as info, so that it will always appear in the logs. > On March 18, 2015, 9:28 p.m., Abraham Elmahrek wrote: > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsToInitializer.java, > > lines 50-53 > > <https://reviews.apache.org/r/32216/diff/1/?file=899374#file899374line50> > > > > Not opposed to it... but these probably can be removed since they're > > only useful for debugging It's a good reminder of what conditions we're expecting to be true as they are checked elsewhere. I've started adding it to other places as well - I think that it will help when multiple developpers will look at the same file to realize that we're simply assuming those conditions without any additional checks. - Jarek ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32216/#review76949 ----------------------------------------------------------- On March 18, 2015, 8:48 p.m., Jarek Cecho wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/32216/ > ----------------------------------------------------------- > > (Updated March 18, 2015, 8:48 p.m.) > > > Review request for Sqoop. > > > Repository: sqoop-sqoop2 > > > Description > ------- > > Implemented as per description in JIRA. > > > Diffs > ----- > > common/src/main/java/org/apache/sqoop/error/code/HdfsConnectorError.java > c85e7fc > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsConstants.java > bd74bec > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsLoader.java > 0ced6d0 > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsToDestroyer.java > 3c85be8 > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsToInitializer.java > 83bac27 > > connector/connector-hdfs/src/test/java/org/apache/sqoop/connector/hdfs/TestLoader.java > b7c81ec > > connector/connector-hdfs/src/test/java/org/apache/sqoop/connector/hdfs/TestToDestroyer.java > PRE-CREATION > > connector/connector-hdfs/src/test/java/org/apache/sqoop/connector/hdfs/TestToInitializer.java > 1daa25a > > Diff: https://reviews.apache.org/r/32216/diff/ > > > Testing > ------- > > I've added unit tests for new functionality. Existing integration tests > passes without any changes (which is a positive thing here). > > > Thanks, > > Jarek Cecho > >
