samza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jagadish Venkatraman <jagadish1...@gmail.com>
Subject Re: Review Request 44820: SAMZA-896 : Improvements to thread-safety in ContainerRequestState
Date Tue, 15 Mar 2016 03:19:47 GMT

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

(Updated March 15, 2016, 3:19 a.m.)


Review request for samza, Boris Shkolnik, Jake Maes, Yi Pan (Data Infrastructure), Navina
Ramesh, and Xinyu Liu.


Repository: samza


Description (updated)
-------

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.


Diffs (updated)
-----

  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