samza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Branislav Cogic <b.co...@levi9.com>
Subject Re: Review Request 53163: SAMZA-901: SamzaAppState re-design for thread safety
Date Thu, 27 Oct 2016 09:25:23 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message