sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Fero Szabo via Review Board <nore...@reviews.apache.org>
Subject Re: Review Request 64417: SQOOP-3153 Sqoop export with --as-<spec_file_format> error message could be more verbose
Date Thu, 14 Dec 2017 11:29:31 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message