Uploaded image for project: 'Apache Tez'
  1. Apache Tez
  2. TEZ-2383

Cleanup input/output/processor contexts in LogicalIOProcessorRuntimeTask

Details

    • Bug
    • Status: Closed
    • Major
    • Resolution: Fixed
    • None
    • 0.7.0
    • None
    • None

    Description

      Currently they get released when sorter object gets GC-ed, but it might be good to explicitly release them on close as well.

      Attachments

        1. TEZ-2383.1.patch
          3 kB
          Rajesh Balamohan
        2. TEZ-2383.2.patch
          12 kB
          Rajesh Balamohan
        3. TEZ-2383.3.patch
          12 kB
          Rajesh Balamohan
        4. TEZ-2383.4.patch
          18 kB
          Rajesh Balamohan
        5. TEZ-2383.branch-0.6.patch
          23 kB
          Rajesh Balamohan

        Issue Links

          Activity

            rajesh.balamohan Rajesh Balamohan added a comment - - edited

            jeagles - I have rebased the patch for branch-0.6. Please let me know if this can be committed branch-0.6;

            rajesh.balamohan Rajesh Balamohan added a comment - - edited jeagles - I have rebased the patch for branch-0.6. Please let me know if this can be committed branch-0.6;

            Rebased patch for branch-0.6

            rajesh.balamohan Rajesh Balamohan added a comment - Rebased patch for branch-0.6
            hitesh Hitesh Shah added a comment -

            Closing issue as 0.5.4, 0.6.1 and 0.7.0 have been released.

            hitesh Hitesh Shah added a comment - Closing issue as 0.5.4, 0.6.1 and 0.7.0 have been released.

            Thanks sseth. Committed to master.

            >>
            commit 5f63de8eecbc8c0f487da122714f0aca1639ac4f
            >>

            rajesh.balamohan Rajesh Balamohan added a comment - Thanks sseth . Committed to master. >> commit 5f63de8eecbc8c0f487da122714f0aca1639ac4f >>
            tezqa TezQA added a comment -

            +1 overall. Here are the results of testing the latest attachment
            http://issues.apache.org/jira/secure/attachment/12729337/TEZ-2383.4.patch
            against master revision ce1bd72.

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

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

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

            +1 javadoc. There were no new javadoc warning messages.

            +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

            +1 release audit. The applied patch does not increase the total number of release audit warnings.

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

            Test results: https://builds.apache.org/job/PreCommit-TEZ-Build/583//testReport/
            Console output: https://builds.apache.org/job/PreCommit-TEZ-Build/583//console

            This message is automatically generated.

            tezqa TezQA added a comment - +1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12729337/TEZ-2383.4.patch against master revision ce1bd72. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-TEZ-Build/583//testReport/ Console output: https://builds.apache.org/job/PreCommit-TEZ-Build/583//console This message is automatically generated.
            sseth Siddharth Seth added a comment -

            +1. Looks good. Thanks rajesh.balamohan

            sseth Siddharth Seth added a comment - +1. Looks good. Thanks rajesh.balamohan

            Addressing review comments. Added same for input/output contexts.

            rajesh.balamohan Rajesh Balamohan added a comment - Addressing review comments. Added same for input/output contexts.
            sseth Siddharth Seth added a comment -

            The patch looks good.
            The intent is to allow GC of the Inputs / Outputs even if the user code holds on to a context object after it completes ?

            We should probably do the same for the Input and OutputContexts.

            sseth Siddharth Seth added a comment - The patch looks good. The intent is to allow GC of the Inputs / Outputs even if the user code holds on to a context object after it completes ? We should probably do the same for the Input and OutputContexts.
            tezqa TezQA added a comment -

            +1 overall. Here are the results of testing the latest attachment
            http://issues.apache.org/jira/secure/attachment/12729093/TEZ-2383.3.patch
            against master revision 9ba4b1b.

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

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

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

            +1 javadoc. There were no new javadoc warning messages.

            +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

            +1 release audit. The applied patch does not increase the total number of release audit warnings.

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

            Test results: https://builds.apache.org/job/PreCommit-TEZ-Build/576//testReport/
            Console output: https://builds.apache.org/job/PreCommit-TEZ-Build/576//console

            This message is automatically generated.

            tezqa TezQA added a comment - +1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12729093/TEZ-2383.3.patch against master revision 9ba4b1b. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-TEZ-Build/576//testReport/ Console output: https://builds.apache.org/job/PreCommit-TEZ-Build/576//console This message is automatically generated.
            tezqa TezQA added a comment -

            +1 overall. Here are the results of testing the latest attachment
            http://issues.apache.org/jira/secure/attachment/12729093/TEZ-2383.3.patch
            against master revision 9ba4b1b.

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

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

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

            +1 javadoc. There were no new javadoc warning messages.

            +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

            +1 release audit. The applied patch does not increase the total number of release audit warnings.

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

            Test results: https://builds.apache.org/job/PreCommit-TEZ-Build/575//testReport/
            Console output: https://builds.apache.org/job/PreCommit-TEZ-Build/575//console

            This message is automatically generated.

            tezqa TezQA added a comment - +1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12729093/TEZ-2383.3.patch against master revision 9ba4b1b. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-TEZ-Build/575//testReport/ Console output: https://builds.apache.org/job/PreCommit-TEZ-Build/575//console This message is automatically generated.

            Added null checks.

            rajesh.balamohan Rajesh Balamohan added a comment - Added null checks.

            Attaching revised patch, which cleans up the processorcontext in LogicalIOProcessorRuntimeTask. This would also remove any stale references to objects related to inputs/outputs, which would internally release the memory buffers as well. gopalv, sseth - Please review.

            rajesh.balamohan Rajesh Balamohan added a comment - Attaching revised patch, which cleans up the processorcontext in LogicalIOProcessorRuntimeTask. This would also remove any stale references to objects related to inputs/outputs, which would internally release the memory buffers as well. gopalv , sseth - Please review.
            tezqa TezQA added a comment -

            -1 overall. Here are the results of testing the latest attachment
            http://issues.apache.org/jira/secure/attachment/12729042/TEZ-2383.1.patch
            against master revision 5b2f011.

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

            -1 tests included. The patch doesn't appear to include any new or modified tests.
            Please justify why no new tests are needed for this patch.
            Also please list what manual steps were performed to verify this patch.

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

            +1 javadoc. There were no new javadoc warning messages.

            +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

            +1 release audit. The applied patch does not increase the total number of release audit warnings.

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

            Test results: https://builds.apache.org/job/PreCommit-TEZ-Build/571//testReport/
            Console output: https://builds.apache.org/job/PreCommit-TEZ-Build/571//console

            This message is automatically generated.

            tezqa TezQA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12729042/TEZ-2383.1.patch against master revision 5b2f011. +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-TEZ-Build/571//testReport/ Console output: https://builds.apache.org/job/PreCommit-TEZ-Build/571//console This message is automatically generated.

            rajesh.balamohan: This is likely to be a fix limited to the sort implementation.

            You can broaden this fix to all possible edge types, by moving up the abstraction.

            Since ProcessorContext is the visible opaque implementation setup for the tez-api, it would be better to clean up that chain of references - cleaning up TezTaskContextImpl::runTimeTask within the ProcessorContext impl when the logical IO processor close is called.

            This follows a neat contract as any user-code which holds onto a processor context object after the close of the task runtime will be holding onto an empty/unusable shell of a context.

            gopalv Gopal Vijayaraghavan added a comment - rajesh.balamohan : This is likely to be a fix limited to the sort implementation. You can broaden this fix to all possible edge types, by moving up the abstraction. Since ProcessorContext is the visible opaque implementation setup for the tez-api, it would be better to clean up that chain of references - cleaning up TezTaskContextImpl::runTimeTask within the ProcessorContext impl when the logical IO processor close is called. This follows a neat contract as any user-code which holds onto a processor context object after the close of the task runtime will be holding onto an empty/unusable shell of a context.

            sseth - Can you please review when you find time?

            rajesh.balamohan Rajesh Balamohan added a comment - sseth - Can you please review when you find time?

            People

              rajesh.balamohan Rajesh Balamohan
              rajesh.balamohan Rajesh Balamohan
              Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: