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-3.patch
        5 kB
        Cheolsoo Park
      2. PIG-3484-2.patch
        5 kB
        Cheolsoo Park
      3. PIG-3484-1.patch
        5 kB
        Cheolsoo Park

        Activity

        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Patch Available Patch Available
        2m 54s 1 Cheolsoo Park 26/Sep/13 05:44
        Resolved Resolved Reopened Reopened
        14h 20m 1 Cheolsoo Park 27/Sep/13 18:19
        Reopened Reopened Patch Available Patch Available
        2d 10h 53m 1 Cheolsoo Park 30/Sep/13 05:13
        Patch Available Patch Available Resolved Resolved
        1d 10h 28m 2 Cheolsoo Park 30/Sep/13 17:27
        Resolved Resolved Closed Closed
        280d 1h 40m 1 Daniel Dai 07/Jul/14 19:07
        Daniel Dai made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Cheolsoo Park made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        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.
        Cheolsoo Park made changes -
        Status Reopened [ 4 ] Patch Available [ 10002 ]
        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.
        Cheolsoo Park made changes -
        Attachment PIG-3484-3.patch [ 12605513 ]
        Cheolsoo Park made changes -
        Attachment PIG-3484-3.patch [ 12605512 ]
        Cheolsoo Park made changes -
        Attachment PIG-3484-3.patch [ 12605512 ]
        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.
        Cheolsoo Park made changes -
        Resolution Fixed [ 1 ]
        Status Resolved [ 5 ] Reopened [ 4 ]
        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?
        Cheolsoo Park made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        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
        Cheolsoo Park made changes -
        Attachment PIG-3484-2.patch [ 12605342 ]
        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. */
        Cheolsoo Park made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Cheolsoo Park made changes -
        Field Original Value New Value
        Attachment PIG-3484-1.patch [ 12605188 ]
        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.
        Cheolsoo Park created issue -

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development