Oozie
  1. Oozie
  2. OOZIE-684

A race condition may happen under stress while executing an interrupt command

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.2.0
    • Component/s: None
    • Labels:

      Description

      While executing an interrupt driven commands it may be executed from the map on one thread and executed from the queue at a different thread
      This will cause the following exception to be thrown:
      java.lang.IllegalStateException: CoordChangeXCommand already used.

        Activity

        Mohamed Battisha created issue -
        Mohamed Battisha made changes -
        Field Original Value New Value
        Summary A race condition may happen while executing the system under stress while executing an interrupt command A race condition may happen under stress while executing an interrupt command
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4035/
        -----------------------------------------------------------

        Review request for oozie.

        Summary
        -------

        While executing an interrupt driven commands it may be executed from the map on one thread and executed from the queue at a different thread
        This will cause the following exception to be thrown:
        java.lang.IllegalStateException: CoordChangeXCommand already used.

        • Avoiding throwing exception in case of interrupts and synchronizing the changes in [used]
        • Avoiding executing interrupt in case of commands that doesn't need locks
        • Changing the debug message for execute interrupt to explicitly mentioning it is an interrupt command

        This addresses bug OOZIE-684.
        https://issues.apache.org/jira/browse/OOZIE-684

        Diffs


        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java 1293381

        Diff: https://reviews.apache.org/r/4035/diff

        Testing
        -------

        Thanks,

        Mohamed

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4035/ ----------------------------------------------------------- Review request for oozie. Summary ------- While executing an interrupt driven commands it may be executed from the map on one thread and executed from the queue at a different thread This will cause the following exception to be thrown: java.lang.IllegalStateException: CoordChangeXCommand already used. Avoiding throwing exception in case of interrupts and synchronizing the changes in [used] Avoiding executing interrupt in case of commands that doesn't need locks Changing the debug message for execute interrupt to explicitly mentioning it is an interrupt command This addresses bug OOZIE-684 . https://issues.apache.org/jira/browse/OOZIE-684 Diffs http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java 1293381 Diff: https://reviews.apache.org/r/4035/diff Testing ------- Thanks, Mohamed
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4035/#review5359
        -----------------------------------------------------------

        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java
        <https://reviews.apache.org/r/4035/#comment11663>

        we should not allow getEntityKey == null to get the lock. Please add condition check here too.

        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java
        <https://reviews.apache.org/r/4035/#comment11664>

        if lock != null, isLockRequired() must be true.

        So you can remove isLockRequired().

        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java
        <https://reviews.apache.org/r/4035/#comment11666>

        change to

        if (!isLockRequired() || getInterruptMode() || lock != null) {
        }

        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java
        <https://reviews.apache.org/r/4035/#comment11662>

        there is no need for this condition check when you already check lock != null.

        lock != null already means:
        inInterrupt == false
        getEntityKey != null

        • Angelo K.

        On 2012-02-24 19:44:14, Mohamed Battisha wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4035/

        -----------------------------------------------------------

        (Updated 2012-02-24 19:44:14)

        Review request for oozie.

        Summary

        -------

        While executing an interrupt driven commands it may be executed from the map on one thread and executed from the queue at a different thread

        This will cause the following exception to be thrown:

        java.lang.IllegalStateException: CoordChangeXCommand already used.

        - Avoiding throwing exception in case of interrupts and synchronizing the changes in [used]

        - Avoiding executing interrupt in case of commands that doesn't need locks

        - Changing the debug message for execute interrupt to explicitly mentioning it is an interrupt command

        This addresses bug OOZIE-684.

        https://issues.apache.org/jira/browse/OOZIE-684

        Diffs

        -----

        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java 1293381

        Diff: https://reviews.apache.org/r/4035/diff

        Testing

        -------

        Thanks,

        Mohamed

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4035/#review5359 ----------------------------------------------------------- http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java < https://reviews.apache.org/r/4035/#comment11663 > we should not allow getEntityKey == null to get the lock. Please add condition check here too. http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java < https://reviews.apache.org/r/4035/#comment11664 > if lock != null, isLockRequired() must be true. So you can remove isLockRequired(). http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java < https://reviews.apache.org/r/4035/#comment11666 > change to if (!isLockRequired() || getInterruptMode() || lock != null) { } http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java < https://reviews.apache.org/r/4035/#comment11662 > there is no need for this condition check when you already check lock != null. lock != null already means: inInterrupt == false getEntityKey != null Angelo K. On 2012-02-24 19:44:14, Mohamed Battisha wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4035/ ----------------------------------------------------------- (Updated 2012-02-24 19:44:14) Review request for oozie. Summary ------- While executing an interrupt driven commands it may be executed from the map on one thread and executed from the queue at a different thread This will cause the following exception to be thrown: java.lang.IllegalStateException: CoordChangeXCommand already used. - Avoiding throwing exception in case of interrupts and synchronizing the changes in [used] - Avoiding executing interrupt in case of commands that doesn't need locks - Changing the debug message for execute interrupt to explicitly mentioning it is an interrupt command This addresses bug OOZIE-684 . https://issues.apache.org/jira/browse/OOZIE-684 Diffs ----- http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java 1293381 Diff: https://reviews.apache.org/r/4035/diff Testing ------- Thanks, Mohamed
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-02-27 21:12:35, Angelo K. Huang wrote:

        > http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java, line 195

        > <https://reviews.apache.org/r/4035/diff/2/?file=85862#file85862line195>

        >

        > we should not allow getEntityKey == null to get the lock. Please add condition check here too.

        sure will add it. I was assuming the null checking should happen generically while getting the write lock

        On 2012-02-27 21:12:35, Angelo K. Huang wrote:

        > http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java, line 263

        > <https://reviews.apache.org/r/4035/diff/2/?file=85862#file85862line263>

        >

        > if lock != null, isLockRequired() must be true.

        >

        > So you can remove isLockRequired().

        Yep, good catch

        On 2012-02-27 21:12:35, Angelo K. Huang wrote:

        > http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java, line 331

        > <https://reviews.apache.org/r/4035/diff/2/?file=85862#file85862line331>

        >

        > there is no need for this condition check when you already check lock != null.

        >

        > lock != null already means:

        > inInterrupt == false

        > getEntityKey != null

        if the entity key is checked while acquiring lock as you requested, we can remove it here
        But, we have to check for inInterrupt, otherwise we can be in an endless loop while executing interrupts within interrupts

        • Mohamed

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4035/#review5359
        -----------------------------------------------------------

        On 2012-02-24 19:44:14, Mohamed Battisha wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4035/

        -----------------------------------------------------------

        (Updated 2012-02-24 19:44:14)

        Review request for oozie.

        Summary

        -------

        While executing an interrupt driven commands it may be executed from the map on one thread and executed from the queue at a different thread

        This will cause the following exception to be thrown:

        java.lang.IllegalStateException: CoordChangeXCommand already used.

        - Avoiding throwing exception in case of interrupts and synchronizing the changes in [used]

        - Avoiding executing interrupt in case of commands that doesn't need locks

        - Changing the debug message for execute interrupt to explicitly mentioning it is an interrupt command

        This addresses bug OOZIE-684.

        https://issues.apache.org/jira/browse/OOZIE-684

        Diffs

        -----

        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java 1293381

        Diff: https://reviews.apache.org/r/4035/diff

        Testing

        -------

        Thanks,

        Mohamed

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-02-27 21:12:35, Angelo K. Huang wrote: > http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java , line 195 > < https://reviews.apache.org/r/4035/diff/2/?file=85862#file85862line195 > > > we should not allow getEntityKey == null to get the lock. Please add condition check here too. sure will add it. I was assuming the null checking should happen generically while getting the write lock On 2012-02-27 21:12:35, Angelo K. Huang wrote: > http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java , line 263 > < https://reviews.apache.org/r/4035/diff/2/?file=85862#file85862line263 > > > if lock != null, isLockRequired() must be true. > > So you can remove isLockRequired(). Yep, good catch On 2012-02-27 21:12:35, Angelo K. Huang wrote: > http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java , line 331 > < https://reviews.apache.org/r/4035/diff/2/?file=85862#file85862line331 > > > there is no need for this condition check when you already check lock != null. > > lock != null already means: > inInterrupt == false > getEntityKey != null if the entity key is checked while acquiring lock as you requested, we can remove it here But, we have to check for inInterrupt, otherwise we can be in an endless loop while executing interrupts within interrupts Mohamed ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4035/#review5359 ----------------------------------------------------------- On 2012-02-24 19:44:14, Mohamed Battisha wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4035/ ----------------------------------------------------------- (Updated 2012-02-24 19:44:14) Review request for oozie. Summary ------- While executing an interrupt driven commands it may be executed from the map on one thread and executed from the queue at a different thread This will cause the following exception to be thrown: java.lang.IllegalStateException: CoordChangeXCommand already used. - Avoiding throwing exception in case of interrupts and synchronizing the changes in [used] - Avoiding executing interrupt in case of commands that doesn't need locks - Changing the debug message for execute interrupt to explicitly mentioning it is an interrupt command This addresses bug OOZIE-684 . https://issues.apache.org/jira/browse/OOZIE-684 Diffs ----- http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java 1293381 Diff: https://reviews.apache.org/r/4035/diff Testing ------- Thanks, Mohamed
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4035/#review5367
        -----------------------------------------------------------

        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java
        <https://reviews.apache.org/r/4035/#comment11685>

        Is it possible to put this line within if(used) ?
        This is the common execution path for all commands. Any synchronization, will add a overhead on the processing.

        I know there could be double-locking issue for that.

        • Mohammad

        On 2012-02-24 19:44:14, Mohamed Battisha wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4035/

        -----------------------------------------------------------

        (Updated 2012-02-24 19:44:14)

        Review request for oozie.

        Summary

        -------

        While executing an interrupt driven commands it may be executed from the map on one thread and executed from the queue at a different thread

        This will cause the following exception to be thrown:

        java.lang.IllegalStateException: CoordChangeXCommand already used.

        - Avoiding throwing exception in case of interrupts and synchronizing the changes in [used]

        - Avoiding executing interrupt in case of commands that doesn't need locks

        - Changing the debug message for execute interrupt to explicitly mentioning it is an interrupt command

        This addresses bug OOZIE-684.

        https://issues.apache.org/jira/browse/OOZIE-684

        Diffs

        -----

        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java 1293381

        Diff: https://reviews.apache.org/r/4035/diff

        Testing

        -------

        Thanks,

        Mohamed

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4035/#review5367 ----------------------------------------------------------- http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java < https://reviews.apache.org/r/4035/#comment11685 > Is it possible to put this line within if(used) ? This is the common execution path for all commands. Any synchronization, will add a overhead on the processing. I know there could be double-locking issue for that. Mohammad On 2012-02-24 19:44:14, Mohamed Battisha wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4035/ ----------------------------------------------------------- (Updated 2012-02-24 19:44:14) Review request for oozie. Summary ------- While executing an interrupt driven commands it may be executed from the map on one thread and executed from the queue at a different thread This will cause the following exception to be thrown: java.lang.IllegalStateException: CoordChangeXCommand already used. - Avoiding throwing exception in case of interrupts and synchronizing the changes in [used] - Avoiding executing interrupt in case of commands that doesn't need locks - Changing the debug message for execute interrupt to explicitly mentioning it is an interrupt command This addresses bug OOZIE-684 . https://issues.apache.org/jira/browse/OOZIE-684 Diffs ----- http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java 1293381 Diff: https://reviews.apache.org/r/4035/diff Testing ------- Thanks, Mohamed
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-02-27 22:38:10, Mohammad Islam wrote:

        > http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java, line 230

        > <https://reviews.apache.org/r/4035/diff/2/?file=85862#file85862line230>

        >

        > Is it possible to put this line within if(used) ?

        > This is the common execution path for all commands. Any synchronization, will add a overhead on the processing.

        >

        > I know there could be double-locking issue for that.

        I believe we should include it for setting up the used flag as well which happen after the if condition , otherwise we may end up with some issues

        • Mohamed

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4035/#review5367
        -----------------------------------------------------------

        On 2012-02-24 19:44:14, Mohamed Battisha wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4035/

        -----------------------------------------------------------

        (Updated 2012-02-24 19:44:14)

        Review request for oozie.

        Summary

        -------

        While executing an interrupt driven commands it may be executed from the map on one thread and executed from the queue at a different thread

        This will cause the following exception to be thrown:

        java.lang.IllegalStateException: CoordChangeXCommand already used.

        - Avoiding throwing exception in case of interrupts and synchronizing the changes in [used]

        - Avoiding executing interrupt in case of commands that doesn't need locks

        - Changing the debug message for execute interrupt to explicitly mentioning it is an interrupt command

        This addresses bug OOZIE-684.

        https://issues.apache.org/jira/browse/OOZIE-684

        Diffs

        -----

        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java 1293381

        Diff: https://reviews.apache.org/r/4035/diff

        Testing

        -------

        Thanks,

        Mohamed

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-02-27 22:38:10, Mohammad Islam wrote: > http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java , line 230 > < https://reviews.apache.org/r/4035/diff/2/?file=85862#file85862line230 > > > Is it possible to put this line within if(used) ? > This is the common execution path for all commands. Any synchronization, will add a overhead on the processing. > > I know there could be double-locking issue for that. I believe we should include it for setting up the used flag as well which happen after the if condition , otherwise we may end up with some issues Mohamed ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4035/#review5367 ----------------------------------------------------------- On 2012-02-24 19:44:14, Mohamed Battisha wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4035/ ----------------------------------------------------------- (Updated 2012-02-24 19:44:14) Review request for oozie. Summary ------- While executing an interrupt driven commands it may be executed from the map on one thread and executed from the queue at a different thread This will cause the following exception to be thrown: java.lang.IllegalStateException: CoordChangeXCommand already used. - Avoiding throwing exception in case of interrupts and synchronizing the changes in [used] - Avoiding executing interrupt in case of commands that doesn't need locks - Changing the debug message for execute interrupt to explicitly mentioning it is an interrupt command This addresses bug OOZIE-684 . https://issues.apache.org/jira/browse/OOZIE-684 Diffs ----- http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java 1293381 Diff: https://reviews.apache.org/r/4035/diff Testing ------- Thanks, Mohamed
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4035/
        -----------------------------------------------------------

        (Updated 2012-02-27 23:05:10.609795)

        Review request for oozie.

        Changes
        -------

        Updated based on reviewers comments

        Summary
        -------

        While executing an interrupt driven commands it may be executed from the map on one thread and executed from the queue at a different thread
        This will cause the following exception to be thrown:
        java.lang.IllegalStateException: CoordChangeXCommand already used.

        • Avoiding throwing exception in case of interrupts and synchronizing the changes in [used]
        • Avoiding executing interrupt in case of commands that doesn't need locks
        • Changing the debug message for execute interrupt to explicitly mentioning it is an interrupt command

        This addresses bug OOZIE-684.
        https://issues.apache.org/jira/browse/OOZIE-684

        Diffs (updated)


        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java 1293381

        Diff: https://reviews.apache.org/r/4035/diff

        Testing
        -------

        Thanks,

        Mohamed

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4035/ ----------------------------------------------------------- (Updated 2012-02-27 23:05:10.609795) Review request for oozie. Changes ------- Updated based on reviewers comments Summary ------- While executing an interrupt driven commands it may be executed from the map on one thread and executed from the queue at a different thread This will cause the following exception to be thrown: java.lang.IllegalStateException: CoordChangeXCommand already used. Avoiding throwing exception in case of interrupts and synchronizing the changes in [used] Avoiding executing interrupt in case of commands that doesn't need locks Changing the debug message for execute interrupt to explicitly mentioning it is an interrupt command This addresses bug OOZIE-684 . https://issues.apache.org/jira/browse/OOZIE-684 Diffs (updated) http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java 1293381 Diff: https://reviews.apache.org/r/4035/diff Testing ------- Thanks, Mohamed
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4035/#review5381
        -----------------------------------------------------------

        Comment on the conservative locking mechanism.

        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java
        <https://reviews.apache.org/r/4035/#comment11707>

        Same comment as Mohammad - can you check how to optimize this?

        Ref:
        http://www.cs.wustl.edu/~schmidt/PDF/DC-Locking.pdf
        http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html

        • Santhosh

        On 2012-02-27 23:05:10, Mohamed Battisha wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4035/

        -----------------------------------------------------------

        (Updated 2012-02-27 23:05:10)

        Review request for oozie.

        Summary

        -------

        While executing an interrupt driven commands it may be executed from the map on one thread and executed from the queue at a different thread

        This will cause the following exception to be thrown:

        java.lang.IllegalStateException: CoordChangeXCommand already used.

        - Avoiding throwing exception in case of interrupts and synchronizing the changes in [used]

        - Avoiding executing interrupt in case of commands that doesn't need locks

        - Changing the debug message for execute interrupt to explicitly mentioning it is an interrupt command

        This addresses bug OOZIE-684.

        https://issues.apache.org/jira/browse/OOZIE-684

        Diffs

        -----

        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java 1293381

        Diff: https://reviews.apache.org/r/4035/diff

        Testing

        -------

        Thanks,

        Mohamed

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4035/#review5381 ----------------------------------------------------------- Comment on the conservative locking mechanism. http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java < https://reviews.apache.org/r/4035/#comment11707 > Same comment as Mohammad - can you check how to optimize this? Ref: http://www.cs.wustl.edu/~schmidt/PDF/DC-Locking.pdf http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html Santhosh On 2012-02-27 23:05:10, Mohamed Battisha wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4035/ ----------------------------------------------------------- (Updated 2012-02-27 23:05:10) Review request for oozie. Summary ------- While executing an interrupt driven commands it may be executed from the map on one thread and executed from the queue at a different thread This will cause the following exception to be thrown: java.lang.IllegalStateException: CoordChangeXCommand already used. - Avoiding throwing exception in case of interrupts and synchronizing the changes in [used] - Avoiding executing interrupt in case of commands that doesn't need locks - Changing the debug message for execute interrupt to explicitly mentioning it is an interrupt command This addresses bug OOZIE-684 . https://issues.apache.org/jira/browse/OOZIE-684 Diffs ----- http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java 1293381 Diff: https://reviews.apache.org/r/4035/diff Testing ------- Thanks, Mohamed
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-02-28 03:48:16, Santhosh Srinivasan wrote:

        > http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java, line 234

        > <https://reviews.apache.org/r/4035/diff/3/?file=86114#file86114line234>

        >

        > Same comment as Mohammad - can you check how to optimize this?

        >

        > Ref:

        > http://www.cs.wustl.edu/~schmidt/PDF/DC-Locking.pdf

        > http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html

        interesting read and it is very common to use in the singelton pattern in which we write once and read a lot. In this case,it will be great to optimize on the reads. mainly to avoid the synchronization in the common case which is reading. The main goal here is to avoid a needless thread contention in case of reads.

        In our case it is bit different, as our common case is to set the used flag [write] which happen for all the commands. Assuming the same command may be called from different threads, we should synchronize around the write [avoiding two threads set the used flag at the same time and hence continue the execution path which may cause a problem].

        So, if the command is called only once which is our common case, it is unavoidable to have this one call go outside the synchronization block.

        • Mohamed

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4035/#review5381
        -----------------------------------------------------------

        On 2012-02-27 23:05:10, Mohamed Battisha wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4035/

        -----------------------------------------------------------

        (Updated 2012-02-27 23:05:10)

        Review request for oozie.

        Summary

        -------

        While executing an interrupt driven commands it may be executed from the map on one thread and executed from the queue at a different thread

        This will cause the following exception to be thrown:

        java.lang.IllegalStateException: CoordChangeXCommand already used.

        - Avoiding throwing exception in case of interrupts and synchronizing the changes in [used]

        - Avoiding executing interrupt in case of commands that doesn't need locks

        - Changing the debug message for execute interrupt to explicitly mentioning it is an interrupt command

        This addresses bug OOZIE-684.

        https://issues.apache.org/jira/browse/OOZIE-684

        Diffs

        -----

        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java 1293381

        Diff: https://reviews.apache.org/r/4035/diff

        Testing

        -------

        Thanks,

        Mohamed

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-02-28 03:48:16, Santhosh Srinivasan wrote: > http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java , line 234 > < https://reviews.apache.org/r/4035/diff/3/?file=86114#file86114line234 > > > Same comment as Mohammad - can you check how to optimize this? > > Ref: > http://www.cs.wustl.edu/~schmidt/PDF/DC-Locking.pdf > http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html interesting read and it is very common to use in the singelton pattern in which we write once and read a lot. In this case,it will be great to optimize on the reads. mainly to avoid the synchronization in the common case which is reading. The main goal here is to avoid a needless thread contention in case of reads. In our case it is bit different, as our common case is to set the used flag [write] which happen for all the commands. Assuming the same command may be called from different threads, we should synchronize around the write [avoiding two threads set the used flag at the same time and hence continue the execution path which may cause a problem] . So, if the command is called only once which is our common case, it is unavoidable to have this one call go outside the synchronization block. Mohamed ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4035/#review5381 ----------------------------------------------------------- On 2012-02-27 23:05:10, Mohamed Battisha wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4035/ ----------------------------------------------------------- (Updated 2012-02-27 23:05:10) Review request for oozie. Summary ------- While executing an interrupt driven commands it may be executed from the map on one thread and executed from the queue at a different thread This will cause the following exception to be thrown: java.lang.IllegalStateException: CoordChangeXCommand already used. - Avoiding throwing exception in case of interrupts and synchronizing the changes in [used] - Avoiding executing interrupt in case of commands that doesn't need locks - Changing the debug message for execute interrupt to explicitly mentioning it is an interrupt command This addresses bug OOZIE-684 . https://issues.apache.org/jira/browse/OOZIE-684 Diffs ----- http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java 1293381 Diff: https://reviews.apache.org/r/4035/diff Testing ------- Thanks, Mohamed
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4035/
        -----------------------------------------------------------

        (Updated 2012-02-28 18:25:19.938370)

        Review request for oozie.

        Changes
        -------

        Updating the execute interrupts to check only for the interrupt mode

        Summary
        -------

        While executing an interrupt driven commands it may be executed from the map on one thread and executed from the queue at a different thread
        This will cause the following exception to be thrown:
        java.lang.IllegalStateException: CoordChangeXCommand already used.

        • Avoiding throwing exception in case of interrupts and synchronizing the changes in [used]
        • Avoiding executing interrupt in case of commands that doesn't need locks
        • Changing the debug message for execute interrupt to explicitly mentioning it is an interrupt command

        This addresses bug OOZIE-684.
        https://issues.apache.org/jira/browse/OOZIE-684

        Diffs (updated)


        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java 1293381

        Diff: https://reviews.apache.org/r/4035/diff

        Testing
        -------

        Thanks,

        Mohamed

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4035/ ----------------------------------------------------------- (Updated 2012-02-28 18:25:19.938370) Review request for oozie. Changes ------- Updating the execute interrupts to check only for the interrupt mode Summary ------- While executing an interrupt driven commands it may be executed from the map on one thread and executed from the queue at a different thread This will cause the following exception to be thrown: java.lang.IllegalStateException: CoordChangeXCommand already used. Avoiding throwing exception in case of interrupts and synchronizing the changes in [used] Avoiding executing interrupt in case of commands that doesn't need locks Changing the debug message for execute interrupt to explicitly mentioning it is an interrupt command This addresses bug OOZIE-684 . https://issues.apache.org/jira/browse/OOZIE-684 Diffs (updated) http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java 1293381 Diff: https://reviews.apache.org/r/4035/diff Testing ------- Thanks, Mohamed
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-02-27 21:12:35, Angelo K. Huang wrote:

        > http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java, line 331

        > <https://reviews.apache.org/r/4035/diff/2/?file=85862#file85862line331>

        >

        > there is no need for this condition check when you already check lock != null.

        >

        > lock != null already means:

        > inInterrupt == false

        > getEntityKey != null

        Mohamed Battisha wrote:

        if the entity key is checked while acquiring lock as you requested, we can remove it here

        But, we have to check for inInterrupt, otherwise we can be in an endless loop while executing interrupts within interrupts

        Before you enter this executeInterrupts(), you already checked lock != null. So these two conditions check are redundant.

        • Angelo K.

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4035/#review5359
        -----------------------------------------------------------

        On 2012-02-28 18:25:19, Mohamed Battisha wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4035/

        -----------------------------------------------------------

        (Updated 2012-02-28 18:25:19)

        Review request for oozie.

        Summary

        -------

        While executing an interrupt driven commands it may be executed from the map on one thread and executed from the queue at a different thread

        This will cause the following exception to be thrown:

        java.lang.IllegalStateException: CoordChangeXCommand already used.

        - Avoiding throwing exception in case of interrupts and synchronizing the changes in [used]

        - Avoiding executing interrupt in case of commands that doesn't need locks

        - Changing the debug message for execute interrupt to explicitly mentioning it is an interrupt command

        This addresses bug OOZIE-684.

        https://issues.apache.org/jira/browse/OOZIE-684

        Diffs

        -----

        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java 1293381

        Diff: https://reviews.apache.org/r/4035/diff

        Testing

        -------

        Thanks,

        Mohamed

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-02-27 21:12:35, Angelo K. Huang wrote: > http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java , line 331 > < https://reviews.apache.org/r/4035/diff/2/?file=85862#file85862line331 > > > there is no need for this condition check when you already check lock != null. > > lock != null already means: > inInterrupt == false > getEntityKey != null Mohamed Battisha wrote: if the entity key is checked while acquiring lock as you requested, we can remove it here But, we have to check for inInterrupt, otherwise we can be in an endless loop while executing interrupts within interrupts Before you enter this executeInterrupts(), you already checked lock != null. So these two conditions check are redundant. Angelo K. ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4035/#review5359 ----------------------------------------------------------- On 2012-02-28 18:25:19, Mohamed Battisha wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4035/ ----------------------------------------------------------- (Updated 2012-02-28 18:25:19) Review request for oozie. Summary ------- While executing an interrupt driven commands it may be executed from the map on one thread and executed from the queue at a different thread This will cause the following exception to be thrown: java.lang.IllegalStateException: CoordChangeXCommand already used. - Avoiding throwing exception in case of interrupts and synchronizing the changes in [used] - Avoiding executing interrupt in case of commands that doesn't need locks - Changing the debug message for execute interrupt to explicitly mentioning it is an interrupt command This addresses bug OOZIE-684 . https://issues.apache.org/jira/browse/OOZIE-684 Diffs ----- http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java 1293381 Diff: https://reviews.apache.org/r/4035/diff Testing ------- Thanks, Mohamed
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-02-27 21:12:35, Angelo K. Huang wrote:

        > http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java, line 331

        > <https://reviews.apache.org/r/4035/diff/2/?file=85862#file85862line331>

        >

        > there is no need for this condition check when you already check lock != null.

        >

        > lock != null already means:

        > inInterrupt == false

        > getEntityKey != null

        Mohamed Battisha wrote:

        if the entity key is checked while acquiring lock as you requested, we can remove it here

        But, we have to check for inInterrupt, otherwise we can be in an endless loop while executing interrupts within interrupts

        Angelo K. Huang wrote:

        Before you enter this executeInterrupts(), you already checked lock != null. So these two conditions check are redundant.

        nope, you have to check if you are executing regular or interrupt commands. We should avoid executing interrupts within interrupts.

        • Mohamed

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4035/#review5359
        -----------------------------------------------------------

        On 2012-02-28 18:25:19, Mohamed Battisha wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4035/

        -----------------------------------------------------------

        (Updated 2012-02-28 18:25:19)

        Review request for oozie.

        Summary

        -------

        While executing an interrupt driven commands it may be executed from the map on one thread and executed from the queue at a different thread

        This will cause the following exception to be thrown:

        java.lang.IllegalStateException: CoordChangeXCommand already used.

        - Avoiding throwing exception in case of interrupts and synchronizing the changes in [used]

        - Avoiding executing interrupt in case of commands that doesn't need locks

        - Changing the debug message for execute interrupt to explicitly mentioning it is an interrupt command

        This addresses bug OOZIE-684.

        https://issues.apache.org/jira/browse/OOZIE-684

        Diffs

        -----

        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java 1293381

        Diff: https://reviews.apache.org/r/4035/diff

        Testing

        -------

        Thanks,

        Mohamed

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-02-27 21:12:35, Angelo K. Huang wrote: > http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java , line 331 > < https://reviews.apache.org/r/4035/diff/2/?file=85862#file85862line331 > > > there is no need for this condition check when you already check lock != null. > > lock != null already means: > inInterrupt == false > getEntityKey != null Mohamed Battisha wrote: if the entity key is checked while acquiring lock as you requested, we can remove it here But, we have to check for inInterrupt, otherwise we can be in an endless loop while executing interrupts within interrupts Angelo K. Huang wrote: Before you enter this executeInterrupts(), you already checked lock != null. So these two conditions check are redundant. nope, you have to check if you are executing regular or interrupt commands. We should avoid executing interrupts within interrupts. Mohamed ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4035/#review5359 ----------------------------------------------------------- On 2012-02-28 18:25:19, Mohamed Battisha wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4035/ ----------------------------------------------------------- (Updated 2012-02-28 18:25:19) Review request for oozie. Summary ------- While executing an interrupt driven commands it may be executed from the map on one thread and executed from the queue at a different thread This will cause the following exception to be thrown: java.lang.IllegalStateException: CoordChangeXCommand already used. - Avoiding throwing exception in case of interrupts and synchronizing the changes in [used] - Avoiding executing interrupt in case of commands that doesn't need locks - Changing the debug message for execute interrupt to explicitly mentioning it is an interrupt command This addresses bug OOZIE-684 . https://issues.apache.org/jira/browse/OOZIE-684 Diffs ----- http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java 1293381 Diff: https://reviews.apache.org/r/4035/diff Testing ------- Thanks, Mohamed
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4035/#review5572
        -----------------------------------------------------------

        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java
        <https://reviews.apache.org/r/4035/#comment12064>

        Wouldn't be simpler to use an AtomicBoolean for used?

        if (used.compare(true, true))

        { .... }

        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java
        <https://reviews.apache.org/r/4035/#comment12065>

        shouldn't be

        if (this.getInterrupMode())

        { ... }

        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java
        <https://reviews.apache.org/r/4035/#comment12067>

        also, the getInterruptMode() method should be isInterruptMode()

        • Alejandro

        On 2012-02-28 18:25:19, Mohamed Battisha wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4035/

        -----------------------------------------------------------

        (Updated 2012-02-28 18:25:19)

        Review request for oozie.

        Summary

        -------

        While executing an interrupt driven commands it may be executed from the map on one thread and executed from the queue at a different thread

        This will cause the following exception to be thrown:

        java.lang.IllegalStateException: CoordChangeXCommand already used.

        - Avoiding throwing exception in case of interrupts and synchronizing the changes in [used]

        - Avoiding executing interrupt in case of commands that doesn't need locks

        - Changing the debug message for execute interrupt to explicitly mentioning it is an interrupt command

        This addresses bug OOZIE-684.

        https://issues.apache.org/jira/browse/OOZIE-684

        Diffs

        -----

        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java 1293381

        Diff: https://reviews.apache.org/r/4035/diff

        Testing

        -------

        Thanks,

        Mohamed

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4035/#review5572 ----------------------------------------------------------- http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java < https://reviews.apache.org/r/4035/#comment12064 > Wouldn't be simpler to use an AtomicBoolean for used? if (used.compare(true, true)) { .... } http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java < https://reviews.apache.org/r/4035/#comment12065 > shouldn't be if (this.getInterrupMode()) { ... } http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java < https://reviews.apache.org/r/4035/#comment12067 > also, the getInterruptMode() method should be isInterruptMode() Alejandro On 2012-02-28 18:25:19, Mohamed Battisha wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4035/ ----------------------------------------------------------- (Updated 2012-02-28 18:25:19) Review request for oozie. Summary ------- While executing an interrupt driven commands it may be executed from the map on one thread and executed from the queue at a different thread This will cause the following exception to be thrown: java.lang.IllegalStateException: CoordChangeXCommand already used. - Avoiding throwing exception in case of interrupts and synchronizing the changes in [used] - Avoiding executing interrupt in case of commands that doesn't need locks - Changing the debug message for execute interrupt to explicitly mentioning it is an interrupt command This addresses bug OOZIE-684 . https://issues.apache.org/jira/browse/OOZIE-684 Diffs ----- http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java 1293381 Diff: https://reviews.apache.org/r/4035/diff Testing ------- Thanks, Mohamed
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-02 19:06:59, Alejandro Abdelnur wrote:

        > http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java, line 235

        > <https://reviews.apache.org/r/4035/diff/4/?file=86276#file86276line235>

        >

        > Wouldn't be simpler to use an AtomicBoolean for used?

        >

        > if (used.compare(true, true)) { bq. > .... bq. > }

        you mean compareAndSet .... [yes it should work]

        On 2012-03-02 19:06:59, Alejandro Abdelnur wrote:

        > http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java, line 237

        > <https://reviews.apache.org/r/4035/diff/4/?file=86276#file86276line237>

        >

        > shouldn't be

        >

        > if (this.getInterrupMode()) { bq. > ... bq. > }

        will use it instead

        On 2012-03-02 19:06:59, Alejandro Abdelnur wrote:

        > http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java, line 259

        > <https://reviews.apache.org/r/4035/diff/4/?file=86276#file86276line259>

        >

        > also, the getInterruptMode() method should be isInterruptMode()

        will change it

        • Mohamed

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4035/#review5572
        -----------------------------------------------------------

        On 2012-02-28 18:25:19, Mohamed Battisha wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4035/

        -----------------------------------------------------------

        (Updated 2012-02-28 18:25:19)

        Review request for oozie.

        Summary

        -------

        While executing an interrupt driven commands it may be executed from the map on one thread and executed from the queue at a different thread

        This will cause the following exception to be thrown:

        java.lang.IllegalStateException: CoordChangeXCommand already used.

        - Avoiding throwing exception in case of interrupts and synchronizing the changes in [used]

        - Avoiding executing interrupt in case of commands that doesn't need locks

        - Changing the debug message for execute interrupt to explicitly mentioning it is an interrupt command

        This addresses bug OOZIE-684.

        https://issues.apache.org/jira/browse/OOZIE-684

        Diffs

        -----

        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java 1293381

        Diff: https://reviews.apache.org/r/4035/diff

        Testing

        -------

        Thanks,

        Mohamed

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-02 19:06:59, Alejandro Abdelnur wrote: > http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java , line 235 > < https://reviews.apache.org/r/4035/diff/4/?file=86276#file86276line235 > > > Wouldn't be simpler to use an AtomicBoolean for used? > > if (used.compare(true, true)) { bq. > .... bq. > } you mean compareAndSet .... [yes it should work] On 2012-03-02 19:06:59, Alejandro Abdelnur wrote: > http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java , line 237 > < https://reviews.apache.org/r/4035/diff/4/?file=86276#file86276line237 > > > shouldn't be > > if (this.getInterrupMode()) { bq. > ... bq. > } will use it instead On 2012-03-02 19:06:59, Alejandro Abdelnur wrote: > http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java , line 259 > < https://reviews.apache.org/r/4035/diff/4/?file=86276#file86276line259 > > > also, the getInterruptMode() method should be isInterruptMode() will change it Mohamed ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4035/#review5572 ----------------------------------------------------------- On 2012-02-28 18:25:19, Mohamed Battisha wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4035/ ----------------------------------------------------------- (Updated 2012-02-28 18:25:19) Review request for oozie. Summary ------- While executing an interrupt driven commands it may be executed from the map on one thread and executed from the queue at a different thread This will cause the following exception to be thrown: java.lang.IllegalStateException: CoordChangeXCommand already used. - Avoiding throwing exception in case of interrupts and synchronizing the changes in [used] - Avoiding executing interrupt in case of commands that doesn't need locks - Changing the debug message for execute interrupt to explicitly mentioning it is an interrupt command This addresses bug OOZIE-684 . https://issues.apache.org/jira/browse/OOZIE-684 Diffs ----- http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java 1293381 Diff: https://reviews.apache.org/r/4035/diff Testing ------- Thanks, Mohamed
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4035/
        -----------------------------------------------------------

        (Updated 2012-03-02 23:03:35.875277)

        Review request for oozie.

        Changes
        -------

        Updated based on reviewers comments

        Summary
        -------

        While executing an interrupt driven commands it may be executed from the map on one thread and executed from the queue at a different thread
        This will cause the following exception to be thrown:
        java.lang.IllegalStateException: CoordChangeXCommand already used.

        • Avoiding throwing exception in case of interrupts and synchronizing the changes in [used]
        • Avoiding executing interrupt in case of commands that doesn't need locks
        • Changing the debug message for execute interrupt to explicitly mentioning it is an interrupt command

        This addresses bug OOZIE-684.
        https://issues.apache.org/jira/browse/OOZIE-684

        Diffs (updated)


        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/util/XCallable.java 1296506
        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/test/java/org/apache/oozie/command/TestCommand.java 1296506
        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/test/java/org/apache/oozie/service/TestCallableQueueService.java 1296506
        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/service/CallableQueueService.java 1296506
        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/Command.java 1296506
        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java 1296506

        Diff: https://reviews.apache.org/r/4035/diff

        Testing
        -------

        Thanks,

        Mohamed

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4035/ ----------------------------------------------------------- (Updated 2012-03-02 23:03:35.875277) Review request for oozie. Changes ------- Updated based on reviewers comments Summary ------- While executing an interrupt driven commands it may be executed from the map on one thread and executed from the queue at a different thread This will cause the following exception to be thrown: java.lang.IllegalStateException: CoordChangeXCommand already used. Avoiding throwing exception in case of interrupts and synchronizing the changes in [used] Avoiding executing interrupt in case of commands that doesn't need locks Changing the debug message for execute interrupt to explicitly mentioning it is an interrupt command This addresses bug OOZIE-684 . https://issues.apache.org/jira/browse/OOZIE-684 Diffs (updated) http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/util/XCallable.java 1296506 http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/test/java/org/apache/oozie/command/TestCommand.java 1296506 http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/test/java/org/apache/oozie/service/TestCallableQueueService.java 1296506 http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/service/CallableQueueService.java 1296506 http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/Command.java 1296506 http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java 1296506 Diff: https://reviews.apache.org/r/4035/diff Testing ------- Thanks, Mohamed
        Gavin made changes -
        Workflow jira [ 12652392 ] no-reopen-closed, patch-avail [ 12655960 ]
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4035/#review5667
        -----------------------------------------------------------

        Ship it!

        +1

        • Mohammad

        On 2012-03-02 23:03:35, Mohamed Battisha wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4035/

        -----------------------------------------------------------

        (Updated 2012-03-02 23:03:35)

        Review request for oozie.

        Summary

        -------

        While executing an interrupt driven commands it may be executed from the map on one thread and executed from the queue at a different thread

        This will cause the following exception to be thrown:

        java.lang.IllegalStateException: CoordChangeXCommand already used.

        - Avoiding throwing exception in case of interrupts and synchronizing the changes in [used]

        - Avoiding executing interrupt in case of commands that doesn't need locks

        - Changing the debug message for execute interrupt to explicitly mentioning it is an interrupt command

        This addresses bug OOZIE-684.

        https://issues.apache.org/jira/browse/OOZIE-684

        Diffs

        -----

        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/util/XCallable.java 1296506

        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/test/java/org/apache/oozie/command/TestCommand.java 1296506

        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/test/java/org/apache/oozie/service/TestCallableQueueService.java 1296506

        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/service/CallableQueueService.java 1296506

        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/Command.java 1296506

        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java 1296506

        Diff: https://reviews.apache.org/r/4035/diff

        Testing

        -------

        Thanks,

        Mohamed

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4035/#review5667 ----------------------------------------------------------- Ship it! +1 Mohammad On 2012-03-02 23:03:35, Mohamed Battisha wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4035/ ----------------------------------------------------------- (Updated 2012-03-02 23:03:35) Review request for oozie. Summary ------- While executing an interrupt driven commands it may be executed from the map on one thread and executed from the queue at a different thread This will cause the following exception to be thrown: java.lang.IllegalStateException: CoordChangeXCommand already used. - Avoiding throwing exception in case of interrupts and synchronizing the changes in [used] - Avoiding executing interrupt in case of commands that doesn't need locks - Changing the debug message for execute interrupt to explicitly mentioning it is an interrupt command This addresses bug OOZIE-684 . https://issues.apache.org/jira/browse/OOZIE-684 Diffs ----- http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/util/XCallable.java 1296506 http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/test/java/org/apache/oozie/command/TestCommand.java 1296506 http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/test/java/org/apache/oozie/service/TestCallableQueueService.java 1296506 http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/service/CallableQueueService.java 1296506 http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/Command.java 1296506 http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java 1296506 Diff: https://reviews.apache.org/r/4035/diff Testing ------- Thanks, Mohamed
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-02-27 21:12:35, Angelo K. Huang wrote:

        > http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java, line 331

        > <https://reviews.apache.org/r/4035/diff/2/?file=85862#file85862line331>

        >

        > there is no need for this condition check when you already check lock != null.

        >

        > lock != null already means:

        > inInterrupt == false

        > getEntityKey != null

        Mohamed Battisha wrote:

        if the entity key is checked while acquiring lock as you requested, we can remove it here

        But, we have to check for inInterrupt, otherwise we can be in an endless loop while executing interrupts within interrupts

        Angelo K. Huang wrote:

        Before you enter this executeInterrupts(), you already checked lock != null. So these two conditions check are redundant.

        Mohamed Battisha wrote:

        nope, you have to check if you are executing regular or interrupt commands. We should avoid executing interrupts within interrupts.

        First you get the lock only when it is not in the Interrupt mode. Then, executeInterrupts will only called when lock is not NULL. So in the function executeInterrupts(), this inInterruptMode() will always be false.

        ==> if (isLockRequired() && !this.inInterruptMode())

        { Instrumentation.Cron acquireLockCron = new Instrumentation.Cron(); acquireLockCron.start(); acquireLock(); acquireLockCron.stop(); instrumentation.addCron(INSTRUMENTATION_GROUP, getName() + ".acquireLock", acquireLockCron); }

        ==> if (lock != null)

        { this.executeInterrupts(); }
        • Angelo K.

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4035/#review5359
        -----------------------------------------------------------

        On 2012-03-02 23:03:35, Mohamed Battisha wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4035/

        -----------------------------------------------------------

        (Updated 2012-03-02 23:03:35)

        Review request for oozie.

        Summary

        -------

        While executing an interrupt driven commands it may be executed from the map on one thread and executed from the queue at a different thread

        This will cause the following exception to be thrown:

        java.lang.IllegalStateException: CoordChangeXCommand already used.

        - Avoiding throwing exception in case of interrupts and synchronizing the changes in [used]

        - Avoiding executing interrupt in case of commands that doesn't need locks

        - Changing the debug message for execute interrupt to explicitly mentioning it is an interrupt command

        This addresses bug OOZIE-684.

        https://issues.apache.org/jira/browse/OOZIE-684

        Diffs

        -----

        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/util/XCallable.java 1296506

        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/test/java/org/apache/oozie/command/TestCommand.java 1296506

        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/test/java/org/apache/oozie/service/TestCallableQueueService.java 1296506

        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/service/CallableQueueService.java 1296506

        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/Command.java 1296506

        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java 1296506

        Diff: https://reviews.apache.org/r/4035/diff

        Testing

        -------

        Thanks,

        Mohamed

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-02-27 21:12:35, Angelo K. Huang wrote: > http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java , line 331 > < https://reviews.apache.org/r/4035/diff/2/?file=85862#file85862line331 > > > there is no need for this condition check when you already check lock != null. > > lock != null already means: > inInterrupt == false > getEntityKey != null Mohamed Battisha wrote: if the entity key is checked while acquiring lock as you requested, we can remove it here But, we have to check for inInterrupt, otherwise we can be in an endless loop while executing interrupts within interrupts Angelo K. Huang wrote: Before you enter this executeInterrupts(), you already checked lock != null. So these two conditions check are redundant. Mohamed Battisha wrote: nope, you have to check if you are executing regular or interrupt commands. We should avoid executing interrupts within interrupts. First you get the lock only when it is not in the Interrupt mode. Then, executeInterrupts will only called when lock is not NULL. So in the function executeInterrupts(), this inInterruptMode() will always be false. ==> if (isLockRequired() && !this.inInterruptMode()) { Instrumentation.Cron acquireLockCron = new Instrumentation.Cron(); acquireLockCron.start(); acquireLock(); acquireLockCron.stop(); instrumentation.addCron(INSTRUMENTATION_GROUP, getName() + ".acquireLock", acquireLockCron); } ==> if (lock != null) { this.executeInterrupts(); } Angelo K. ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4035/#review5359 ----------------------------------------------------------- On 2012-03-02 23:03:35, Mohamed Battisha wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4035/ ----------------------------------------------------------- (Updated 2012-03-02 23:03:35) Review request for oozie. Summary ------- While executing an interrupt driven commands it may be executed from the map on one thread and executed from the queue at a different thread This will cause the following exception to be thrown: java.lang.IllegalStateException: CoordChangeXCommand already used. - Avoiding throwing exception in case of interrupts and synchronizing the changes in [used] - Avoiding executing interrupt in case of commands that doesn't need locks - Changing the debug message for execute interrupt to explicitly mentioning it is an interrupt command This addresses bug OOZIE-684 . https://issues.apache.org/jira/browse/OOZIE-684 Diffs ----- http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/util/XCallable.java 1296506 http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/test/java/org/apache/oozie/command/TestCommand.java 1296506 http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/test/java/org/apache/oozie/service/TestCallableQueueService.java 1296506 http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/service/CallableQueueService.java 1296506 http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/Command.java 1296506 http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java 1296506 Diff: https://reviews.apache.org/r/4035/diff Testing ------- Thanks, Mohamed
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4035/
        -----------------------------------------------------------

        (Updated 2012-03-07 03:51:47.441182)

        Review request for oozie.

        Changes
        -------

        Updated based on the reviewer comments

        Summary
        -------

        While executing an interrupt driven commands it may be executed from the map on one thread and executed from the queue at a different thread
        This will cause the following exception to be thrown:
        java.lang.IllegalStateException: CoordChangeXCommand already used.

        • Avoiding throwing exception in case of interrupts and synchronizing the changes in [used]
        • Avoiding executing interrupt in case of commands that doesn't need locks
        • Changing the debug message for execute interrupt to explicitly mentioning it is an interrupt command

        This addresses bug OOZIE-684.
        https://issues.apache.org/jira/browse/OOZIE-684

        Diffs (updated)


        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/Command.java 1296506
        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java 1296506
        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/service/CallableQueueService.java 1296506
        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/util/XCallable.java 1296506
        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/test/java/org/apache/oozie/command/TestCommand.java 1296506
        http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/test/java/org/apache/oozie/service/TestCallableQueueService.java 1296506

        Diff: https://reviews.apache.org/r/4035/diff

        Testing
        -------

        Thanks,

        Mohamed

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4035/ ----------------------------------------------------------- (Updated 2012-03-07 03:51:47.441182) Review request for oozie. Changes ------- Updated based on the reviewer comments Summary ------- While executing an interrupt driven commands it may be executed from the map on one thread and executed from the queue at a different thread This will cause the following exception to be thrown: java.lang.IllegalStateException: CoordChangeXCommand already used. Avoiding throwing exception in case of interrupts and synchronizing the changes in [used] Avoiding executing interrupt in case of commands that doesn't need locks Changing the debug message for execute interrupt to explicitly mentioning it is an interrupt command This addresses bug OOZIE-684 . https://issues.apache.org/jira/browse/OOZIE-684 Diffs (updated) http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/Command.java 1296506 http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/command/XCommand.java 1296506 http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/service/CallableQueueService.java 1296506 http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/main/java/org/apache/oozie/util/XCallable.java 1296506 http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/test/java/org/apache/oozie/command/TestCommand.java 1296506 http://svn.apache.org/repos/asf/incubator/oozie/trunk/core/src/test/java/org/apache/oozie/service/TestCallableQueueService.java 1296506 Diff: https://reviews.apache.org/r/4035/diff Testing ------- Thanks, Mohamed
        Hide
        Virag Kothari added a comment -

        Already committed to 3.2

        Show
        Virag Kothari added a comment - Already committed to 3.2
        Virag Kothari made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Assignee Mohamed Battisha [ mbattish@yahoo.com ]
        Fix Version/s 3.2.0 [ 12320149 ]
        Resolution Fixed [ 1 ]

          People

          • Assignee:
            Mohamed Battisha
            Reporter:
            Mohamed Battisha
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 168h
              168h
              Remaining:
              Remaining Estimate - 168h
              168h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development