----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44820/#review123572 -----------------------------------------------------------
Ship it! Ship It! - Jake Maes On March 15, 2016, 1:06 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, 1:06 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, don't return the original map, > 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. > > > 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 > >
