[jira] [Commented] (YARN-10185) Container executor fields should be volatile
[ https://issues.apache.org/jira/browse/YARN-10185?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17074416#comment-17074416 ] Adam Antal commented on YARN-10185: --- I think this can be closed based on the comments above. Thanks for the discussion [~ebadger], [~pbacsko], [~denes.gerencser]. > Container executor fields should be volatile > > > Key: YARN-10185 > URL: https://issues.apache.org/jira/browse/YARN-10185 > Project: Hadoop YARN > Issue Type: Bug > Components: yarn >Affects Versions: 3.3.0 >Reporter: Adam Antal >Assignee: Denes Gerencser >Priority: Major > > In YARN-7226 and YARN-10173 there two fields have been added to the > {{ContainerExecutor}} class. These fields are set through {{#setConf()}} only > once, but in a multithreaded environment the volatile keyword should be added > to ensure that the updated fields are used. > > Related piece of code: > {code:java} > private String[] whitelistVars; > private int exitCodeFileTimeout = > YarnConfiguration.DEFAULT_NM_CONTAINER_EXECUTOR_EXIT_FILE_TIMEOUT; > {code} > This can be hardly unit tested, but the bug could cause the UT added by > YARN-10173 to fail in a very small percentage. > Thanks for [~denes.gerencser] for finding this. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
[jira] [Commented] (YARN-10185) Container executor fields should be volatile
[ https://issues.apache.org/jira/browse/YARN-10185?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17055446#comment-17055446 ] Eric Badger commented on YARN-10185: I agree with [~pbacsko]. Since initialization is synchronized and happens before the variable is used, I don't see a need for volatile. > Container executor fields should be volatile > > > Key: YARN-10185 > URL: https://issues.apache.org/jira/browse/YARN-10185 > Project: Hadoop YARN > Issue Type: Bug > Components: yarn >Affects Versions: 3.3.0 >Reporter: Adam Antal >Assignee: Denes Gerencser >Priority: Major > > In YARN-7226 and YARN-10173 there two fields have been added to the > {{ContainerExecutor}} class. These fields are set through {{#setConf()}} only > once, but in a multithreaded environment the volatile keyword should be added > to ensure that the updated fields are used. > > Related piece of code: > {code:java} > private String[] whitelistVars; > private int exitCodeFileTimeout = > YarnConfiguration.DEFAULT_NM_CONTAINER_EXECUTOR_EXIT_FILE_TIMEOUT; > {code} > This can be hardly unit tested, but the bug could cause the UT added by > YARN-10173 to fail in a very small percentage. > Thanks for [~denes.gerencser] for finding this. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
[jira] [Commented] (YARN-10185) Container executor fields should be volatile
[ https://issues.apache.org/jira/browse/YARN-10185?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17055383#comment-17055383 ] Denes Gerencser commented on YARN-10185: Thanks [~pbacsko] for looking into this! The whole call hierarchy is so diverging, hard to track... By memory model only those threads are guaranteed to see the "flushed" values who also synchronize via the same monitor. So in case of {{ContainerExecutor.setConf()}} (which sets the value of {{exitCodeFileTimeout}}) eventually called by {{AbstractService.init()}}, all caller threads of {{ContainerExecutor.reacquireContainer()}} (it reads the value of {{exitCodeFileTimeout}}) should also use the same monitor, {{AbstractService .stateChangeLock}} in this case. Do we think that this condition is always met? (It was suspicious for me that {{pidFiles}} in this class is a concurrent collection.) > Container executor fields should be volatile > > > Key: YARN-10185 > URL: https://issues.apache.org/jira/browse/YARN-10185 > Project: Hadoop YARN > Issue Type: Bug > Components: yarn >Affects Versions: 3.3.0 >Reporter: Adam Antal >Assignee: Denes Gerencser >Priority: Major > > In YARN-7226 and YARN-10173 there two fields have been added to the > {{ContainerExecutor}} class. These fields are set through {{#setConf()}} only > once, but in a multithreaded environment the volatile keyword should be added > to ensure that the updated fields are used. > > Related piece of code: > {code:java} > private String[] whitelistVars; > private int exitCodeFileTimeout = > YarnConfiguration.DEFAULT_NM_CONTAINER_EXECUTOR_EXIT_FILE_TIMEOUT; > {code} > This can be hardly unit tested, but the bug could cause the UT added by > YARN-10173 to fail in a very small percentage. > Thanks for [~denes.gerencser] for finding this. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
[jira] [Commented] (YARN-10185) Container executor fields should be volatile
[ https://issues.apache.org/jira/browse/YARN-10185?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17055089#comment-17055089 ] Peter Bacsko commented on YARN-10185: - I don't think that {{volatile}} is needed at all. The initialization occurs from the following code path: {noformat} ContainerExecutor.setConf(Configuration) ReflectionUtils.setConf(Object, Configuration) ReflectionUtils.newInstance(Class, Configuration) NodeManager.createContainerExecutor(Configuration) NodeManager.serviceInit(Configuration) AbstractService.init(Configuration) ... {noformat} The initialization in {{AbstractService.init()}} occurs inside a {{synchronized}} block, which happens to act as a memory fence as soon as you exit the block. That is, all pending writes and reads are flushed: https://www.infoq.com/articles/memory_barriers_jvm_concurrency/ Also, there's {{LinuxContainerExecutor}} which calls {{super.setConf()}}, essentially all the variables there should be handled too. But it's not necessary, given the JVM memory model. > Container executor fields should be volatile > > > Key: YARN-10185 > URL: https://issues.apache.org/jira/browse/YARN-10185 > Project: Hadoop YARN > Issue Type: Bug > Components: yarn >Affects Versions: 3.3.0 >Reporter: Adam Antal >Assignee: Denes Gerencser >Priority: Major > > In YARN-7226 and YARN-10173 there two fields have been added to the > {{ContainerExecutor}} class. These fields are set through {{#setConf()}} only > once, but in a multithreaded environment the volatile keyword should be added > to ensure that the updated fields are used. > > Related piece of code: > {code:java} > private String[] whitelistVars; > private int exitCodeFileTimeout = > YarnConfiguration.DEFAULT_NM_CONTAINER_EXECUTOR_EXIT_FILE_TIMEOUT; > {code} > This can be hardly unit tested, but the bug could cause the UT added by > YARN-10173 to fail in a very small percentage. > Thanks for [~denes.gerencser] for finding this. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
[jira] [Commented] (YARN-10185) Container executor fields should be volatile
[ https://issues.apache.org/jira/browse/YARN-10185?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17054783#comment-17054783 ] Adam Antal commented on YARN-10185: --- [~ebadger] can you please take a look at this? I wonder how this can be verified. > Container executor fields should be volatile > > > Key: YARN-10185 > URL: https://issues.apache.org/jira/browse/YARN-10185 > Project: Hadoop YARN > Issue Type: Bug > Components: yarn >Affects Versions: 3.3.0 >Reporter: Adam Antal >Assignee: Denes Gerencser >Priority: Major > > In YARN-7226 and YARN-10173 there two fields have been added to the > {{ContainerExecutor}} class. These fields are set through {{#setConf()}} only > once, but in a multithreaded environment the volatile keyword should be added > to ensure that the updated fields are used. > > Related piece of code: > {code:java} > private String[] whitelistVars; > private int exitCodeFileTimeout = > YarnConfiguration.DEFAULT_NM_CONTAINER_EXECUTOR_EXIT_FILE_TIMEOUT; > {code} > This can be hardly unit tested, but the bug could cause the UT added by > YARN-10173 to fail in a very small percentage. > Thanks for [~denes.gerencser] for finding this. -- This message was sent by Atlassian Jira (v8.3.4#803005) - To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org