> On Oct. 26, 2016, 2:05 a.m., Jagadish Venkatraman wrote: > > samza-core/src/main/java/org/apache/samza/clustermanager/SamzaApplicationState.java, > > line 56 > > <https://reviews.apache.org/r/53163/diff/1/?file=1545047#file1545047line56> > > > > Prefer `volatile` here. > > > > There are 2 aspects to concurrency in Java: > > > > 1. Atomicity: > > In Java, reads and writes are atomic for references and for primitives > > (with the exception of double/long). > > > > 2. Visibility: > > Volatile guarantees visibility of updates, and defines a happens-before > > ordering between updates. > > > > For this case, we can use a `volatile` instead of using a per-object > > lock. (Using a non-volatile variable would still provide guarantees on > > atomicity but not visibility. Using a lock, will provide you both - except > > that it appears to be an overkill for this particular scenario). > > Branislav Cogic wrote: > So basicly, all that it was needed regarding SamzaApplicationState thread > safety is to put locks or volatile on these three variables. You have stated > in the javadocs that the scope of this change is larger. Also, making a bunch > of accessor methods seems to be needless. Am I getting something wrong?
>> You have stated in the javadocs that the scope of this change is larger. The RB for the Job coordinator refactor has been through a lot of changes and churn. (So, it's possible that the TODO was not accurate). The original intent then, was to move the `JobModelManager` out of `SamzaAppState`. >> making a bunch of accessor methods seems to be needless Yes, Agreed (on the accessors). I think for now, we can simply stick to making the 4 variables `volatile`. We can certainly, revisit this when we have integration with other resource managers like Mesos. - Jagadish ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53163/#review153840 ----------------------------------------------------------- On Oct. 27, 2016, 9:25 a.m., Branislav Cogic wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53163/ > ----------------------------------------------------------- > > (Updated Oct. 27, 2016, 9:25 a.m.) > > > Review request for samza and Jagadish Venkatraman. > > > Bugs: SAMZA-901 > https://issues.apache.org/jira/browse/SAMZA-901 > > > Repository: samza > > > Description > ------- > > SAMZA-901 SamzaAppState re-design for thread safety > > > Diffs > ----- > > > samza-core/src/main/java/org/apache/samza/clustermanager/AbstractContainerAllocator.java > d47f217 > > samza-core/src/main/java/org/apache/samza/clustermanager/ClusterBasedJobCoordinator.java > d0d4e34 > > samza-core/src/main/java/org/apache/samza/clustermanager/ContainerProcessManager.java > b4309d9 > > samza-core/src/main/java/org/apache/samza/clustermanager/HostAwareContainerAllocator.java > da73049 > > samza-core/src/main/java/org/apache/samza/clustermanager/SamzaApplicationState.java > cf91044 > > samza-core/src/main/scala/org/apache/samza/metrics/ContainerProcessManagerMetrics.scala > f24beb1 > > samza-core/src/test/java/org/apache/samza/clustermanager/TestContainerAllocator.java > 5351bc3 > > samza-core/src/test/java/org/apache/samza/clustermanager/TestContainerProcessManager.java > 0d61814 > > samza-core/src/test/java/org/apache/samza/clustermanager/TestHostAwareContainerAllocator.java > b6651f2 > > samza-yarn/src/main/java/org/apache/samza/job/yarn/YarnResourceManagerFactory.java > 988a8e8 > samza-yarn/src/main/resources/scalate/WEB-INF/views/index.scaml 93176ff > > samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterMetrics.scala > 8a5b4aa > > samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaYarnAppMasterLifecycle.scala > c9c1e18 > > samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaYarnAppMasterService.scala > 5f2bfc5 > > samza-yarn/src/main/scala/org/apache/samza/webapp/ApplicationMasterRestServlet.scala > cdd389c > > Diff: https://reviews.apache.org/r/53163/diff/ > > > Testing > ------- > > ./gradlew clean build > ./gradlew checkstyleMain checkstyleTest > > Checked that app state is shown correctly on web servlet index page. > > > Thanks, > > Branislav Cogic > >
