sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Veena Basavaraj" <vbasava...@cloudera.com>
Subject Re: Review Request 29848: bad smell: NPE check in SQOOP-1995 is not obvious
Date Thu, 15 Jan 2015 18:31:42 GMT


> On Jan. 13, 2015, 1:07 p.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.

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


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


On Jan. 13, 2015, 6:42 a.m., Qian Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29848/
> -----------------------------------------------------------
> 
> (Updated Jan. 13, 2015, 6:42 a.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