> On Dec. 13, 2017, 9:56 a.m., Szabolcs Vasas wrote: > > src/java/org/apache/sqoop/tool/ExportTool.java > > Lines 22 (patched) > > <https://reviews.apache.org/r/64417/diff/4/?file=1911620#file1911620line22> > > > > This import is unused.
Removed. I've turned on the 'Optimize imports on the fly' option in my IDE as well, that should prevent this from happening in the future. > On Dec. 13, 2017, 9:56 a.m., Szabolcs Vasas wrote: > > src/java/org/apache/sqoop/tool/ExportTool.java > > Lines 51 (patched) > > <https://reviews.apache.org/r/64417/diff/4/?file=1911620#file1911620line51> > > > > Arrays.asList returns a list which is unmodifiable so > > Collections.unmodifiableList is redundant. Are you sure? That is not what I found here... https://docs.oracle.com/javase/7/docs/api/java/util/Arrays.html#asList(T...) I see that most operations (add, remove) are unsupported, but the set(int index, String value) function still works, so I'd like to leave this as is. > On Dec. 13, 2017, 9:56 a.m., Szabolcs Vasas wrote: > > src/java/org/apache/sqoop/tool/ExportTool.java > > Lines 403 (patched) > > <https://reviews.apache.org/r/64417/diff/4/?file=1911620#file1911620line403> > > > > Do we need to convert the array to a list? Nope, removed. > On Dec. 13, 2017, 9:56 a.m., Szabolcs Vasas wrote: > > src/java/org/apache/sqoop/tool/ExportTool.java > > Lines 406 (patched) > > <https://reviews.apache.org/r/64417/diff/4/?file=1911620#file1911620line406> > > > > I think you could make this condition more readable by introducing a > > method like org.apache.commons.cli.Util#stripLeadingHyphens. Done. - Fero ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64417/#review193657 ----------------------------------------------------------- On Dec. 13, 2017, 11:19 a.m., Fero Szabo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64417/ > ----------------------------------------------------------- > > (Updated Dec. 13, 2017, 11:19 a.m.) > > > Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas. > > > Bugs: SQOOP-3153 > https://issues.apache.org/jira/browse/SQOOP-3153 > > > Repository: sqoop-trunk > > > Description > ------- > > The errormessages are generated by the hasUnrecognizedArgs funciton, which is > located in the base class of every Tool: BaseSqoopTool. > > I overrided the hasUnrecognizedArgs function, in the subclass (i.e. the > ExportTool class), this calls the original function first and then checks > whether the file formats were specified and prints an additional hint / error > message. > > Example output: > 2017-12-07 16:02:42,640 ERROR [main] tool.BaseSqoopTool > (BaseSqoopTool.java:hasUnrecognizedArgs(335)) - Error parsing arguments for > export: > 2017-12-07 16:02:42,643 ERROR [main] tool.BaseSqoopTool > (BaseSqoopTool.java:hasUnrecognizedArgs(338)) - Unrecognized argument: > --as-parquetfile > 2017-12-07 16:02:42,643 ERROR [main] tool.ExportTool > (ExportTool.java:hasUnrecognizedArgs(405)) - Please note that the export tool > detects the file format automatically and does not support it as an argument: > --as-parquetfile > > Also, corrected a typo in the function javadoc. > > Concerns: > - Please let me know if a different message would provide more clarity. > - The intention was to implement the decorator pattern here, please feel free > to point it out if a different, simpler design would make more sense. > > > Diffs > ----- > > src/java/org/apache/sqoop/tool/BaseSqoopTool.java 6a4dcb09 > src/java/org/apache/sqoop/tool/ExportTool.java cd6cdf32 > > > Diff: https://reviews.apache.org/r/64417/diff/5/ > > > Testing > ------- > > The following tests passed: > - Unit tests > - 3rd party integration tests > > I haven't added a new test case since testHCatExportWithParquetFile in the > TestHCatalogBasic class already covers the code path and didn't require > modification. > > > Thanks, > > Fero Szabo > >
