> On Jan. 13, 2015, 9:27 p.m., Qian Xu wrote:
> > core/src/main/java/org/apache/sqoop/connector/ConnectorManagerUtils.java,
> > line 89
> > <https://reviews.apache.org/r/29773/diff/5/?file=818714#file818714line89>
> >
> > Although the varialbe `file` is defined, it seems not to be used
> > overall. There are still some usage of `listOfFiles[i]`
> >
> > I see you are using apache-commons, I'd suggest you use
> > `FileUtils.listFiles(...)`.
> >
> > Example:
> >
> > Collection<File> files = FileUtils.listFiles(
> > folder,
> > new RegexFileFilter("^(.*?).jar$"),
> > DirectoryFileFilter.DIRECTORY
> > );
> > for (File file : files) {
> > if (hasConnectorJar(file)) {
> > jarFiles.add(file);
> > }
> > }
> >
> > Doc:
> > http://commons.apache.org/proper/commons-io/apidocs/org/apache/commons/io/FileUtils.html#listFiles%28java.io.File,%20org.apache.commons.io.filefilter.IOFileFilter,%20org.apache.commons.io.filefilter.IOFileFilter%29
>
> Veena Basavaraj wrote:
> file is used in the else case., else the IDE would have complained. It
> still a nitpick, using one ot other, since we are already looking for
> connector jars with certain sqoopconnector.properties, using a regex seems is
> not adding much value, so I prefer to drop this at this point
1. It's totally okay not to use any listFile util.
2. If you declare a variable file, you should use it in both if and else block.
Furthermore, for loop over index is not required. Here is what I suggest.
for (File file : folder.listFiles()) {
if (file.isDirectory()) {
jarFiles.addAll(getConnectorJars(file.getPath()));
} else if (file.getName().endsWith(".jar") && hasConnectorJar(file)) {
jarFiles.add(file);
}
}
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29773/#review67843
-----------------------------------------------------------
On Jan. 13, 2015, 1:08 p.m., Veena Basavaraj wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29773/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2015, 1:08 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1821
> https://issues.apache.org/jira/browse/SQOOP-1821
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> see jira for details
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java b9d4d60
> core/src/main/java/org/apache/sqoop/connector/ConnectorManagerUtils.java
> c7193ee
> core/src/main/java/org/apache/sqoop/core/ConfigurationConstants.java
> f341108
>
> core/src/test/java/org/apache/sqoop/connector/TestConnectorManagerUtils.java
> PRE-CREATION
> core/src/test/resources/test-connector.jar PRE-CREATION
> core/src/test/resources/test-non-connector.jar PRE-CREATION
> dist/src/main/server/conf/sqoop.properties e22e8b0
>
> Diff: https://reviews.apache.org/r/29773/diff/
>
>
> Testing
> -------
>
> added unit tests as well
>
>
> Thanks,
>
> Veena Basavaraj
>
>
