You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Veena Basavaraj <vb...@cloudera.com> on 2014/11/11 21:03:46 UTC
Review Request 27881: Sqoop2: Support null schema for the connector
intiializers
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27881/
-----------------------------------------------------------
Review request for Sqoop.
Bugs: SQOOP-1621
https://issues.apache.org/jira/browse/SQOOP-1621
Repository: sqoop-sqoop2
Description
-------
see jira
Diffs
-----
common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java e006761
connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsFromInitializer.java 4c6f566
connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsToInitializer.java bce72b5
execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/MRConfigurationUtils.java d5f74f0
execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestMRConfigurationUtils.java fbe3e7b
Diff: https://reviews.apache.org/r/27881/diff/
Testing
-------
yes
Thanks,
Veena Basavaraj
Re: Review Request 27881: Sqoop2: Support null schema for the
connector intiializers
Posted by Veena Basavaraj <vb...@cloudera.com>.
> On Nov. 11, 2014, 7 p.m., Qian Xu wrote:
> > common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java, line 72
> > <https://reviews.apache.org/r/27881/diff/5/?file=758636#file758636line72>
> >
> > The `Initializer` will return a `NullSchema` instance now, if no specific schema is provided. So null as schema will violate the design. As other methods do not have defensive check against null, is the check for schema necessary?
>
> Veena Basavaraj wrote:
> I am not sure I understand the violation, part, since I changed it to be a NullSchema class, this check is not required. I will remove it.
>
> can you clarify what you mean by violation
As a function, it is pretty good practice to not make assumptions on how this function will be called and who calls, AFAI have seen it is good coding practice to be be defensive and not cause embarassing NPE, in java it is pretty verbose if check. other languages are much succint.
One of the things I have noticed in all the Beans and its corr extract/ restor method is that it expects every field of that bean to be present. If I have to update a LINK name from a JSON field, and I have to POST a JSON I have to give every field that the extract methods expects, which is such a pain. I really did not have time to create a ticket for each and fix it:) I will someday. Try testing JSON apis with post data, you will see the pain.
- Veena
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27881/#review60935
-----------------------------------------------------------
On Nov. 11, 2014, 8:38 p.m., Veena Basavaraj wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27881/
> -----------------------------------------------------------
>
> (Updated Nov. 11, 2014, 8:38 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1621
> https://issues.apache.org/jira/browse/SQOOP-1621
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> see jira
>
> The point of this ticket is to guard the connector developers.
>
> Internally in Sqoop we can still create a empty Schema object and male sure we dont have to worry about guarding everyy single place a NPE for null schema can happen:)
>
> hence the code change is in the SchemaSerialization class so that we handle null schema
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java ecb5aa3
> common/src/main/java/org/apache/sqoop/schema/NullSchema.java PRE-CREATION
> common/src/test/java/org/apache/sqoop/json/util/TestSchemaSerialization.java 1640ead
> connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsFromInitializer.java 4c6f566
> connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsToInitializer.java bce72b5
> docs/src/site/sphinx/ConnectorDevelopment.rst 58b3b61
> execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/MRConfigurationUtils.java d5f74f0
> execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestMRConfigurationUtils.java fbe3e7b
> spi/src/main/java/org/apache/sqoop/job/etl/Initializer.java d66b099
>
> Diff: https://reviews.apache.org/r/27881/diff/
>
>
> Testing
> -------
>
> yes ( unit and integration )
>
>
> Thanks,
>
> Veena Basavaraj
>
>
Re: Review Request 27881: Sqoop2: Support null schema for the
connector intiializers
Posted by Veena Basavaraj <vb...@cloudera.com>.
> On Nov. 11, 2014, 7 p.m., Qian Xu wrote:
> > common/src/main/java/org/apache/sqoop/schema/NullSchema.java, line 24
> > <https://reviews.apache.org/r/27881/diff/5/?file=758637#file758637line24>
> >
> > I hate to add complexity, but `NullSchema` should be immutable, right? All setter methods should be hide. Maybe singleton implementation as well?
>
> Veena Basavaraj wrote:
> sure.
I just used this aa reference, http://en.wikipedia.org/wiki/Null_Object_pattern#Java it is not mandated to have a singleton. We use this aa default case, I am not sure when would someone use this class for non null use cases
http://refactoring.com/catalog/introduceNullObject.html
- Veena
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27881/#review60935
-----------------------------------------------------------
On Nov. 11, 2014, 4:45 p.m., Veena Basavaraj wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27881/
> -----------------------------------------------------------
>
> (Updated Nov. 11, 2014, 4:45 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1621
> https://issues.apache.org/jira/browse/SQOOP-1621
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> see jira
>
> The point of this ticket is to guard the connector developers.
>
> Internally in Sqoop we can still create a empty Schema object and male sure we dont have to worry about guarding everyy single place a NPE for null schema can happen:)
>
> hence the code change is in the SchemaSerialization class so that we handle null schema
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java ecb5aa3
> common/src/main/java/org/apache/sqoop/schema/NullSchema.java PRE-CREATION
> connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsFromInitializer.java 4c6f566
> connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsToInitializer.java bce72b5
> docs/src/site/sphinx/ConnectorDevelopment.rst 58b3b61
> execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/MRConfigurationUtils.java d5f74f0
> execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestMRConfigurationUtils.java fbe3e7b
> spi/src/main/java/org/apache/sqoop/job/etl/Initializer.java d66b099
>
> Diff: https://reviews.apache.org/r/27881/diff/
>
>
> Testing
> -------
>
> yes ( unit and integration )
>
>
> Thanks,
>
> Veena Basavaraj
>
>
Re: Review Request 27881: Sqoop2: Support null schema for the
connector intiializers
Posted by Veena Basavaraj <vb...@cloudera.com>.
> On Nov. 11, 2014, 7 p.m., Qian Xu wrote:
> > common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java, line 92
> > <https://reviews.apache.org/r/27881/diff/5/?file=758636#file758636line92>
> >
> > Add one blank line please
this is just personal preference, lets not pick on these things.
- Veena
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27881/#review60935
-----------------------------------------------------------
On Nov. 11, 2014, 4:45 p.m., Veena Basavaraj wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27881/
> -----------------------------------------------------------
>
> (Updated Nov. 11, 2014, 4:45 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1621
> https://issues.apache.org/jira/browse/SQOOP-1621
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> see jira
>
> The point of this ticket is to guard the connector developers.
>
> Internally in Sqoop we can still create a empty Schema object and male sure we dont have to worry about guarding everyy single place a NPE for null schema can happen:)
>
> hence the code change is in the SchemaSerialization class so that we handle null schema
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java ecb5aa3
> common/src/main/java/org/apache/sqoop/schema/NullSchema.java PRE-CREATION
> connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsFromInitializer.java 4c6f566
> connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsToInitializer.java bce72b5
> docs/src/site/sphinx/ConnectorDevelopment.rst 58b3b61
> execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/MRConfigurationUtils.java d5f74f0
> execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestMRConfigurationUtils.java fbe3e7b
> spi/src/main/java/org/apache/sqoop/job/etl/Initializer.java d66b099
>
> Diff: https://reviews.apache.org/r/27881/diff/
>
>
> Testing
> -------
>
> yes ( unit and integration )
>
>
> Thanks,
>
> Veena Basavaraj
>
>
Re: Review Request 27881: Sqoop2: Support null schema for the
connector intiializers
Posted by Veena Basavaraj <vb...@cloudera.com>.
> On Nov. 11, 2014, 7 p.m., Qian Xu wrote:
> > common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java, line 96
> > <https://reviews.apache.org/r/27881/diff/5/?file=758636#file758636line96>
> >
> > Please keep setter chain in multiple lines
I just use the standard we have, unless we have a standardised rules, I rather not pick on such things.
> On Nov. 11, 2014, 7 p.m., Qian Xu wrote:
> > common/src/main/java/org/apache/sqoop/schema/NullSchema.java, line 24
> > <https://reviews.apache.org/r/27881/diff/5/?file=758637#file758637line24>
> >
> > I hate to add complexity, but `NullSchema` should be immutable, right? All setter methods should be hide. Maybe singleton implementation as well?
sure.
> On Nov. 11, 2014, 7 p.m., Qian Xu wrote:
> > common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java, line 72
> > <https://reviews.apache.org/r/27881/diff/5/?file=758636#file758636line72>
> >
> > The `Initializer` will return a `NullSchema` instance now, if no specific schema is provided. So null as schema will violate the design. As other methods do not have defensive check against null, is the check for schema necessary?
I am not sure I understand the violation, part, since I changed it to be a NullSchema class, this check is not required. I will remove it.
can you clarify what you mean by violation
- Veena
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27881/#review60935
-----------------------------------------------------------
On Nov. 11, 2014, 4:45 p.m., Veena Basavaraj wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27881/
> -----------------------------------------------------------
>
> (Updated Nov. 11, 2014, 4:45 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1621
> https://issues.apache.org/jira/browse/SQOOP-1621
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> see jira
>
> The point of this ticket is to guard the connector developers.
>
> Internally in Sqoop we can still create a empty Schema object and male sure we dont have to worry about guarding everyy single place a NPE for null schema can happen:)
>
> hence the code change is in the SchemaSerialization class so that we handle null schema
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java ecb5aa3
> common/src/main/java/org/apache/sqoop/schema/NullSchema.java PRE-CREATION
> connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsFromInitializer.java 4c6f566
> connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsToInitializer.java bce72b5
> docs/src/site/sphinx/ConnectorDevelopment.rst 58b3b61
> execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/MRConfigurationUtils.java d5f74f0
> execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestMRConfigurationUtils.java fbe3e7b
> spi/src/main/java/org/apache/sqoop/job/etl/Initializer.java d66b099
>
> Diff: https://reviews.apache.org/r/27881/diff/
>
>
> Testing
> -------
>
> yes ( unit and integration )
>
>
> Thanks,
>
> Veena Basavaraj
>
>
Re: Review Request 27881: Sqoop2: Support null schema for the
connector intiializers
Posted by Qian Xu <sx...@googlemail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27881/#review60935
-----------------------------------------------------------
common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java
<https://reviews.apache.org/r/27881/#comment102352>
The `Initializer` will return a `NullSchema` instance now, if no specific schema is provided. So null as schema will violate the design. As other methods do not have defensive check against null, is the check for schema necessary?
common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java
<https://reviews.apache.org/r/27881/#comment102353>
Add one blank line please
common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java
<https://reviews.apache.org/r/27881/#comment102356>
Please keep setter chain in multiple lines
common/src/main/java/org/apache/sqoop/schema/NullSchema.java
<https://reviews.apache.org/r/27881/#comment102357>
I hate to add complexity, but `NullSchema` should be immutable, right? All setter methods should be hide. Maybe singleton implementation as well?
- Qian Xu
On Nov. 12, 2014, 8:45 a.m., Veena Basavaraj wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27881/
> -----------------------------------------------------------
>
> (Updated Nov. 12, 2014, 8:45 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1621
> https://issues.apache.org/jira/browse/SQOOP-1621
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> see jira
>
> The point of this ticket is to guard the connector developers.
>
> Internally in Sqoop we can still create a empty Schema object and male sure we dont have to worry about guarding everyy single place a NPE for null schema can happen:)
>
> hence the code change is in the SchemaSerialization class so that we handle null schema
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java ecb5aa3
> common/src/main/java/org/apache/sqoop/schema/NullSchema.java PRE-CREATION
> connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsFromInitializer.java 4c6f566
> connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsToInitializer.java bce72b5
> docs/src/site/sphinx/ConnectorDevelopment.rst 58b3b61
> execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/MRConfigurationUtils.java d5f74f0
> execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestMRConfigurationUtils.java fbe3e7b
> spi/src/main/java/org/apache/sqoop/job/etl/Initializer.java d66b099
>
> Diff: https://reviews.apache.org/r/27881/diff/
>
>
> Testing
> -------
>
> yes ( unit and integration )
>
>
> Thanks,
>
> Veena Basavaraj
>
>
Re: Review Request 27881: Sqoop2: Support null schema for the
connector intiializers
Posted by Abraham Elmahrek <ab...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27881/#review61174
-----------------------------------------------------------
Ship it!
Normal white space comment
- Abraham Elmahrek
On Nov. 12, 2014, 5:12 a.m., Veena Basavaraj wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27881/
> -----------------------------------------------------------
>
> (Updated Nov. 12, 2014, 5:12 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1621
> https://issues.apache.org/jira/browse/SQOOP-1621
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> see jira
>
> The point of this ticket is to guard the connector developers.
>
> Internally in Sqoop we can still create a empty Schema object and male sure we dont have to worry about guarding everyy single place a NPE for null schema can happen:)
>
> hence the code change is in the SchemaSerialization class so that we handle null schema
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java ecb5aa3
> common/src/main/java/org/apache/sqoop/schema/NullSchema.java PRE-CREATION
> common/src/test/java/org/apache/sqoop/json/util/TestSchemaSerialization.java 1640ead
> connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsFromInitializer.java 4c6f566
> connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsToInitializer.java bce72b5
> docs/src/site/sphinx/ConnectorDevelopment.rst 58b3b61
> execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/MRConfigurationUtils.java d5f74f0
> execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestMRConfigurationUtils.java fbe3e7b
> spi/src/main/java/org/apache/sqoop/job/etl/Initializer.java d66b099
>
> Diff: https://reviews.apache.org/r/27881/diff/
>
>
> Testing
> -------
>
> yes ( unit and integration )
>
>
> Thanks,
>
> Veena Basavaraj
>
>
Re: Review Request 27881: Sqoop2: Support null schema for the
connector intiializers
Posted by Abraham Elmahrek <ab...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27881/#review61173
-----------------------------------------------------------
Ship it!
Normal white space comment
- Abraham Elmahrek
On Nov. 12, 2014, 5:12 a.m., Veena Basavaraj wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27881/
> -----------------------------------------------------------
>
> (Updated Nov. 12, 2014, 5:12 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1621
> https://issues.apache.org/jira/browse/SQOOP-1621
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> see jira
>
> The point of this ticket is to guard the connector developers.
>
> Internally in Sqoop we can still create a empty Schema object and male sure we dont have to worry about guarding everyy single place a NPE for null schema can happen:)
>
> hence the code change is in the SchemaSerialization class so that we handle null schema
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java ecb5aa3
> common/src/main/java/org/apache/sqoop/schema/NullSchema.java PRE-CREATION
> common/src/test/java/org/apache/sqoop/json/util/TestSchemaSerialization.java 1640ead
> connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsFromInitializer.java 4c6f566
> connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsToInitializer.java bce72b5
> docs/src/site/sphinx/ConnectorDevelopment.rst 58b3b61
> execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/MRConfigurationUtils.java d5f74f0
> execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestMRConfigurationUtils.java fbe3e7b
> spi/src/main/java/org/apache/sqoop/job/etl/Initializer.java d66b099
>
> Diff: https://reviews.apache.org/r/27881/diff/
>
>
> Testing
> -------
>
> yes ( unit and integration )
>
>
> Thanks,
>
> Veena Basavaraj
>
>
Re: Review Request 27881: Sqoop2: Support null schema for the
connector intiializers
Posted by Veena Basavaraj <vb...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27881/
-----------------------------------------------------------
(Updated Nov. 12, 2014, 5:58 p.m.)
Review request for Sqoop.
Changes
-------
remove the infamous WS
Bugs: SQOOP-1621
https://issues.apache.org/jira/browse/SQOOP-1621
Repository: sqoop-sqoop2
Description
-------
see jira
The point of this ticket is to guard the connector developers.
Internally in Sqoop we can still create a empty Schema object and male sure we dont have to worry about guarding everyy single place a NPE for null schema can happen:)
hence the code change is in the SchemaSerialization class so that we handle null schema
Diffs (updated)
-----
common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java ecb5aa3
common/src/main/java/org/apache/sqoop/schema/NullSchema.java PRE-CREATION
common/src/test/java/org/apache/sqoop/json/util/TestSchemaSerialization.java 1640ead
connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsFromInitializer.java 4c6f566
connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsToInitializer.java bce72b5
docs/src/site/sphinx/ConnectorDevelopment.rst 58b3b61
execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/MRConfigurationUtils.java d5f74f0
execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestMRConfigurationUtils.java fbe3e7b
spi/src/main/java/org/apache/sqoop/job/etl/Initializer.java d66b099
Diff: https://reviews.apache.org/r/27881/diff/
Testing
-------
yes ( unit and integration )
Thanks,
Veena Basavaraj
Re: Review Request 27881: Sqoop2: Support null schema for the
connector intiializers
Posted by Veena Basavaraj <vb...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27881/
-----------------------------------------------------------
(Updated Nov. 11, 2014, 9:12 p.m.)
Review request for Sqoop.
Bugs: SQOOP-1621
https://issues.apache.org/jira/browse/SQOOP-1621
Repository: sqoop-sqoop2
Description
-------
see jira
The point of this ticket is to guard the connector developers.
Internally in Sqoop we can still create a empty Schema object and male sure we dont have to worry about guarding everyy single place a NPE for null schema can happen:)
hence the code change is in the SchemaSerialization class so that we handle null schema
Diffs (updated)
-----
common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java ecb5aa3
common/src/main/java/org/apache/sqoop/schema/NullSchema.java PRE-CREATION
common/src/test/java/org/apache/sqoop/json/util/TestSchemaSerialization.java 1640ead
connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsFromInitializer.java 4c6f566
connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsToInitializer.java bce72b5
docs/src/site/sphinx/ConnectorDevelopment.rst 58b3b61
execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/MRConfigurationUtils.java d5f74f0
execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestMRConfigurationUtils.java fbe3e7b
spi/src/main/java/org/apache/sqoop/job/etl/Initializer.java d66b099
Diff: https://reviews.apache.org/r/27881/diff/
Testing
-------
yes ( unit and integration )
Thanks,
Veena Basavaraj
Re: Review Request 27881: Sqoop2: Support null schema for the
connector intiializers
Posted by Veena Basavaraj <vb...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27881/
-----------------------------------------------------------
(Updated Nov. 11, 2014, 8:38 p.m.)
Review request for Sqoop.
Bugs: SQOOP-1621
https://issues.apache.org/jira/browse/SQOOP-1621
Repository: sqoop-sqoop2
Description
-------
see jira
The point of this ticket is to guard the connector developers.
Internally in Sqoop we can still create a empty Schema object and male sure we dont have to worry about guarding everyy single place a NPE for null schema can happen:)
hence the code change is in the SchemaSerialization class so that we handle null schema
Diffs (updated)
-----
common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java ecb5aa3
common/src/main/java/org/apache/sqoop/schema/NullSchema.java PRE-CREATION
common/src/test/java/org/apache/sqoop/json/util/TestSchemaSerialization.java 1640ead
connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsFromInitializer.java 4c6f566
connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsToInitializer.java bce72b5
docs/src/site/sphinx/ConnectorDevelopment.rst 58b3b61
execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/MRConfigurationUtils.java d5f74f0
execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestMRConfigurationUtils.java fbe3e7b
spi/src/main/java/org/apache/sqoop/job/etl/Initializer.java d66b099
Diff: https://reviews.apache.org/r/27881/diff/
Testing
-------
yes ( unit and integration )
Thanks,
Veena Basavaraj
Re: Review Request 27881: Sqoop2: Support null schema for the
connector intiializers
Posted by Veena Basavaraj <vb...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27881/
-----------------------------------------------------------
(Updated Nov. 11, 2014, 4:45 p.m.)
Review request for Sqoop.
Changes
-------
fix bad merge
Bugs: SQOOP-1621
https://issues.apache.org/jira/browse/SQOOP-1621
Repository: sqoop-sqoop2
Description
-------
see jira
The point of this ticket is to guard the connector developers.
Internally in Sqoop we can still create a empty Schema object and male sure we dont have to worry about guarding everyy single place a NPE for null schema can happen:)
hence the code change is in the SchemaSerialization class so that we handle null schema
Diffs (updated)
-----
common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java ecb5aa3
common/src/main/java/org/apache/sqoop/schema/NullSchema.java PRE-CREATION
connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsFromInitializer.java 4c6f566
connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsToInitializer.java bce72b5
docs/src/site/sphinx/ConnectorDevelopment.rst 58b3b61
execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/MRConfigurationUtils.java d5f74f0
execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestMRConfigurationUtils.java fbe3e7b
spi/src/main/java/org/apache/sqoop/job/etl/Initializer.java d66b099
Diff: https://reviews.apache.org/r/27881/diff/
Testing
-------
yes ( unit and integration )
Thanks,
Veena Basavaraj
Re: Review Request 27881: Sqoop2: Support null schema for the
connector intiializers
Posted by Veena Basavaraj <vb...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27881/
-----------------------------------------------------------
(Updated Nov. 11, 2014, 4:32 p.m.)
Review request for Sqoop.
Changes
-------
after rebase
Bugs: SQOOP-1621
https://issues.apache.org/jira/browse/SQOOP-1621
Repository: sqoop-sqoop2
Description
-------
see jira
The point of this ticket is to guard the connector developers.
Internally in Sqoop we can still create a empty Schema object and male sure we dont have to worry about guarding everyy single place a NPE for null schema can happen:)
hence the code change is in the SchemaSerialization class so that we handle null schema
Diffs (updated)
-----
common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java ecb5aa3
common/src/main/java/org/apache/sqoop/schema/NullSchema.java PRE-CREATION
connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsFromInitializer.java 4c6f566
connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsToInitializer.java bce72b5
docs/src/site/sphinx/ConnectorDevelopment.rst 58b3b61
execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/MRConfigurationUtils.java d5f74f0
execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestMRConfigurationUtils.java fbe3e7b
spi/src/main/java/org/apache/sqoop/job/etl/Initializer.java d66b099
Diff: https://reviews.apache.org/r/27881/diff/
Testing
-------
yes ( unit and integration )
Thanks,
Veena Basavaraj
Re: Review Request 27881: Sqoop2: Support null schema for the
connector intiializers
Posted by Veena Basavaraj <vb...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27881/
-----------------------------------------------------------
(Updated Nov. 11, 2014, 2:24 p.m.)
Review request for Sqoop.
Bugs: SQOOP-1621
https://issues.apache.org/jira/browse/SQOOP-1621
Repository: sqoop-sqoop2
Description
-------
see jira
The point of this ticket is to guard the connector developers.
Internally in Sqoop we can still create a empty Schema object and male sure we dont have to worry about guarding everyy single place a NPE for null schema can happen:)
hence the code change is in the SchemaSerialization class so that we handle null schema
Diffs (updated)
-----
common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java e006761
common/src/main/java/org/apache/sqoop/schema/NullSchema.java PRE-CREATION
connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsFromInitializer.java 4c6f566
connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsToInitializer.java bce72b5
docs/src/site/sphinx/ConnectorDevelopment.rst 58b3b61
execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/MRConfigurationUtils.java d5f74f0
execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestMRConfigurationUtils.java fbe3e7b
spi/src/main/java/org/apache/sqoop/job/etl/Initializer.java d66b099
Diff: https://reviews.apache.org/r/27881/diff/
Testing
-------
yes ( unit and integration )
Thanks,
Veena Basavaraj
Re: Review Request 27881: Sqoop2: Support null schema for the
connector intiializers
Posted by Veena Basavaraj <vb...@cloudera.com>.
> On Nov. 11, 2014, 2:05 p.m., Gwen Shapira wrote:
> > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsFromInitializer.java, line 46
> > <https://reviews.apache.org/r/27881/diff/1/?file=758226#file758226line46>
> >
> > Even though I agree we should support nulls, I prefer to return empty schemas where we control the code.
> >
> > Do you see an issue with that?
Not against anyone returning an new Schema, but if we have more connectors written, it might as well be default returning null if there is no schema. I have made this change to have atleast the integration tests pass
Let me update the docs that this can be null, second make the base class methods default return null, so that it is not mandatory to override this.
I can revert the HDFS then to return this if you like it to be that way.
- Veena
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27881/#review60864
-----------------------------------------------------------
On Nov. 11, 2014, 2:04 p.m., Veena Basavaraj wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27881/
> -----------------------------------------------------------
>
> (Updated Nov. 11, 2014, 2:04 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1621
> https://issues.apache.org/jira/browse/SQOOP-1621
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> see jira
>
> The point of this ticket is to guard the connector developers.
>
> Internally in Sqoop we can still create a empty Schema object and male sure we dont have to worry about guarding everyy single place a NPE for null schema can happen:)
>
> hence the code change is in the SchemaSerialization class so that we handle null schema
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java e006761
> connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsFromInitializer.java 4c6f566
> connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsToInitializer.java bce72b5
> execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/MRConfigurationUtils.java d5f74f0
> execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestMRConfigurationUtils.java fbe3e7b
>
> Diff: https://reviews.apache.org/r/27881/diff/
>
>
> Testing
> -------
>
> yes ( unit and integration )
>
>
> Thanks,
>
> Veena Basavaraj
>
>
Re: Review Request 27881: Sqoop2: Support null schema for the
connector intiializers
Posted by Gwen Shapira <gs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27881/#review60864
-----------------------------------------------------------
connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsFromInitializer.java
<https://reviews.apache.org/r/27881/#comment102269>
Even though I agree we should support nulls, I prefer to return empty schemas where we control the code.
Do you see an issue with that?
- Gwen Shapira
On Nov. 11, 2014, 10:04 p.m., Veena Basavaraj wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27881/
> -----------------------------------------------------------
>
> (Updated Nov. 11, 2014, 10:04 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1621
> https://issues.apache.org/jira/browse/SQOOP-1621
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> see jira
>
> The point of this ticket is to guard the connector developers.
>
> Internally in Sqoop we can still create a empty Schema object and male sure we dont have to worry about guarding everyy single place a NPE for null schema can happen:)
>
> hence the code change is in the SchemaSerialization class so that we handle null schema
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java e006761
> connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsFromInitializer.java 4c6f566
> connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsToInitializer.java bce72b5
> execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/MRConfigurationUtils.java d5f74f0
> execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestMRConfigurationUtils.java fbe3e7b
>
> Diff: https://reviews.apache.org/r/27881/diff/
>
>
> Testing
> -------
>
> yes ( unit and integration )
>
>
> Thanks,
>
> Veena Basavaraj
>
>
Re: Review Request 27881: Sqoop2: Support null schema for the
connector intiializers
Posted by Veena Basavaraj <vb...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27881/
-----------------------------------------------------------
(Updated Nov. 11, 2014, 2:04 p.m.)
Review request for Sqoop.
Bugs: SQOOP-1621
https://issues.apache.org/jira/browse/SQOOP-1621
Repository: sqoop-sqoop2
Description
-------
see jira
The point of this ticket is to guard the connector developers.
Internally in Sqoop we can still create a empty Schema object and male sure we dont have to worry about guarding everyy single place a NPE for null schema can happen:)
hence the code change is in the SchemaSerialization class so that we handle null schema
Diffs (updated)
-----
common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java e006761
connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsFromInitializer.java 4c6f566
connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsToInitializer.java bce72b5
execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/MRConfigurationUtils.java d5f74f0
execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestMRConfigurationUtils.java fbe3e7b
Diff: https://reviews.apache.org/r/27881/diff/
Testing (updated)
-------
yes ( unit and integration )
Thanks,
Veena Basavaraj
Re: Review Request 27881: Sqoop2: Support null schema for the
connector intiializers
Posted by Veena Basavaraj <vb...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27881/
-----------------------------------------------------------
(Updated Nov. 11, 2014, 1:58 p.m.)
Review request for Sqoop.
Bugs: SQOOP-1621
https://issues.apache.org/jira/browse/SQOOP-1621
Repository: sqoop-sqoop2
Description (updated)
-------
see jira
The point of this ticket is to guard the connector developers.
Internally in Sqoop we can still create a empty Schema object and male sure we dont have to worry about guarding everyy single place a NPE for null schema can happen:)
hence the code change is in the SchemaSerialization class so that we handle null schema
Diffs
-----
common/src/main/java/org/apache/sqoop/json/util/SchemaSerialization.java e006761
connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsFromInitializer.java 4c6f566
connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsToInitializer.java bce72b5
execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/MRConfigurationUtils.java d5f74f0
execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestMRConfigurationUtils.java fbe3e7b
Diff: https://reviews.apache.org/r/27881/diff/
Testing
-------
yes
Thanks,
Veena Basavaraj