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

Review request for hadoop-common and Todd Lipcon.


Summary
-------

Since getSerialization() can possibly return a null, it is only right that getSerializer() and getDeserializer() usage functions do the same, instead of throwing up NPEs.

Related issue to which this improvement is required: https://issues.apache.org/jira/browse/MAPREDUCE-2584


This addresses bug HADOOP-7328.
http://issues.apache.org/jira/browse/HADOOP-7328


Diffs
-----

src/java/org/apache/hadoop/io/serializer/SerializationFactory.java dee314a

Diff: https://reviews.apache.org/r/884/diff


Testing
-------

Existing SequenceFile serialization factory tests pass. The change is merely to make the functions return null instead of throwing an NPE within.


Thanks,

Harsh

Search Discussions

  • Todd Lipcon at Jun 12, 2011 at 2:24 am
    -----------------------------------------------------------
    This is an automatically generated e-mail. To reply, visit:
    https://reviews.apache.org/r/884/#review805
    -----------------------------------------------------------

    Ship it!


    Looks good to me. Can you upload this rev of the patch to the JIRA so the QA Bot runs on it?

    - Todd

    On 2011-06-11 22:10:17, Harsh Chouraria wrote:

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

    (Updated 2011-06-11 22:10:17)


    Review request for hadoop-common and Todd Lipcon.


    Summary
    -------

    Since getSerialization() can possibly return a null, it is only right that getSerializer() and getDeserializer() usage functions do the same, instead of throwing up NPEs.

    Related issue to which this improvement is required: https://issues.apache.org/jira/browse/MAPREDUCE-2584


    This addresses bug HADOOP-7328.
    http://issues.apache.org/jira/browse/HADOOP-7328


    Diffs
    -----

    src/java/org/apache/hadoop/io/serializer/SerializationFactory.java dee314a

    Diff: https://reviews.apache.org/r/884/diff


    Testing
    -------

    Existing SequenceFile serialization factory tests pass. The change is merely to make the functions return null instead of throwing an NPE within.


    Thanks,

    Harsh
  • Harsh J at Jun 12, 2011 at 4:43 am

    On 2011-06-12 02:24:55, Todd Lipcon wrote:
    Looks good to me. Can you upload this rev of the patch to the JIRA so the QA Bot runs on it?
    Submitted on JIRA. Thanks for the review Todd!


    - Harsh


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

    On 2011-06-11 22:10:17, Harsh J wrote:

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

    (Updated 2011-06-11 22:10:17)


    Review request for hadoop-common and Todd Lipcon.


    Summary
    -------

    Since getSerialization() can possibly return a null, it is only right that getSerializer() and getDeserializer() usage functions do the same, instead of throwing up NPEs.

    Related issue to which this improvement is required: https://issues.apache.org/jira/browse/MAPREDUCE-2584


    This addresses bug HADOOP-7328.
    http://issues.apache.org/jira/browse/HADOOP-7328


    Diffs
    -----

    src/java/org/apache/hadoop/io/serializer/SerializationFactory.java dee314a

    Diff: https://reviews.apache.org/r/884/diff


    Testing
    -------

    Existing SequenceFile serialization factory tests pass. The change is merely to make the functions return null instead of throwing an NPE within.


    Thanks,

    Harsh
  • Matt Foley at Jun 13, 2011 at 6:23 pm
    -----------------------------------------------------------
    This is an automatically generated e-mail. To reply, visit:
    https://reviews.apache.org/r/884/#review817
    -----------------------------------------------------------


    Sorry if this is out of context, but is it really best to also return a null here? Shouldn't it check for null result from getSerialization(), then throw a (non-NPE) exception? Or do you prefer to do that check and throw at a higher level of the code?

    - Matt

    On 2011-06-11 22:10:17, Harsh J wrote:

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

    (Updated 2011-06-11 22:10:17)


    Review request for hadoop-common and Todd Lipcon.


    Summary
    -------

    Since getSerialization() can possibly return a null, it is only right that getSerializer() and getDeserializer() usage functions do the same, instead of throwing up NPEs.

    Related issue to which this improvement is required: https://issues.apache.org/jira/browse/MAPREDUCE-2584


    This addresses bug HADOOP-7328.
    http://issues.apache.org/jira/browse/HADOOP-7328


    Diffs
    -----

    src/java/org/apache/hadoop/io/serializer/SerializationFactory.java dee314a

    Diff: https://reviews.apache.org/r/884/diff


    Testing
    -------

    Existing SequenceFile serialization factory tests pass. The change is merely to make the functions return null instead of throwing an NPE within.


    Thanks,

    Harsh
  • Todd Lipcon at Jun 13, 2011 at 6:30 pm

    On 2011-06-13 18:23:35, Matt Foley wrote:
    Sorry if this is out of context, but is it really best to also return a null here? Shouldn't it check for null result from getSerialization(), then throw a (non-NPE) exception? Or do you prefer to do that check and throw at a higher level of the code?
    I thought about this while doing the review... my thinking was that our other similar APIs do return null -eg CompressionCodecFactory.getCodecByName() returns null if the specified codec isn't found. This API is also marked as LimitedPrivate Evolving so it shouldn't be a problematic breaking change.

    Maybe we should add a bit of javadoc saying "@returns null if no serializer is known for the given class." ?


    - Todd


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

    On 2011-06-11 22:10:17, Harsh J wrote:

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

    (Updated 2011-06-11 22:10:17)


    Review request for hadoop-common and Todd Lipcon.


    Summary
    -------

    Since getSerialization() can possibly return a null, it is only right that getSerializer() and getDeserializer() usage functions do the same, instead of throwing up NPEs.

    Related issue to which this improvement is required: https://issues.apache.org/jira/browse/MAPREDUCE-2584


    This addresses bug HADOOP-7328.
    http://issues.apache.org/jira/browse/HADOOP-7328


    Diffs
    -----

    src/java/org/apache/hadoop/io/serializer/SerializationFactory.java dee314a

    Diff: https://reviews.apache.org/r/884/diff


    Testing
    -------

    Existing SequenceFile serialization factory tests pass. The change is merely to make the functions return null instead of throwing an NPE within.


    Thanks,

    Harsh
  • Harsh J at Jun 13, 2011 at 7:11 pm

    On 2011-06-13 18:23:35, Matt Foley wrote:
    Sorry if this is out of context, but is it really best to also return a null here? Shouldn't it check for null result from getSerialization(), then throw a (non-NPE) exception? Or do you prefer to do that check and throw at a higher level of the code?
    Todd Lipcon wrote:
    I thought about this while doing the review... my thinking was that our other similar APIs do return null -eg CompressionCodecFactory.getCodecByName() returns null if the specified codec isn't found. This API is also marked as LimitedPrivate Evolving so it shouldn't be a problematic breaking change.

    Maybe we should add a bit of javadoc saying "@returns null if no serializer is known for the given class." ?
    The public method getSerialization() returns a null if it does not find an acceptor. I think it is fair that a get{Serializer,Deserializer}() call do the same since they are specific nature calls underneath?

    Or we could revamp the entire set if Exceptions are better to have over null returns and null checks, but it ought to be consistent is what I think.


    - Harsh


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

    On 2011-06-11 22:10:17, Harsh J wrote:

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

    (Updated 2011-06-11 22:10:17)


    Review request for hadoop-common and Todd Lipcon.


    Summary
    -------

    Since getSerialization() can possibly return a null, it is only right that getSerializer() and getDeserializer() usage functions do the same, instead of throwing up NPEs.

    Related issue to which this improvement is required: https://issues.apache.org/jira/browse/MAPREDUCE-2584


    This addresses bug HADOOP-7328.
    http://issues.apache.org/jira/browse/HADOOP-7328


    Diffs
    -----

    src/java/org/apache/hadoop/io/serializer/SerializationFactory.java dee314a

    Diff: https://reviews.apache.org/r/884/diff


    Testing
    -------

    Existing SequenceFile serialization factory tests pass. The change is merely to make the functions return null instead of throwing an NPE within.


    Thanks,

    Harsh
  • Sudharsan Sampath at Jun 14, 2011 at 12:50 pm
    Hi,

    Just my thoughts...

    To me throwing a specific exception would be better. The checkSerializerSpecs attempts to see if we can initialise a new instance of the serializer/deserializer from the jvm where the job is submitted. How does it guarantee that the libs/jars from which these classes are loaded are available for the jvm that executes the job or vice versa? Even if this methods succeeds isn't there a chance then the original problem might occur due to the libs missing from the actual jvm that executes the job?

    Sudhan S

    -----Original Message-----
    From: Harsh J
    Sent: Tuesday, June 14, 2011 12:42 AM
    To: Todd Lipcon
    Cc: hadoop-common; Harsh J; Matt Foley
    Subject: Re: Review Request: HADOOP-7328. Improve the SerializationFactory functions.


    On 2011-06-13 18:23:35, Matt Foley wrote:
    Sorry if this is out of context, but is it really best to also return a null here? Shouldn't it check for null result from getSerialization(), then throw a (non-NPE) exception? Or do you prefer to do that check and throw at a higher level of the code?
    Todd Lipcon wrote:
    I thought about this while doing the review... my thinking was that our other similar APIs do return null -eg CompressionCodecFactory.getCodecByName() returns null if the specified codec isn't found. This API is also marked as LimitedPrivate Evolving so it shouldn't be a problematic breaking change.

    Maybe we should add a bit of javadoc saying "@returns null if no serializer is known for the given class." ?
    The public method getSerialization() returns a null if it does not find an acceptor. I think it is fair that a get{Serializer,Deserializer}() call do the same since they are specific nature calls underneath?

    Or we could revamp the entire set if Exceptions are better to have over null returns and null checks, but it ought to be consistent is what I think.


    - Harsh


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

    On 2011-06-11 22:10:17, Harsh J wrote:

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

    (Updated 2011-06-11 22:10:17)


    Review request for hadoop-common and Todd Lipcon.


    Summary
    -------

    Since getSerialization() can possibly return a null, it is only right that getSerializer() and getDeserializer() usage functions do the same, instead of throwing up NPEs.

    Related issue to which this improvement is required:
    https://issues.apache.org/jira/browse/MAPREDUCE-2584


    This addresses bug HADOOP-7328.
    http://issues.apache.org/jira/browse/HADOOP-7328


    Diffs
    -----

    src/java/org/apache/hadoop/io/serializer/SerializationFactory.java
    dee314a

    Diff: https://reviews.apache.org/r/884/diff


    Testing
    -------

    Existing SequenceFile serialization factory tests pass. The change is merely to make the functions return null instead of throwing an NPE within.


    Thanks,

    Harsh
  • Harsh J at Jun 14, 2011 at 1:38 pm
    Hey,

    Agreed on the exception-throwing parts, I'll revamp and post that.

    What you describe about the JVM can surely occur and I've pushed some
    handling at the MapOutputBuffer level as well but in any case its
    better to catch it as early as possible. Don't think guaranteeing that
    the client is indeed packing things along with the job for the DC is
    practically possible, but at least it is an improvement to where the
    client follows a bit of general guidelines I think?

    (P.s. Could you post views on the JIRA/RB so we can track discussions better?)

    [Resending cause I missed lists earlier]

    On Tue, Jun 14, 2011 at 6:19 PM, Sudharsan Sampath
    wrote:
    Hi,

    Just my thoughts...

    To me throwing a specific exception would be better. The checkSerializerSpecs attempts to see if we can initialise a new instance of the serializer/deserializer from the jvm where the job is submitted. How does it guarantee that the libs/jars from which these classes are loaded are available for the jvm that executes the job or vice versa? Even if this methods succeeds isn't there a chance then the original problem might occur due to the libs missing from the actual jvm that executes the job?

    Sudhan S

    -----Original Message-----
    From: Harsh J
    Sent: Tuesday, June 14, 2011 12:42 AM
    To: Todd Lipcon
    Cc: hadoop-common; Harsh J; Matt Foley
    Subject: Re: Review Request: HADOOP-7328. Improve the SerializationFactory functions.


    On 2011-06-13 18:23:35, Matt Foley wrote:
    Sorry if this is out of context, but is it really best to also return a null here?  Shouldn't it check for null result from getSerialization(), then throw a (non-NPE) exception?  Or do you prefer to do that check and throw at a higher level of the code?
    Todd Lipcon wrote:
    I thought about this while doing the review... my thinking was that our other similar APIs do return null -eg CompressionCodecFactory.getCodecByName() returns null if the specified codec isn't found. This API is also marked as LimitedPrivate Evolving so it shouldn't be a problematic breaking change.

    Maybe we should add a bit of javadoc saying "@returns null if no serializer is known for the given class." ?
    The public method getSerialization() returns a null if it does not find an acceptor. I think it is fair that a get{Serializer,Deserializer}() call do the same since they are specific nature calls underneath?

    Or we could revamp the entire set if Exceptions are better to have over null returns and null checks, but it ought to be consistent is what I think.


    - Harsh


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

    On 2011-06-11 22:10:17, Harsh J wrote:

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

    (Updated 2011-06-11 22:10:17)


    Review request for hadoop-common and Todd Lipcon.


    Summary
    -------

    Since getSerialization() can possibly return a null, it is only right that getSerializer() and getDeserializer() usage functions do the same, instead of throwing up NPEs.

    Related issue to which this improvement is required:
    https://issues.apache.org/jira/browse/MAPREDUCE-2584


    This addresses bug HADOOP-7328.
    http://issues.apache.org/jira/browse/HADOOP-7328


    Diffs
    -----

    src/java/org/apache/hadoop/io/serializer/SerializationFactory.java
    dee314a

    Diff: https://reviews.apache.org/r/884/diff


    Testing
    -------

    Existing SequenceFile serialization factory tests pass. The change is merely to make the functions return null instead of throwing an NPE within.


    Thanks,

    Harsh


    --
    Harsh J
  • Harsh J at Jun 16, 2011 at 12:13 pm
    -----------------------------------------------------------
    This is an automatically generated e-mail. To reply, visit:
    https://reviews.apache.org/r/884/
    -----------------------------------------------------------

    (Updated 2011-06-16 12:13:34.081758)


    Review request for hadoop-common and Todd Lipcon.


    Changes
    -------

    Throw exceptions (getting rid of nulls). Add appropriate javadocs and fix one checkstyle nit.


    Summary
    -------

    Since getSerialization() can possibly return a null, it is only right that getSerializer() and getDeserializer() usage functions do the same, instead of throwing up NPEs.

    Related issue to which this improvement is required: https://issues.apache.org/jira/browse/MAPREDUCE-2584


    This addresses bug HADOOP-7328.
    http://issues.apache.org/jira/browse/HADOOP-7328


    Diffs (updated)
    -----

    src/java/org/apache/hadoop/io/serializer/SerializationFactory.java dee314a

    Diff: https://reviews.apache.org/r/884/diff


    Testing
    -------

    Existing SequenceFile serialization factory tests pass. The change is merely to make the functions return null instead of throwing an NPE within.


    Thanks,

    Harsh

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupcommon-dev @
categorieshadoop
postedJun 11, '11 at 10:10p
activeJun 16, '11 at 12:13p
posts9
users5
websitehadoop.apache.org...
irc#hadoop

People

Translate

site design / logo © 2022 Grokbase