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