Mailing-List: contact common-dev-help@hadoop.apache.org; run by ezmlm
Precedence: bulk
Reply-To: common-dev@hadoop.apache.org
Received-SPF: pass (athena.apache.org: domain of harsh@cloudera.com designates
 209.85.214.176 as permitted sender)
MIME-Version: 1.0
In-Reply-To: 
 <DBB1DC060375D147AC43F310AD987DCC25F00A24FF@ESESSCMS0366.eemea.ericsson.se>
References: <20110613182335.13641.70260@reviews.apache.org>
 <20110613191156.13641.71903@reviews.apache.org>
 <DBB1DC060375D147AC43F310AD987DCC25F00A24FF@ESESSCMS0366.eemea.ericsson.se>
From: Harsh J <harsh@cloudera.com>
Date: Tue, 14 Jun 2011 19:07:12 +0530
Message-ID: <BANLkTikHcH8H8iqciT5WNt8q3DgNP4caiQ@mail.gmail.com>
Subject: Re: Review Request: HADOOP-7328. Improve the SerializationFactory
 functions.
To: Sudharsan Sampath <sudharsan.sampath@ericsson.com>
Cc: "common-dev@hadoop.apache.org" <common-dev@hadoop.apache.org>,
 Todd Lipcon <todd@apache.org>,
	Matt Foley <mattf@yahoo-inc.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable

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 bette=
r?)

[Resending cause I missed lists earlier]

On Tue, Jun 14, 2011 at 6:19 PM, Sudharsan Sampath
<sudharsan.sampath@ericsson.com> wrote:
>
> Hi,
>
> Just my thoughts...
>
> To me throwing a specific exception would be better. The checkSerializerS=
pecs 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 t=
he jvm that executes the job or vice versa? Even if this methods succeeds i=
sn't there a chance then the original problem might occur due to the libs m=
issing from the actual jvm that executes the job?
>
> Sudhan S
>
> -----Original Message-----
> From: Harsh J [mailto:harsh@cloudera.com]
> 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 SerializationFactor=
y 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? =A0Shouldn't it check for null result from getSerialization(),=
 then throw a (non-NPE) exception? =A0Or do you prefer to do that check and=
 throw at a higher level of the code?
>>
>> Todd Lipcon wrote:
>> =A0 =A0 I thought about this while doing the review... my thinking was t=
hat our other similar APIs do return null -eg CompressionCodecFactory.getCo=
decByName() returns null if the specified codec isn't found. This API is al=
so marked as LimitedPrivate Evolving so it shouldn't be a problematic break=
ing change.
>>
>> =A0 =A0 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 a=
n acceptor. I think it is fair that a get{Serializer,Deserializer}() call d=
o the same since they are specific nature calls underneath?
>
> Or we could revamp the entire set if Exceptions are better to have over n=
ull 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 th=
at getSerializer() and getDeserializer() usage functions do the same, inste=
ad 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.
>> =A0 =A0 http://issues.apache.org/jira/browse/HADOOP-7328
>>
>>
>> Diffs
>> -----
>>
>> =A0 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 me=
rely to make the functions return null instead of throwing an NPE within.
>>
>>
>> Thanks,
>>
>> Harsh
>>
>>
>
>


--=20
Harsh J