Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.19.0
    • Fix Version/s: 0.19.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Incompatible change
    • Release Note:
      Made Configuration Writable and rename the old write method to writeXml.

      Description

      The new class WritableJobConf doesn't add value over just making JobConf Writable, and I propose that we remove it before it is released.

      1. 4293-2.patch
        12 kB
        Owen O'Malley
      2. 4293.patch
        12 kB
        Owen O'Malley

        Activity

        Hide
        Arun C Murthy added a comment -

        +1 - not introducing a new public api is good. In future we could use this rather than the job.xml + dom-parsing for communicating between the JobClient and JobTracker.

        Show
        Arun C Murthy added a comment - +1 - not introducing a new public api is good. In future we could use this rather than the job.xml + dom-parsing for communicating between the JobClient and JobTracker.
        Hide
        Owen O'Malley added a comment -

        This patch:
        1. Changes the name of Configuration.write to writeXml.
        2. Makes Configuration Writable.
        3. Makes the new binary format a vint length, followed by 2*N Texts.
        4. Removes WritableJobConf from the mapred package.

        I'll create a new jira proposing that we use the binary file format instead of the job.xml for job submission.

        Show
        Owen O'Malley added a comment - This patch: 1. Changes the name of Configuration.write to writeXml. 2. Makes Configuration Writable. 3. Makes the new binary format a vint length, followed by 2*N Texts. 4. Removes WritableJobConf from the mapred package. I'll create a new jira proposing that we use the binary file format instead of the job.xml for job submission.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12391085/4293-2.patch
        against trunk revision 699676.

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

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

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

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

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

        +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

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

        -1 contrib tests. The patch failed contrib unit tests.

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

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12391085/4293-2.patch against trunk revision 699676. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. +1 core tests. The patch passed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3388/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3388/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3388/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3388/console This message is automatically generated.
        Hide
        Enis Soztutar added a comment -

        WritableJobConf is introduced as a work around to breaking compatibility of Configuration#write() (HADOOP-3702). If we are ok to break it, I'm +1.

        Show
        Enis Soztutar added a comment - WritableJobConf is introduced as a work around to breaking compatibility of Configuration#write() ( HADOOP-3702 ). If we are ok to break it, I'm +1.
        Hide
        Alejandro Abdelnur added a comment -

        For HADOOP-3702 we needed to serialize a JobConf into a String.

        There were 3 possible alternatives considered:

        1. Make Configuration or JobConf implement Writable The problem with this option is that Configuration already as a write(OutputStream) method (that serializes to XML) and making it to implement Writable was creating compiler conflicts when used with a DataOutptuStream instance.
        1. Make a Serialization implementation for Configuration (then used by its subclasses) There was some opposition to do this.
        1. Make a subclass of JobConf that implements Writable This was the proposed alternative.

        I didn't want to do Option #1 not to break backwards compatibility (quite a few places in core and contribs).

        I preferred Option #2 but I did not have an objection to Option #3.

        If people feel that Option #2 is more adequate I could make that change (one of the patches was doing this approach)

        Show
        Alejandro Abdelnur added a comment - For HADOOP-3702 we needed to serialize a JobConf into a String . There were 3 possible alternatives considered: Make Configuration or JobConf implement Writable The problem with this option is that Configuration already as a write(OutputStream) method (that serializes to XML) and making it to implement Writable was creating compiler conflicts when used with a DataOutptuStream instance. Make a Serialization implementation for Configuration (then used by its subclasses) There was some opposition to do this. Make a subclass of JobConf that implements Writable This was the proposed alternative. I didn't want to do Option #1 not to break backwards compatibility (quite a few places in core and contribs). I preferred Option #2 but I did not have an objection to Option #3. If people feel that Option #2 is more adequate I could make that change (one of the patches was doing this approach)
        Hide
        Owen O'Malley added a comment -

        Hmm... I don't see what is wrong with option 1. It does introduce an incompatibility, but in a place where it is unlikely to break clients. The change was not extensive. The entire patch is 12k and there were 9 calls to the old write. And it provides the functionality in a way that fits the expected behavior.

        In HADOOP-1230, we're going deprecate JobConf. So I don't think having a subclass that adds readers/writers doesn't seem right. I'm also not very happy with the write(OutputStream) versus write(DataOutput), which can easily lead to confusion.

        And using the binary format instead of the xml for job submission would make a lot of sense, be much much faster, and avoid all of the quoting problems of using xml to serialize the configuration.

        Show
        Owen O'Malley added a comment - Hmm... I don't see what is wrong with option 1. It does introduce an incompatibility, but in a place where it is unlikely to break clients. The change was not extensive. The entire patch is 12k and there were 9 calls to the old write. And it provides the functionality in a way that fits the expected behavior. In HADOOP-1230 , we're going deprecate JobConf. So I don't think having a subclass that adds readers/writers doesn't seem right. I'm also not very happy with the write(OutputStream) versus write(DataOutput), which can easily lead to confusion. And using the binary format instead of the xml for job submission would make a lot of sense, be much much faster, and avoid all of the quoting problems of using xml to serialize the configuration.
        Hide
        Owen O'Malley added a comment -

        I should add that my biggest complaint is adding a new class in the mapred package that is only used by a library class. That doesn't seem like a good direction. Furthermore, the class doesn't add any new fields, just adds on another form of serialization for the preexisting class.

        Show
        Owen O'Malley added a comment - I should add that my biggest complaint is adding a new class in the mapred package that is only used by a library class. That doesn't seem like a good direction. Furthermore, the class doesn't add any new fields, just adds on another form of serialization for the preexisting class.
        Hide
        Arun C Murthy added a comment -

        +1, looks good.

        I agree that option 1 is the right call here.

        Show
        Arun C Murthy added a comment - +1, looks good. I agree that option 1 is the right call here.
        Hide
        Owen O'Malley added a comment -

        I just committed this.

        Show
        Owen O'Malley added a comment - I just committed this.
        Hide
        Hudson added a comment -

        Integrated in Hadoop-trunk #620 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/620/)
        . Make Configuration Writable and remove unreleased
        WritableJobConf. Configuration.write is renamed to writeXml. (omalley)

        Show
        Hudson added a comment - Integrated in Hadoop-trunk #620 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/620/ ) . Make Configuration Writable and remove unreleased WritableJobConf. Configuration.write is renamed to writeXml. (omalley)

          People

          • Assignee:
            Owen O'Malley
            Reporter:
            Owen O'Malley
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development