> On March 16, 2016, 12:16 a.m., Navina Ramesh wrote: > > samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerRequestState.java, > > line 323 > > <https://reviews.apache.org/r/44820/diff/3/?file=1299161#file1299161line323> > > > > 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.
If this is test only I would make it package private. This makes it much more difficult to have non-test code depend on it. Unfortunately documentation is easy to miss / ignore. Minor (ignorable) comment on null: if you consumer doesn't care whether there was an entry or whether the entry is empty Collections.emptyList is a little nicer. - Chris ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44820/#review123795 ----------------------------------------------------------- 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 > >
