[
https://issues.apache.org/jira/browse/SAMZA-896?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15194508#comment-15194508
]
Jagadish commented on SAMZA-896:
--------------------------------
Patch available:
https://reviews.apache.org/r/44820/
> Improvements to thread-safety in ContainerRequestState
> ------------------------------------------------------
>
> Key: SAMZA-896
> URL: https://issues.apache.org/jira/browse/SAMZA-896
> Project: Samza
> Issue Type: Improvement
> Reporter: Jagadish
> Assignee: Jagadish
>
> 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.
> {noformat}
> +-----------------------------+--------------+-----------------+-------------------------+---------------------+
> | 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 |
> +-----------------------------+--------------+-----------------+-------------------------+---------------------+
> {noformat}
> I'm proposing to make the ContainerRequestState class thread-safe by:
> i) Returning a copy of the list (if callers need the entire list - which they
> should not) for getContainersOnAHost, getRequestsQueue
> ii) 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.
>
> 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.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
