Pig
  1. Pig
  2. PIG-3484

Make the size of pig.script property configurable

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.13.0
    • Component/s: impl
    • Labels:
      None

      Description

      Some applications (e.g. Lipstick) use the pig.script property to display the script. But since its size is limited by a hard-coded max, it's not always possible to store an entire script.

      It would be nicer if the size of pig.script is configurable.

      1. PIG-3484-1.patch
        5 kB
        Cheolsoo Park
      2. PIG-3484-2.patch
        5 kB
        Cheolsoo Park
      3. PIG-3484-3.patch
        5 kB
        Cheolsoo Park

        Activity

        Hide
        Cheolsoo Park added a comment -

        Committed to trunk. Thank you Prashant one more time!

        Show
        Cheolsoo Park added a comment - Committed to trunk. Thank you Prashant one more time!
        Hide
        Prashant Kommireddi added a comment -

        Hi Cheolsoo Park - I ran TestPigStats and "test-commit". Looks good and seems like you have run all unit tests. Based on that, +1.

        Show
        Prashant Kommireddi added a comment - Hi Cheolsoo Park - I ran TestPigStats and "test-commit". Looks good and seems like you have run all unit tests. Based on that, +1.
        Hide
        Cheolsoo Park added a comment -

        Enabling the patch again since unit tests now pass.

        Show
        Cheolsoo Park added a comment - Enabling the patch again since unit tests now pass.
        Hide
        Cheolsoo Park added a comment -

        Attaching a new patch that fixes the test failures. The issue was that PigContext can be null if ScriptStats is created by ScriptStats.get(). Although this is not the standard way of instantiating ScriptStats, some unit tests do that. As for fix, I added null checks to ScriptStats.set().

        I reverted the old patch and am running the full unit tests now. I will commit it the new patch if all unit tests pass.

        Show
        Cheolsoo Park added a comment - Attaching a new patch that fixes the test failures. The issue was that PigContext can be null if ScriptStats is created by ScriptStats.get(). Although this is not the standard way of instantiating ScriptStats, some unit tests do that. As for fix, I added null checks to ScriptStats.set(). I reverted the old patch and am running the full unit tests now. I will commit it the new patch if all unit tests pass.
        Hide
        Cheolsoo Park added a comment -

        Sorry. I didn't expect to break any unit tests with this patch. I will fix it right away.

        Show
        Cheolsoo Park added a comment - Sorry. I didn't expect to break any unit tests with this patch. I will fix it right away.
        Hide
        Daniel Dai added a comment -

        Cheolsoo Park, this breaks TestPigStats, stack:
        java.lang.NullPointerException
        at org.apache.pig.tools.pigstats.ScriptState.setScript(ScriptState.java:197)
        at org.apache.pig.tools.pigstats.ScriptState.setScript(ScriptState.java:287)
        at org.apache.pig.tools.pigstats.ScriptState.setScript(ScriptState.java:186)
        at org.apache.pig.test.TestPigStats.testJythonScriptInConf(TestPigStats.java:104)

        Can you take a look?

        Show
        Daniel Dai added a comment - Cheolsoo Park , this breaks TestPigStats, stack: java.lang.NullPointerException at org.apache.pig.tools.pigstats.ScriptState.setScript(ScriptState.java:197) at org.apache.pig.tools.pigstats.ScriptState.setScript(ScriptState.java:287) at org.apache.pig.tools.pigstats.ScriptState.setScript(ScriptState.java:186) at org.apache.pig.test.TestPigStats.testJythonScriptInConf(TestPigStats.java:104) Can you take a look?
        Hide
        Cheolsoo Park added a comment -

        Committed to trunk. Thanks Prashant for the review!

        Show
        Cheolsoo Park added a comment - Committed to trunk. Thanks Prashant for the review!
        Hide
        Prashant Kommireddi added a comment -

        LGTM +1

        Show
        Prashant Kommireddi added a comment - LGTM +1
        Hide
        Cheolsoo Park added a comment -

        Updating a new patch that incorporates Prashant's comment.

        Show
        Cheolsoo Park added a comment - Updating a new patch that incorporates Prashant's comment.
        Hide
        Cheolsoo Park added a comment -

        Prashant Kommireddi, you're right. I think PigConfiguration is a better place as they're user-facing. I'll update the patch. Thanks!

        Show
        Cheolsoo Park added a comment - Prashant Kommireddi , you're right. I think PigConfiguration is a better place as they're user-facing. I'll update the patch. Thanks!
        Hide
        Prashant Kommireddi added a comment -

        Hi Cheolsoo, I see you moved a couple of configs to PigConstants. Should these rather be a part of PigConfiguration if they are going to be user facing? There's no clear distinction between the 2 classes other than PigConfiguration having the following in its docs

        /**
         * Container for static configuration strings, defaults, etc. This is intended just for keys that can
         * be set by users, not for keys that are generally used within pig.
         */
        
        Show
        Prashant Kommireddi added a comment - Hi Cheolsoo, I see you moved a couple of configs to PigConstants. Should these rather be a part of PigConfiguration if they are going to be user facing? There's no clear distinction between the 2 classes other than PigConfiguration having the following in its docs /** * Container for static configuration strings, defaults, etc. This is intended just for keys that can * be set by users, not for keys that are generally used within pig. */
        Hide
        Cheolsoo Park added a comment -

        The attached patch adds a new property "pig.script.max.size". The default value is 10,240.

        Show
        Cheolsoo Park added a comment - The attached patch adds a new property "pig.script.max.size". The default value is 10,240.

          People

          • Assignee:
            Cheolsoo Park
            Reporter:
            Cheolsoo Park
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development