----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53163/#review153840 -----------------------------------------------------------
samza-core/src/main/java/org/apache/samza/clustermanager/SamzaApplicationState.java (line 50) <https://reviews.apache.org/r/53163/#comment223323> 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). samza-core/src/main/java/org/apache/samza/clustermanager/SamzaApplicationState.java (line 102) <https://reviews.apache.org/r/53163/#comment223324> Same comment. Prefer to use a `volatile` here. - Jagadish Venkatraman 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 > >
