samza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Navina Ramesh <nram...@linkedin.com>
Subject Re: Review Request 44820: SAMZA-896 : Improvements to thread-safety in ContainerRequestState
Date Wed, 16 Mar 2016 00:16:31 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44820/#review123795
-----------------------------------------------------------


Fix it, then Ship it!




lgtm. Fix it and then Ship it!


samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerRequestState.java (line 297)
<https://reviews.apache.org/r/44820/#comment186058>

    Fix the warning for Javadoc. 
    If you use /** .. */ for docs, you should provide any @param and @return (if applicable)
for the method.



samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerRequestState.java (line 310)
<https://reviews.apache.org/r/44820/#comment186059>

    Move these comments to the java doc comment.
    
    In this case, you are using this method in Line 217 when releasing containers. 
    
    In general, if the method is only used in tests, please call it out in the javadocs of
that method.



samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerRequestState.java (line 324)
<https://reviews.apache.org/r/44820/#comment186060>

    Seems like this is used only by unit tests. Please correct me if I am wrong.
    
    Can you also explain what you meaning in the TODO?
    
    Also, same comment as the javadoc above. Add @return


- Navina Ramesh


On March 15, 2016, 3:25 a.m., Jagadish Venkatraman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44820/
> -----------------------------------------------------------
> 
> (Updated March 15, 2016, 3:25 a.m.)
> 
> 
> Review request for samza, Boris Shkolnik, Jake Maes, Yi Pan (Data Infrastructure), Navina
Ramesh, and Xinyu Liu.
> 
> 
> Repository: samza
> 
> 
> Description
> -------
> 
> The ContainerRequestState class is currently not thread-safe. The class's methods and
state variables are called from both the ContainerAllocator and the AMRMCallback handler.
> Here's an analysis I summarized by looking at the current implementation. From the below
table, getContainersOnAHost() returns the entire list of containers to the AbstractContainerAllocator
which reads it. However, the Callback thread invokes methods that write to the allocatedContainerQueue.
(causing a potential race condition)
> 
> +-----------------------------+--------------+-----------------+-------------------------+---------------------+
> | Method                      | requestQueue | hostsToCountMap | allocatedContainerQueue
| ThreadsAccesedFrom  |
> +-----------------------------+--------------+-----------------+-------------------------+---------------------+
> | updateRequestState          | write        | write/read      | write              
    | Callback, Allocator |
> +-----------------------------+--------------+-----------------+-------------------------+---------------------+
> | addContainer                |              | read            | write              
    | Callback            |
> +-----------------------------+--------------+-----------------+-------------------------+---------------------+
> | updateStateAfterAssignment  | write        | write           | read               
    | Allocator           |
> +-----------------------------+--------------+-----------------+-------------------------+---------------------+
> | releaseExtraContainers      | write        | write           | write              
    | Allocator           |
> +-----------------------------+--------------+-----------------+-------------------------+---------------------+
> | releaseUnstartableContainer |              |                 |                    
    | Allocator           |
> +-----------------------------+--------------+-----------------+-------------------------+---------------------+
> | getContainersOnAHost        |              |                 | read               
    | Allocator           |
> +-----------------------------+--------------+-----------------+-------------------------+---------------------+
> | getRequestsQueue            | read         |                 |                    
    | Allocator           |
> +-----------------------------+--------------+-----------------+-------------------------+---------------------+
> 
> I'm proposing to make the ContainerRequestState class thread-safe by:
> i) Expose more granular synchronized APIs for things like:
> 1. Getting num containers on a host.
> 2. Getting the number of pending requests.
> 3. Checking if the queue is empty etc. / peeking a queue etc.
> 
> ii) Getting rid of the non-synchronized methods.
> 
> iii) Ensuring that methods that return a map/a list, don't return the original map/list,
but a defensive copy instead.
> 
> The other methods are already synchronized.
> 
> This improvement is beneficial because:
> 1. This allows us to reason about concurrency more effectively when using the request-state
in multi-threaded contexts.
> 2. It's cleaner for the ContainerRequestState class to expose higher level methods and
not expose the guts of the class in its entirety.
> 
> --- Diff 2---
> Modified the getContainersOnHost method to make a defensive copy.
> 
> --- Diff 3---
> -Fix a small typo ';'
> 
> 
> Diffs
> -----
> 
>   samza-yarn/src/main/java/org/apache/samza/job/yarn/AbstractContainerAllocator.java
b4789e62beb1120f11a8101664b10c34ae930e58 
>   samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerRequestState.java 3e3f48ce2b5c0802e8c7c4f09b632df2d2265c12

>   samza-yarn/src/test/java/org/apache/samza/job/yarn/TestContainerAllocator.java 2b1bdab3c8de3184e930c244a8cae55813c33565

>   samza-yarn/src/test/java/org/apache/samza/job/yarn/TestContainerRequestState.java 402fe784120ac40ed542d9fa60d6a6d7df9c8cda

>   samza-yarn/src/test/java/org/apache/samza/job/yarn/TestHostAwareContainerAllocator.java
0c7a09f3e4c4c2ce6788be729d0bf4a294243c68 
>   samza-yarn/src/test/java/org/apache/samza/job/yarn/TestSamzaTaskManager.java 9da1edf6ff165ef0306de8730853ad30551a9831

> 
> Diff: https://reviews.apache.org/r/44820/diff/
> 
> 
> Testing
> -------
> 
> All unit tests pass.
> 
> 
> Thanks,
> 
> Jagadish Venkatraman
> 
>


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