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

Jason Lowe commented on YARN-4365:
----------------------------------

Thanks for the patch, Kuhu!

There appears to be a mix of overriding and mocking approaches in the test that 
leads to a confusing test.  For example, setFileSystem was promoted to 
protected scope yet that's unnecessary in the current patch.  The test is also 
spying on the node label manager and mocking Configuration unnecessarily.

Instead of all the mocking and stubbing, I think it would be more 
straightforward to simply override setFileSystem and have the test use a "real" 
FileSystemNodeLabelsStore rather than a mocked one where we pass through 
various methods.  The only mock at that point would be the filesystem that 
would be set in the overridden setFileSystem method.

There's also a misleading comment in the test:
{code}
    // File Exists returns true the third time
    Mockito.when(myStore.fs.exists(Mockito.any(Path.class))).thenReturn(false);
{code}


> FileSystemNodeLabelStore should check for root dir existence on startup
> -----------------------------------------------------------------------
>
>                 Key: YARN-4365
>                 URL: https://issues.apache.org/jira/browse/YARN-4365
>             Project: Hadoop YARN
>          Issue Type: Bug
>          Components: resourcemanager
>    Affects Versions: 2.7.2
>            Reporter: Jason Lowe
>            Assignee: Kuhu Shukla
>         Attachments: YARN-4365-1.patch
>
>
> If the namenode is in safe mode for some reason then FileSystemNodeLabelStore 
> will prevent the RM from starting since it unconditionally tries to create 
> the root directory for the label store.  If the root directory already exists 
> and no labels are changing then we shouldn't prevent the RM from starting 
> even if the namenode is in safe mode.



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

Reply via email to