You are viewing a plain text version of this content. The canonical link for it is here.
Posted to common-dev@hadoop.apache.org by "Steve Loughran (JIRA)" <ji...@apache.org> on 2008/05/19 15:11:55 UTC
[jira] Created: (HADOOP-3415) JobEndNotifier isnt synchronized,
doesnt check state before acting
JobEndNotifier isnt synchronized, doesnt check state before acting
------------------------------------------------------------------
Key: HADOOP-3415
URL: https://issues.apache.org/jira/browse/HADOOP-3415
Project: Hadoop Core
Issue Type: Bug
Components: mapred
Affects Versions: 0.16.3
Reporter: Steve Loughran
Priority: Minor
JobEndNotifier is pretty hazardous inside.
1. the static startNotifier isnt synchronized, and doesnt check for being already running before it creates a new worker thread. It should be sycnhronized and a no-op if there is a live thread.
2. stopNotifier() should be a no-op if already stopped. It MUST NOT call thread.interrupt() in such a state, as thread may be null.
3. the registerNotification method also assumes that the static queue is non null.
Things would be a lot safer by making this class part of a JobTracker, not a singleton with static methods, as then you could more safely make assumptions about object state. This would not only eliminate a lot of reentrancy problems, but tie the life of the notifier to that of its owner, the JobTracker.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HADOOP-3415) JobEndNotifier isnt synchronized,
doesnt check state before acting
Posted by "Alejandro Abdelnur (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-3415?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12598933#action_12598933 ]
Alejandro Abdelnur commented on HADOOP-3415:
--------------------------------------------
I'm fine with such changes, as you claim the current implementation could have issues in a testing environment.
Regarding #3, the queue should be initialized then on startNotifier() to ensure it's clean.
> JobEndNotifier isnt synchronized, doesnt check state before acting
> ------------------------------------------------------------------
>
> Key: HADOOP-3415
> URL: https://issues.apache.org/jira/browse/HADOOP-3415
> Project: Hadoop Core
> Issue Type: Bug
> Components: mapred
> Affects Versions: 0.16.3
> Reporter: Steve Loughran
> Priority: Minor
>
> JobEndNotifier is pretty hazardous inside.
> 1. the static startNotifier isnt synchronized, and doesnt check for being already running before it creates a new worker thread. It should be sycnhronized and a no-op if there is a live thread.
> 2. stopNotifier() should be a no-op if already stopped. It MUST NOT call thread.interrupt() in such a state, as thread may be null.
> 3. the registerNotification method also assumes that the static queue is non null.
> Things would be a lot safer by making this class part of a JobTracker, not a singleton with static methods, as then you could more safely make assumptions about object state. This would not only eliminate a lot of reentrancy problems, but tie the life of the notifier to that of its owner, the JobTracker.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HADOOP-3415) JobEndNotifier isnt synchronized,
doesnt check state before acting
Posted by "Steve Loughran (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-3415?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12598305#action_12598305 ]
Steve Loughran commented on HADOOP-3415:
----------------------------------------
The JobTracker is only a singleton with the current deployment process. There is nothing to stop anyone imposing a different lifecycle, such as in unit tests. You can't do that now with any isolation or thread safety aound the JobEndNotifier. Create two job trackers, you leak one notifier thread.
1. The notifier is only live for the duration of the tracker - when jobTracker.stopTracker() is called, the notifier is closed too.
2. The notifier is only invoked for notifications in JobTracker, and JobInProgress, which has its own reference to jobTracker.
With all uses via JobTracker-linked objects, and with a live lifespan of that of a JobTracker, the obvious way to manage these instances with an instance created by the JobTracker, and access the same way. It would only be a singleton if the JobTracker were used as a singleton -such as when created via the command line- but not when a JobTracker is brought up differently.
> JobEndNotifier isnt synchronized, doesnt check state before acting
> ------------------------------------------------------------------
>
> Key: HADOOP-3415
> URL: https://issues.apache.org/jira/browse/HADOOP-3415
> Project: Hadoop Core
> Issue Type: Bug
> Components: mapred
> Affects Versions: 0.16.3
> Reporter: Steve Loughran
> Priority: Minor
>
> JobEndNotifier is pretty hazardous inside.
> 1. the static startNotifier isnt synchronized, and doesnt check for being already running before it creates a new worker thread. It should be sycnhronized and a no-op if there is a live thread.
> 2. stopNotifier() should be a no-op if already stopped. It MUST NOT call thread.interrupt() in such a state, as thread may be null.
> 3. the registerNotification method also assumes that the static queue is non null.
> Things would be a lot safer by making this class part of a JobTracker, not a singleton with static methods, as then you could more safely make assumptions about object state. This would not only eliminate a lot of reentrancy problems, but tie the life of the notifier to that of its owner, the JobTracker.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HADOOP-3415) JobEndNotifier isnt synchronized,
doesnt check state before acting
Posted by "Alejandro Abdelnur (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-3415?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12598192#action_12598192 ]
Alejandro Abdelnur commented on HADOOP-3415:
--------------------------------------------
On #1 and #2, due to the lifecycle and singleton characteristics of this class and the JT this is not really an issue not have it synchronized. I would say there is not need for this.
On #3, the queue is a static variable statically initialized, thus is always not null.
I suggest closing this issue as invalid.
> JobEndNotifier isnt synchronized, doesnt check state before acting
> ------------------------------------------------------------------
>
> Key: HADOOP-3415
> URL: https://issues.apache.org/jira/browse/HADOOP-3415
> Project: Hadoop Core
> Issue Type: Bug
> Components: mapred
> Affects Versions: 0.16.3
> Reporter: Steve Loughran
> Priority: Minor
>
> JobEndNotifier is pretty hazardous inside.
> 1. the static startNotifier isnt synchronized, and doesnt check for being already running before it creates a new worker thread. It should be sycnhronized and a no-op if there is a live thread.
> 2. stopNotifier() should be a no-op if already stopped. It MUST NOT call thread.interrupt() in such a state, as thread may be null.
> 3. the registerNotification method also assumes that the static queue is non null.
> Things would be a lot safer by making this class part of a JobTracker, not a singleton with static methods, as then you could more safely make assumptions about object state. This would not only eliminate a lot of reentrancy problems, but tie the life of the notifier to that of its owner, the JobTracker.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HADOOP-3415) JobEndNotifier isnt synchronized,
doesnt check state before acting
Posted by "Hadoop QA (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-3415?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12611443#action_12611443 ]
Hadoop QA commented on HADOOP-3415:
-----------------------------------
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12385399/hadoop-3415.patch
against trunk revision 674671.
+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 tests are needed for this patch.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
-1 core tests. The patch failed core unit tests.
-1 contrib tests. The patch failed contrib unit tests.
Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2808/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2808/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2808/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2808/console
This message is automatically generated.
> JobEndNotifier isnt synchronized, doesnt check state before acting
> ------------------------------------------------------------------
>
> Key: HADOOP-3415
> URL: https://issues.apache.org/jira/browse/HADOOP-3415
> Project: Hadoop Core
> Issue Type: Bug
> Components: mapred
> Affects Versions: 0.19.0
> Reporter: Steve Loughran
> Priority: Minor
> Attachments: hadoop-3415.patch
>
>
> JobEndNotifier is pretty hazardous inside.
> 1. the static startNotifier isnt synchronized, and doesnt check for being already running before it creates a new worker thread. It should be sycnhronized and a no-op if there is a live thread.
> 2. stopNotifier() should be a no-op if already stopped. It MUST NOT call thread.interrupt() in such a state, as thread may be null.
> 3. the registerNotification method also assumes that the static queue is non null.
> Things would be a lot safer by making this class part of a JobTracker, not a singleton with static methods, as then you could more safely make assumptions about object state. This would not only eliminate a lot of reentrancy problems, but tie the life of the notifier to that of its owner, the JobTracker.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (HADOOP-3415) JobEndNotifier isnt synchronized,
doesnt check state before acting
Posted by "Steve Loughran (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-3415?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Steve Loughran updated HADOOP-3415:
-----------------------------------
Attachment: hadoop-3415.patch
simple patch to fix a basic problem: the fact that a thread ref may be null during shutdown.
> JobEndNotifier isnt synchronized, doesnt check state before acting
> ------------------------------------------------------------------
>
> Key: HADOOP-3415
> URL: https://issues.apache.org/jira/browse/HADOOP-3415
> Project: Hadoop Core
> Issue Type: Bug
> Components: mapred
> Affects Versions: 0.19.0
> Reporter: Steve Loughran
> Priority: Minor
> Attachments: hadoop-3415.patch
>
>
> JobEndNotifier is pretty hazardous inside.
> 1. the static startNotifier isnt synchronized, and doesnt check for being already running before it creates a new worker thread. It should be sycnhronized and a no-op if there is a live thread.
> 2. stopNotifier() should be a no-op if already stopped. It MUST NOT call thread.interrupt() in such a state, as thread may be null.
> 3. the registerNotification method also assumes that the static queue is non null.
> Things would be a lot safer by making this class part of a JobTracker, not a singleton with static methods, as then you could more safely make assumptions about object state. This would not only eliminate a lot of reentrancy problems, but tie the life of the notifier to that of its owner, the JobTracker.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (HADOOP-3415) JobEndNotifier isnt synchronized,
doesnt check state before acting
Posted by "Steve Loughran (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-3415?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Steve Loughran updated HADOOP-3415:
-----------------------------------
Affects Version/s: (was: 0.16.3)
0.19.0
Status: Patch Available (was: Open)
> JobEndNotifier isnt synchronized, doesnt check state before acting
> ------------------------------------------------------------------
>
> Key: HADOOP-3415
> URL: https://issues.apache.org/jira/browse/HADOOP-3415
> Project: Hadoop Core
> Issue Type: Bug
> Components: mapred
> Affects Versions: 0.19.0
> Reporter: Steve Loughran
> Priority: Minor
> Attachments: hadoop-3415.patch
>
>
> JobEndNotifier is pretty hazardous inside.
> 1. the static startNotifier isnt synchronized, and doesnt check for being already running before it creates a new worker thread. It should be sycnhronized and a no-op if there is a live thread.
> 2. stopNotifier() should be a no-op if already stopped. It MUST NOT call thread.interrupt() in such a state, as thread may be null.
> 3. the registerNotification method also assumes that the static queue is non null.
> Things would be a lot safer by making this class part of a JobTracker, not a singleton with static methods, as then you could more safely make assumptions about object state. This would not only eliminate a lot of reentrancy problems, but tie the life of the notifier to that of its owner, the JobTracker.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (HADOOP-3415) JobEndNotifier isnt synchronized,
doesnt check state before acting
Posted by "Chris Douglas (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-3415?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Chris Douglas updated HADOOP-3415:
----------------------------------
Resolution: Invalid
Status: Resolved (was: Patch Available)
This change makes more sense integrated into HADOOP-3628, where the "different lifecycle" motivation is not hypothetical. I agree with Alejandro; this isn't a valid bug.
> JobEndNotifier isnt synchronized, doesnt check state before acting
> ------------------------------------------------------------------
>
> Key: HADOOP-3415
> URL: https://issues.apache.org/jira/browse/HADOOP-3415
> Project: Hadoop Core
> Issue Type: Bug
> Components: mapred
> Affects Versions: 0.19.0
> Reporter: Steve Loughran
> Priority: Minor
> Attachments: hadoop-3415.patch
>
>
> JobEndNotifier is pretty hazardous inside.
> 1. the static startNotifier isnt synchronized, and doesnt check for being already running before it creates a new worker thread. It should be sycnhronized and a no-op if there is a live thread.
> 2. stopNotifier() should be a no-op if already stopped. It MUST NOT call thread.interrupt() in such a state, as thread may be null.
> 3. the registerNotification method also assumes that the static queue is non null.
> Things would be a lot safer by making this class part of a JobTracker, not a singleton with static methods, as then you could more safely make assumptions about object state. This would not only eliminate a lot of reentrancy problems, but tie the life of the notifier to that of its owner, the JobTracker.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.