> 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).
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? - Branislav ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53163/#review153840 ----------------------------------------------------------- On Oct. 25, 2016, 8:25 a.m., Branislav Cogic wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53163/ > ----------------------------------------------------------- > > (Updated Oct. 25, 2016, 8: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 > >
