sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Qian Xu" <qian.a...@intel.com>
Subject Re: Review Request 29848: bad smell: NPE check in SQOOP-1995 is not obvious
Date Fri, 16 Jan 2015 01:19:37 GMT


> On Jan. 14, 2015, 5:07 a.m., Veena Basavaraj wrote:
> > connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/IntermediateDataFormat.java,
line 143
> > <https://reviews.apache.org/r/29848/diff/1/?file=818948#file818948line143>
> >
> >     any operation on schema can happen it is not just the getColumns, ? please move
it back to the methods so that is the main entry point.
> 
> Veena Basavaraj wrote:
>     also, bad smell seems a bit vague to use, the goal of adding validate method is to
prevent the emabrrasing NPE. so assiming it is only called in get columsn length method is
bad smell IMo
> 
> Qian Xu wrote:
>     I can move `validateSchema()` out of `getSchemaColumns()` and then put it before
`getSchemaColumns()`. 
>     
>     I dont think it is long-term benefitial to fix NPE in that way. Validate schema in
every getter and setter of data array does not make sense.
> 
> Veena Basavaraj wrote:
>     I am not sure what part you rfer to as not making sense, please have details.
>     
>     a schema is part of IDF and before the actualy IDF methods are invoked to set data,
it is important to validate that schema in not null. Give it some thought.
> 
> Veena Basavaraj wrote:
>     I really do not think it is good use of time for me to keep explaining the same thing
as I did, the reason why validate Schema ( which internally checks if schema is null and throws
exeption) is to make sure no part of the code is invoked without a valid schema, I have iterated
it many times that get columsn length is not the only method on schema, I am not sure what
other way I can make such a thing more clear to you :)

Okay, as you think it is the way to go, I'll discard the patch to your previous commit SQOOP-1995.


- Qian


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29848/#review67939
-----------------------------------------------------------


On Jan. 13, 2015, 10:42 p.m., Qian Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29848/
> -----------------------------------------------------------
> 
> (Updated Jan. 13, 2015, 10:42 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-2006
>     https://issues.apache.org/jira/browse/SQOOP-2006
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> This is a follow-up jira for SQOOP-1995
> 
> 
> Diffs
> -----
> 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/AVROIntermediateDataFormat.java
4d68ea0 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java
4870fae 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/IntermediateDataFormat.java
6f945c2 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/JSONIntermediateDataFormat.java
c8df6e0 
> 
> Diff: https://reviews.apache.org/r/29848/diff/
> 
> 
> Testing
> -------
> 
> Existing tests passed
> 
> 
> Thanks,
> 
> Qian Xu
> 
>


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