> On March 14, 2016, 6:51 a.m., Navina Ramesh wrote:
> > samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerRequestState.java,
> > line 149
> > <https://reviews.apache.org/r/44775/diff/1/?file=1297033#file1297033line149>
> >
> > I remember specifically not simplifying the conditions so that we can
> > enable debug logging to troubleshoot any issues with the allocation logic.
> > I do agree there is some amount of readability issue here. But I feel that
> > the log lines are much useful until we stabilize the allocator.
>
> Navina Ramesh wrote:
> Having said that, I would like to hear the comments from others. I also
> think we need to add unit tests specifically for the conditions that we
> simplify / optimize, because our unit test coverage is pretty low at this
> point :(
Thanks for the feedback Navina! I dont think this refactoring makes debug
logging any worse than current.
1.Currently, I think the debug logging does not provide any additional insight.
For example, the debug log messages were exactly the same in the first 2 cases.
>> log.debug("Saving the container {} in the buffer for {}",
>> container.getId(), hostName);
If improving debug logging is the intent, I can change the messages to be
different.
2.In the 3rd case, I'm not sure the else block on line 149 is reachable. I
could not think of a scenario where the code will be executed. (which is why I
removed it)
>>else {
log.debug(
"This host was never requested. Hence, saving the container {} in
the buffer for ANY_HOST",
new Object[]{
hostName,
requestCountOnThisHost,
container.getId()
}
);
addToAllocatedContainerList(ANY_HOST, container);
- Jagadish
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44775/#review123359
-----------------------------------------------------------
On March 14, 2016, 6:02 a.m., Jagadish Venkatraman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44775/
> -----------------------------------------------------------
>
> (Updated March 14, 2016, 6:02 a.m.)
>
>
> Review request for samza, Boris Shkolnik, Jake Maes, Yi Pan (Data
> Infrastructure), Navina Ramesh, and Xinyu Liu.
>
>
> Repository: samza
>
>
> Description
> -------
>
> The current container allocator has some repetitive checks - when it decides
> to enqueue a container to ANY_HOST. I believe it is a good idea to eliminate
> those repetitive checks and refactor the allocation logic. It will help in
> long-term maintainability.
>
>
> Diffs
> -----
>
>
> samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerRequestState.java
> 3e3f48ce2b5c0802e8c7c4f09b632df2d2265c12
>
> Diff: https://reviews.apache.org/r/44775/diff/
>
>
> Testing
> -------
>
> All unit tests still pass.
>
>
> Thanks,
>
> Jagadish Venkatraman
>
>
