Stringifier

• Key: HADOOP-3048
• Type: New Feature
• Status: Closed
• Resolution: Fixed
• Priority: Blocker
• Assignee: Enis Soztutar
• Reporter: Enis Soztutar
• Component/s: io
• Affects Version/s: 0.17.0
• Fix Version/s: 0.17.0

Description

Storing arbitrary objects in the configuration has been discussed before in HADOOP-449 and HADOOP-1873. Although enabling such functionality has the risk of encouraging people to put big binary objects in the configuration, for some use cases passing objects to tasks is absolutely necessary.

This issue will track the implementation of a Stringifier interface which stringifies and destringifies objects. Using this implementation, developers can store objects in the configuration and restore them later.

Any thoughts ?

Comments

[Enis Soztutar]

Here is an initial patch which was part of the patch for HADOOP-449. We can use this as a starting point.

[Enis Soztutar]

For completeness, I will repeat Chris Douglas' comments about the implementation here :

• DefaultStringifier could reuse more of the objects it creates. The serializer will hold a reference to the ByteArray(Input/Output)Buffers you've defined, so re-opening and closing them every time is unnecessary if you use a resetable stream like o.a.h.io.Data(Input/Output)Buffer.
• The static convenience methods (load, store, storeArray) that create new Stringifiers in DefaultStringifier are probably unnecessary. Adding a stringified object to a config doesn't need that much support.
• Further, it's not clear to me how one would register a Stringifier that isn't a DefaultStringifier. Would it make sense to follow the pattern in the Serialization framework from HADOOP-1986- or the WritableComparator static initialization- and register custom Stringifiers for classes?

[Enis Soztutar]

Here is the updated patch with the following comments :

DefaultStringifier could reuse more of the objects it creates. The serializer will hold a reference to the ByteArray(Input/Output)Buffers you've defined, so re-opening and closing them every time is unnecessary if you use a resetable stream like o.a.h.io.Data(Input/Output)Buffer.

Nod, i have changed the code to use DataInput/OutputBuffer.

The static convenience methods (load, store, storeArray) that create new Stringifiers in DefaultStringifier are probably unnecessary. Adding a stringified object to a config doesn't need that much support.

In my opinion the main use case for the Stringifiers would be to store/load objects in the configuration, so the static convenience methods are helpful. Besides I do not see why would not want them.

Further, it's not clear to me how one would register a Stringifier that isn't a DefaultStringifier. Would it make sense to follow the pattern in the Serialization framework from HADOOP-1986- or the WritableComparator static initialization- and register custom Stringifiers for classes?

Well, i first designed Stringifier to store the object in the configuration and DefaultStringifier just does the job. i do not foresee any other implementation other than Default but I just extracted the interface out of the class so that new stringifiers can be implemented, for whatever reason. In the current patch(HADOOP-449) everybody who wants to use a Stringifier just uses the Default one, and I think there is no need to develop a framework such as the Serialization until a need for another Stringifier emerges.

[Tsz Wo (Nicholas), SZE]

I like the idea of Stringifier interface.

In the patch, there are quite a few @SuppressWarnings("unchecked") tag in the codes. Is it possible to make the generics right and get ride of the tags?

[Hadoop QA]

+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12378258/stringifier_v2.patch
against trunk revision 619744.

@author +1. The patch does not contain any @author tags.

tests included +1. The patch appears to include 4 new or modified tests.

javadoc +1. The javadoc tool did not generate any warning messages.

javac +1. The applied patch does not generate any new javac compiler warnings.

release audit +1. The applied patch does not generate any new release audit warnings.

findbugs +1. The patch does not introduce any new Findbugs warnings.

core tests +1. The patch passed core unit tests.

contrib tests +1. The patch passed contrib unit tests.

Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2008/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2008/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2008/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2008/console

This message is automatically generated.

[Enis Soztutar]

Version 3 of the patch :

• removed static load/store accepting properties objects(the need for them in the patch for HADOOP-449 is lifted).
• extended test case
• added a method, to create a generic array. creating arrays of generics is not possible.

In the patch, there are quite a few @SuppressWarnings("unchecked") tag in the codes. Is it possible to make the generics right and get ride of the tags?

Well i guess not. The warnings result from obj.getClass() function returning Class<? extends X>} where X is the erasure of the static type of the expression on which getClass is called. If you can find a better way of not causing warnings i would gladly accept that.

[Tsz Wo (Nicholas), SZE]

> Well i guess not. The warnings result from obj.getClass() function returning Class<? extends X>} where X is the erasure of the static type of the expression on which getClass is called. If you can find a better way of not causing warnings i would gladly accept that.

+1: It seems to me that there is no way to do t.getClass() for generic t without a unchecked warning. i.e.

T t = ...;
Class<XXX> c = t.getClass(); //there will be a unchecked warning, no matter XXX is (or it does not compile).

I have the following suggestions:

• Only add @SuppressWarnings("unchecked") right before the required statements but not before the entire method.

• Create a static getClass(T) as following:

  static public <T> Class<T> getClass(T t) {
      @SuppressWarnings("unchecked")
      Class<T> clazz = (Class<T>)t.getClass();
      return clazz;
  }

  Use getClass(t) instead of t.getClass(). Then, I guess we only need to add one @SuppressWarnings("unchecked") tag.

[Hadoop QA]

+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12378328/stringifier_v3.patch
against trunk revision 619744.

@author +1. The patch does not contain any @author tags.

tests included +1. The patch appears to include 4 new or modified tests.

javadoc +1. The javadoc tool did not generate any warning messages.

javac +1. The applied patch does not generate any new javac compiler warnings.

release audit +1. The applied patch does not generate any new release audit warnings.

findbugs +1. The patch does not introduce any new Findbugs warnings.

core tests +1. The patch passed core unit tests.

contrib tests +1. The patch passed contrib unit tests.

Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2018/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2018/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2018/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2018/console

This message is automatically generated.

[Enis Soztutar]

I've updated the patch, added a GenericsUtil class and its test case, and moved the generics functions to it. Now the @SuppressWarnings("unchecked")'s are as localized as possible.

[Hadoop QA]

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12378563/stringifier_v4.patch
against trunk revision 619744.

@author +1. The patch does not contain any @author tags.

tests included +1. The patch appears to include 11 new or modified tests.

javadoc -1. The javadoc tool appears to have generated 1 warning messages.

javac +1. The applied patch does not generate any new javac compiler warnings.

release audit +1. The applied patch does not generate any new release audit warnings.

findbugs +1. The patch does not introduce any new Findbugs warnings.

core tests +1. The patch passed core unit tests.

contrib tests +1. The patch passed contrib unit tests.

Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2051/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2051/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2051/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2051/console

This message is automatically generated.

[Enis Soztutar]

Fixed the javadoc problem, I guess there is no need to retrigger hudson.

[Tsz Wo (Nicholas), SZE]

+1 for GenericsUtil. I am sorry that I am not familiar with the other parts of the patch.

[Chris Douglas]

• I'm uncomfortable with GenericsUtil. It can't be denied that Java generics have some fiercely annoying properties, but factoring this out is probably the wrong solution. Where GenericsUtil::getClass(T) is called, Class::asSubclass(Class) should be preferred. Similarly, GenericsUtil::toArray is called from a function that's destringifying and deserializing objects. It's not statically-checkable code, the casts are wholly justified, and the warning should be suppressed. Contexts where toArray might be useful should typically prefer the equally typesafe implementation in ArrayList (this case included).
• Since separator is a constant, it should be in all caps

Other than that, this looks great.

I think there is no need to develop a framework such as the Serialization until a need for another Stringifier emerges.

Agreed.

[Enis Soztutar]

I'm uncomfortable with GenericsUtil. It can't be denied that Java generics have some fiercely annoying properties, but factoring this out is probably the wrong solution.

I cannot understand why we do not want to separate the functionality to deal with generics. The proposed functions are fairly general and can be used by others, and it seems that we cannot get rid of them(please see below), so extracting this out is better than introducing ad hoc solutions.

Where GenericsUtil::getClass(T) is called, Class::asSubclass(Class) should be preferred.

we cannot use Class::asSubclass(Class) get a Class<T> object where T is a generic type, because then we would use :

T item; 
Class<T> c = item.getClass().asSubclass(  <cannot_fill>  );

in the above code, we cannot write anything as an argument to getSubclass, since we do not have any object of type Class<T>. (if we had then why would we bother getting an object of the same type). GenericsUtil#getClass(T) is introduced to handle the repetitive pattern of

@SuppressWarnings("unchecked")
Class<T> c = (Class<T>)t.getClass();

Similarly, GenericsUtil::toArray is called from a function that's destringifying and deserializing objects. It's not statically-checkable code, the casts are wholly justified, and the warning should be suppressed. Contexts where toArray might be useful should typically prefer the equally typesafe implementation in ArrayList (this case included).

Unfortunatelly, we cannot use ArrayList#toArray(T[] a), since creating a generic array is not allowed, similarly ArrayList#toArray() will not work either, since it returns an Object[] and casting Object[] to T[] is not possible. Thus we needed a method to do the job of returning a generic array from a List, that's why we have GenericsUtil::toArray(). The above cases are clearly demonstrated in TestGenericsUtil#testWithGenericClass().

Since separator is a constant, it should be in all caps

I would change that, if it matters.

[Enis Soztutar]

updated patch refactors the constant variable into uppercase. If there is no further issues with the patch, I will commit this before the feature freeze. (04/04/08)

[Chris Douglas]

Attached is an example of your patch that avoids GenericsUtil. It contains only a single, commented SuppressWarnings annotation for loadArray, necessary because Array.newInstance returns Object. Most of what it does is move the load/store functionality from static methods- that create instances of DefaultSerializer- to instance methods that maintain the modest type safety that generics can guarantee. It also keeps the class object one passes in to the constructor. Other than those minor tweaks, the rest of the functionality remains as you had it in your last patch.

You've convinced me of the utility of getClass(T t), though. Do you think it could go into ReflectionUtils?

[Enis Soztutar]

There is NO difference between

public static <T> T[] toArray(Class<T> c, List<T> list)
{
    @SuppressWarnings("unchecked")
    T[] ta= (T[])Array.newInstance(c, list.size());
    for (int i= 0; i<list.size(); i++)
      ta[i]= list.get(i);
    return ta;
}

and

list.toArray((T[])Array.newInstance(clazz, parts.length));

except that the former is encapsulated in a reusable, well documented utility function, that others can freely use, so in that case I am sorry but I have to insist on keeping GenericsUtil. Here is the Sun's implementation of ArrayList#toArray() for reference :

public <T> T[] toArray(T[] a) {
        if (a.length < size)
            a = (T[])java.lang.reflect.Array.
		newInstance(a.getClass().getComponentType(), size);
	System.arraycopy(elementData, 0, a, 0, size);
        if (a.length > size)
            a[size] = null;
        return a;
    }

On the issue about load/store methods being static or not, I can say that having one or two-liner non-static functions will not hurt, but the static versions are "convenience" methods (as stated before, their main usage will be to store/load objects to/from the configuration, and there is no need to keep a DefaultStringifier reference for a one-time operation.), thus I propose we introduce both static and non-static versions. Static methods are not useless, since they are already being used in the patch for HADOOP-449.

[Enis Soztutar]

Updated patch, includes both static and non-static methods for load/store, keeps GenericsUtil as it is.
Chris are you OK with this version?

[Chris Douglas]

There is NO difference between X and Y except that the former is encapsulated in a reusable, well documented utility function, that others can freely use...

That said functionality is encapsulated in a reusable, well documented utility function that others can freely use is the substance of my objection. I don't think it's necessary and while the compiler (and Hudson) can flag an unchecked cast and committers can spot-check for wanton (and uncommented) SuppressWarnings annotations, less fastidious uses of GenericUtils functions can slip in unnoted, unscrutinized, and unjustified. I fully agree with your reasoning, but come to the opposite conclusion: that there is no difference between the one line of code (that generates a warning that must be justified) and a utility function in a new class recommends against the latter approach.

All that said, I can live with GenericsUtil if you remain convinced that it has a role. Arguing against my self of three lines ago, most of its functionality could also fit in ReflectionUtils, but the distinction couldn't matter less.

On the issue about load/store methods being static or not, I can say that having one or two-liner non-static functions will not hurt, but the static versions are "convenience" methods (as stated before, their main usage will be to store/load objects to/from the configuration, and there is no need to keep a DefaultStringifier reference for a one-time operation.), thus I propose we introduce both static and non-static versions.

OK. If you're going to leave the statics in, then you might as well drop the instance methods (v6 instead of v7). If someone creates an instance and keeps it around, you're right, they're probably dealing with volumes of data that won't go into the configuration (we hope).

+1 for v6. Thanks for all your work- and rework- on this.

[Enis Soztutar]

After coming to terms, I have committed the version 6 of the patch. Thanks to the reviewers.

[Hudson]

Integrated in Hadoop-trunk #451 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/451/)