Pig
  1. Pig
  2. PIG-3194

Changes to ObjectSerializer.java break compatibility with Hadoop 0.20.2

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.11
    • Fix Version/s: 0.12.0, 0.11.1
    • Component/s: None
    • Labels:
      None
    • Patch Info:
      Patch Available

      Description

      The changes to ObjectSerializer.java in the following commit
      http://svn.apache.org/viewvc?view=revision&revision=1403934 break compatibility with Hadoop 0.20.2 Clusters.

      The reason is, that the code uses methods from Apache Commons Codec 1.4 - which are not available in Apache Commons Codec 1.3 which is shipping with Hadoop 0.20.2.

      The offending methods are Base64.decodeBase64(String) and Base64.encodeBase64URLSafeString(byte[])

      If I revert these changes, Pig 0.11.0 candidate 2 works well with our Hadoop 0.20.2 Clusters.

      1. PIG-3194_2.patch
        5 kB
        Prashant Kommireddi
      2. PIG-3194.patch
        5 kB
        Prashant Kommireddi

        Activity

        Hide
        Jonathan Coveney added a comment -

        Do we want to consider just using commons codec 1.3? Do the newer versions of Hadoop still use that version of commons codec?

        Show
        Jonathan Coveney added a comment - Do we want to consider just using commons codec 1.3? Do the newer versions of Hadoop still use that version of commons codec?
        Hide
        Kai Londenberg added a comment -

        I would propose to try and stay as compatible as possible with reasonable effort. Here, the effort to stay compatible with a still widely used version of Hadoop seems to be minimal. I see 3 alternative solutions here:

        1.) Revert the mentioned commit.

        2.) Reproduce the required functionality using code that's compatible to commons codec 1.3 (i.e. use Base64.decodeBase64(str.getBytes()) instead of the shortcut Base64.decodeBase64(str).

        3.) Embed a rebased commons codec 1.4+ into pig. With that, I mean a version of commons codec 1.4+ which has been automatically refactored such that it has a new base package, for example pig.org.apache.commons.codec or something like that.

        Show
        Kai Londenberg added a comment - I would propose to try and stay as compatible as possible with reasonable effort. Here, the effort to stay compatible with a still widely used version of Hadoop seems to be minimal. I see 3 alternative solutions here: 1.) Revert the mentioned commit. 2.) Reproduce the required functionality using code that's compatible to commons codec 1.3 (i.e. use Base64.decodeBase64(str.getBytes()) instead of the shortcut Base64.decodeBase64(str). 3.) Embed a rebased commons codec 1.4+ into pig. With that, I mean a version of commons codec 1.4+ which has been automatically refactored such that it has a new base package, for example pig.org.apache.commons.codec or something like that.
        Hide
        Jonathan Coveney added a comment -

        Assuming that all hadoop uses commons codec 1.3 (is that what hadoop 2 uses?) then #2 seems like a no-brainer to me

        Show
        Jonathan Coveney added a comment - Assuming that all hadoop uses commons codec 1.3 (is that what hadoop 2 uses?) then #2 seems like a no-brainer to me
        Hide
        Jonathan Coveney added a comment -

        Hmm, so I simply set common-codec.version in ivy/library.properties back to 1.3 and everything worked fine, but it looks like something else is pulling in the 1.4 lib (it appears in build/ivy/lib/Pig/). Need to figure out what is causing that and then my plan is to roll back to 1.3, see what breaks, and fix it. I doubt there is very much.

        Need to get to the bottom of why 1.4 is still getting brought in, though.

        Show
        Jonathan Coveney added a comment - Hmm, so I simply set common-codec.version in ivy/library.properties back to 1.3 and everything worked fine, but it looks like something else is pulling in the 1.4 lib (it appears in build/ivy/lib/Pig/). Need to figure out what is causing that and then my plan is to roll back to 1.3, see what breaks, and fix it. I doubt there is very much. Need to get to the bottom of why 1.4 is still getting brought in, though.
        Hide
        Ashutosh Chauhan added a comment -

        Hadoop has moved to 1.4 long way back. If I remember correctly 0.20.2 was last release with 1.3. In hadoop2 I believe they are at 1.5. Though this is debatable but IMO consistency with hadoop-1 is more important than 0.20.2.
        http://svn.apache.org/viewvc/hadoop/common/branches/branch-1/ivy/libraries.properties?revision=1342816&view=markup

        Show
        Ashutosh Chauhan added a comment - Hadoop has moved to 1.4 long way back. If I remember correctly 0.20.2 was last release with 1.3. In hadoop2 I believe they are at 1.5. Though this is debatable but IMO consistency with hadoop-1 is more important than 0.20.2. http://svn.apache.org/viewvc/hadoop/common/branches/branch-1/ivy/libraries.properties?revision=1342816&view=markup
        Hide
        Kai Londenberg added a comment -

        I'd recommend to build the way you do now with 1.4. But test with Hadoop 0.20.2 as well. Mostly, commons-codec 1.3 and 1.4 are compatible. 1.3 is just missing some Methods. Actually the compatibility problem is just these 2 methods. Maybe it's even easier to copy the Base64 class from commons-codec 1.4 into some utility package of pit itself.

        Show
        Kai Londenberg added a comment - I'd recommend to build the way you do now with 1.4. But test with Hadoop 0.20.2 as well. Mostly, commons-codec 1.3 and 1.4 are compatible. 1.3 is just missing some Methods. Actually the compatibility problem is just these 2 methods. Maybe it's even easier to copy the Base64 class from commons-codec 1.4 into some utility package of pit itself.
        Hide
        Prashant Kommireddi added a comment -

        Base64 seems to have been refactored a bit for codec 1.4. It now extends a base class (BaseNCodec) and uses a util (StringUtils). There's not much of dependency on StringUtils. Though BaseNCodec and Base64 are slightly tied up (even though it's just the 2 methods we use in pig). We could do a few things here

        1. Bring Base* classes into a pig util
        2. Eliminate call to encodeBase64URLSafeString and use byte[] encodeBase64 (available in 1.3) instead.

        Looking at the Base64 code, there isn't much difference in the 2 methods except that '-' is used for '+' and '_' is used for '/' with URLSafe. Doesn't look like we need the encoding be url safe either?

        And as Kai suggested, we can use Base64.decodeBase64(str.getBytes()) from 1.3 for decoding.

        Thoughts?

        Show
        Prashant Kommireddi added a comment - Base64 seems to have been refactored a bit for codec 1.4. It now extends a base class (BaseNCodec) and uses a util (StringUtils). There's not much of dependency on StringUtils. Though BaseNCodec and Base64 are slightly tied up (even though it's just the 2 methods we use in pig). We could do a few things here 1. Bring Base* classes into a pig util 2. Eliminate call to encodeBase64URLSafeString and use byte[] encodeBase64 (available in 1.3) instead. Looking at the Base64 code, there isn't much difference in the 2 methods except that '-' is used for '+' and '_' is used for '/' with URLSafe. Doesn't look like we need the encoding be url safe either? And as Kai suggested, we can use Base64.decodeBase64(str.getBytes()) from 1.3 for decoding. Thoughts?
        Hide
        Prashant Kommireddi added a comment -

        Would be great to have some ideas from others and discuss

        Show
        Prashant Kommireddi added a comment - Would be great to have some ideas from others and discuss
        Hide
        Jonathan Coveney added a comment -

        I don't love the idea of bringing the classes in, and would rather just use the methods available in 1.3 if possible. I think that's a fine path, as you are correct: we don't need to be url safe.

        Show
        Jonathan Coveney added a comment - I don't love the idea of bringing the classes in, and would rather just use the methods available in 1.3 if possible. I think that's a fine path, as you are correct: we don't need to be url safe.
        Hide
        Julien Le Dem added a comment -

        same as Jon. we can just use the methods present in 1.3 and we don't need to be URL safe.
        Let's not repackage commons.codec or duplicate part of it just for this.

        Show
        Julien Le Dem added a comment - same as Jon. we can just use the methods present in 1.3 and we don't need to be URL safe. Let's not repackage commons.codec or duplicate part of it just for this.
        Hide
        Prashant Kommireddi added a comment -

        Sounds good to me. 1 was a just an option to put out there, I agree using 1.3 methods is a lot cleaner/better approach.

        Show
        Prashant Kommireddi added a comment - Sounds good to me. 1 was a just an option to put out there, I agree using 1.3 methods is a lot cleaner/better approach.
        Hide
        Prashant Kommireddi added a comment -

        Changed ObjectSerializer to use codec 1.3 methods. Tested with unit tests that are using ser/deser (TestPruneColumn, TestEvalPipeline2) and "ant test-commit" - all pass. Also tested against Load/Store funcs that use a lot of this (TestPigStorage, TestHBaseStorage).

        Please review.

        Show
        Prashant Kommireddi added a comment - Changed ObjectSerializer to use codec 1.3 methods. Tested with unit tests that are using ser/deser (TestPruneColumn, TestEvalPipeline2) and "ant test-commit" - all pass. Also tested against Load/Store funcs that use a lot of this (TestPigStorage, TestHBaseStorage). Please review.
        Hide
        Dmitriy V. Ryaboy added a comment -

        Prashant, were you able to test in environments where 1.4 is and is not available?

        Show
        Dmitriy V. Ryaboy added a comment - Prashant, were you able to test in environments where 1.4 is and is not available?
        Hide
        Prashant Kommireddi added a comment -

        Dmitriy, yes. Here is what I have done after the fix:

        1. compile pig with 1.4
        1a. start up pig with hadoop 0.20.2 - runs [expected]
        1b. start up pig with hadoop 1 - runs [expected]
        1c. test-commit and few other tests mentioned in my previous comment all pass.
        1d. run sample scripts against 1a and 1b - works good.

        2. compile pig with 1.3
        2a. start up pig with hadoop 0.20.2 - runs [expected]
        2b. start up pig with hadoop-1 - runs [expected]
        2c. test-commit and other tests pass but TestMRCompiler fails - [not expected]
        2d. run sample scripts against 2a and 2b - works good

        The issue with 2c (TestMRCompiler.testMergeJoin failing against 1.3) is that the gold file being used contains a serialized string that was generated using 1.4. There seem to be a few differences between 1.3 and 1.4 with encode/decode behavior - please see https://issues.apache.org/jira/browse/CODEC-89, https://issues.apache.org/jira/browse/CODEC-91

        Considering our tests run against hadoop-1 and all pass I am not sure if we should be spending time in making the test case codec 1.3 aware?

        Show
        Prashant Kommireddi added a comment - Dmitriy, yes. Here is what I have done after the fix: 1. compile pig with 1.4 1a. start up pig with hadoop 0.20.2 - runs [expected] 1b. start up pig with hadoop 1 - runs [expected] 1c. test-commit and few other tests mentioned in my previous comment all pass. 1d. run sample scripts against 1a and 1b - works good. 2. compile pig with 1.3 2a. start up pig with hadoop 0.20.2 - runs [expected] 2b. start up pig with hadoop-1 - runs [expected] 2c. test-commit and other tests pass but TestMRCompiler fails - [not expected] 2d. run sample scripts against 2a and 2b - works good The issue with 2c (TestMRCompiler.testMergeJoin failing against 1.3) is that the gold file being used contains a serialized string that was generated using 1.4. There seem to be a few differences between 1.3 and 1.4 with encode/decode behavior - please see https://issues.apache.org/jira/browse/CODEC-89 , https://issues.apache.org/jira/browse/CODEC-91 Considering our tests run against hadoop-1 and all pass I am not sure if we should be spending time in making the test case codec 1.3 aware?
        Hide
        Prashant Kommireddi added a comment -

        Dmitriy, let me know what you think?

        Show
        Prashant Kommireddi added a comment - Dmitriy, let me know what you think?
        Hide
        Dmitriy V. Ryaboy added a comment -

        We can skip the test if we detect that we are on Hadoop 2.0 by using org.junit.Assume

        Let's do that so we don't have to come back and fix this later.

        Show
        Dmitriy V. Ryaboy added a comment - We can skip the test if we detect that we are on Hadoop 2.0 by using org.junit.Assume Let's do that so we don't have to come back and fix this later.
        Hide
        Prashant Kommireddi added a comment -

        Uploading a new patch with Dmitriy's feedback incorporated.

        Show
        Prashant Kommireddi added a comment - Uploading a new patch with Dmitriy's feedback incorporated.
        Hide
        Dmitriy V. Ryaboy added a comment -

        +1

        Show
        Dmitriy V. Ryaboy added a comment - +1
        Hide
        Dmitriy V. Ryaboy added a comment -

        Committed to 0.11.1 and trunk.

        Thanks Kai for reporting and Prashant for fixing!

        Show
        Dmitriy V. Ryaboy added a comment - Committed to 0.11.1 and trunk. Thanks Kai for reporting and Prashant for fixing!
        Hide
        Prashant Kommireddi added a comment -

        Thanks for review/commit, Dmitriy.

        Show
        Prashant Kommireddi added a comment - Thanks for review/commit, Dmitriy.
        Hide
        Prashant Kommireddi added a comment -

        Kai, can you confirm 11.1 works for you?

        Show
        Prashant Kommireddi added a comment - Kai, can you confirm 11.1 works for you?

          People

          • Assignee:
            Prashant Kommireddi
            Reporter:
            Kai Londenberg
          • Votes:
            0 Vote for this issue
            Watchers:
            8 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development