[jira] [Comment Edited] (TEZ-1951) Fix general findbugs warnings in tez-dag

["Siddharth Seth (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/TEZ-1951?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14278215#comment-14278215 ] 

Siddharth Seth edited comment on TEZ-1951 at 1/15/15 3:59 AM:
--------------------------------------------------------------

{code}
<Class name="org.apache.tez.dag.app.rm.YarnTaskSchedulerService$DelayedContainerManager"/>
{code}
I think  tryAssigningAll was meant to be inside the synchronized block in this case.

{code}+    this.localDirs = localDirs.clone();
+    this.logDirs = logDirs.clone();
{code}
localDirs and logDirs are only exposed to internal code. In runtime-internals I've ignored such warnings.

The other changes in DAGAppMaster - are they primarily formatting changes, with INTERNAL_ERROR added as a new case ?

{code}
-      this.pullAttempt = null;
       this.writeLock.unlock();
       // KKK Moves it out of the lock - so theoretically thread safe. Likely need a final inside
+      this.pullAttempt = null;
{code}
This moves resetting of pullAttempt outside of the lock, which can cause visibility issues. This may need to be a new finally block under the upper else condition.


{code}
+    return ( this.tsLogParams == null ? null : this.tsLogParams.clone() );
{code}
Never exposed to user code, safe to ignore ?

Rest looks good.



was (Author: sseth):
{code}
<Class name="org.apache.tez.dag.app.rm.YarnTaskSchedulerService$DelayedContainerManager"/>
{code}
I think was meant to be inside the synchronized block in this case.

{code}+    this.localDirs = localDirs.clone();
+    this.logDirs = logDirs.clone();
{code}
localDirs and logDirs are only exposed to internal code. In runtime-internals I've ignored such warnings.

The other changes in DAGAppMaster - are they primarily formatting changes, with INTERNAL_ERROR added as a new case ?

{code}
-      this.pullAttempt = null;
       this.writeLock.unlock();
       // KKK Moves it out of the lock - so theoretically thread safe. Likely need a final inside
+      this.pullAttempt = null;
{code}
This moves resetting of pullAttempt outside of the lock, which can cause visibility issues. This may need to be a new finally block under the upper else condition.


{code}
+    return ( this.tsLogParams == null ? null : this.tsLogParams.clone() );
{code}
Never exposed to user code, safe to ignore ?

Rest looks good.


> Fix general findbugs warnings in tez-dag
> ----------------------------------------
>
>                 Key: TEZ-1951
>                 URL: https://issues.apache.org/jira/browse/TEZ-1951
>             Project: Apache Tez
>          Issue Type: Sub-task
>            Reporter: Hitesh Shah
>            Assignee: Hitesh Shah
>         Attachments: TEZ-1951.1.patch
>
>




--
This message was sent by Atlassian JIRA
(v6.3.4#6332)