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 Wed, 14 Jan 2015 06:17:55 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

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.


- 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