You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@samza.apache.org by Jake Maes <ja...@gmail.com> on 2016/02/02 00:34:02 UTC
Review Request 43074: SAMZA-866 Refactor and fix Container allocation
logic.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43074/
-----------------------------------------------------------
Review request for samza.
Repository: samza
Description
-------
Bug:
ContainerAllocator and HostAwareContainerAllocator have different try-catch logic in their run() methods. The HostAwareContainerAllocator try-catch is outside the while loop, which would cause it to stop allocating containers in the event of an exception. ContainerAllocator correctly has the try-catch inside the loop
Fix:
Refactor the loop and try-catch to a common run() method in AbstractContainerAllocator
Change the log type to WARN for the general exception case
Refactor some duplicate code in ContainerRequestState
Diffs
-----
samza-yarn/src/main/java/org/apache/samza/job/yarn/AbstractContainerAllocator.java 9ee2daccc3c44202308637207a084def81b49c09
samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerAllocator.java 7c57a866114aabf76a36d3b7ca4c5810628e0c77
samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerRequestState.java ab3061eae2cfc2da6681ce2034492b165d0d8b96
samza-yarn/src/main/java/org/apache/samza/job/yarn/HostAwareContainerAllocator.java ff22dbfe5537ba2a3c55c4e6063680ff4c9e55f4
Diff: https://reviews.apache.org/r/43074/diff/
Testing
-------
Thanks,
Jake Maes
Re: Review Request 43074: SAMZA-866 Refactor and fix Container
allocation logic.
Posted by Navina Ramesh <nr...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43074/#review118073
-----------------------------------------------------------
Ship it!
samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerAllocator.java (line 54)
<https://reviews.apache.org/r/43074/#comment179332>
Minor nit:
I think the max character line limit is 120 character. Ideally, it should be :
```java
while (!containerRequestState.getRequestsQueue() &&
allocatedContainers != null &&
allocatedContainers.size() > 0) {
...
}
```
- Navina Ramesh
On Feb. 3, 2016, 8:04 p.m., Jake Maes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43074/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2016, 8:04 p.m.)
>
>
> Review request for samza, Navina Ramesh and Yi Pan (Data Infrastructure).
>
>
> Repository: samza
>
>
> Description
> -------
>
> Bug:
> ContainerAllocator and HostAwareContainerAllocator have different try-catch logic in their run() methods. The HostAwareContainerAllocator try-catch is outside the while loop, which would cause it to stop allocating containers in the event of an exception. ContainerAllocator correctly has the try-catch inside the loop
>
> Fix:
> * Refactor the loop and try-catch to a common run() method in AbstractContainerAllocator
> * Change the log type to WARN for the general exception case
> * Refactor some duplicate code in ContainerRequestState
>
>
> Diffs
> -----
>
> samza-yarn/src/main/java/org/apache/samza/job/yarn/AbstractContainerAllocator.java 9ee2daccc3c44202308637207a084def81b49c09
> samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerAllocator.java 7c57a866114aabf76a36d3b7ca4c5810628e0c77
> samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerRequestState.java ab3061eae2cfc2da6681ce2034492b165d0d8b96
> samza-yarn/src/main/java/org/apache/samza/job/yarn/HostAwareContainerAllocator.java ff22dbfe5537ba2a3c55c4e6063680ff4c9e55f4
>
> Diff: https://reviews.apache.org/r/43074/diff/
>
>
> Testing
> -------
>
> Unit tests still pass.
>
>
> Thanks,
>
> Jake Maes
>
>
Re: Review Request 43074: SAMZA-866 Refactor and fix Container
allocation logic.
Posted by "Yi Pan (Data Infrastructure)" <yi...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43074/#review118603
-----------------------------------------------------------
Ship it!
Ship It!
- Yi Pan (Data Infrastructure)
On Feb. 3, 2016, 8:04 p.m., Jake Maes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43074/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2016, 8:04 p.m.)
>
>
> Review request for samza, Navina Ramesh and Yi Pan (Data Infrastructure).
>
>
> Repository: samza
>
>
> Description
> -------
>
> Bug:
> ContainerAllocator and HostAwareContainerAllocator have different try-catch logic in their run() methods. The HostAwareContainerAllocator try-catch is outside the while loop, which would cause it to stop allocating containers in the event of an exception. ContainerAllocator correctly has the try-catch inside the loop
>
> Fix:
> * Refactor the loop and try-catch to a common run() method in AbstractContainerAllocator
> * Change the log type to WARN for the general exception case
> * Refactor some duplicate code in ContainerRequestState
>
>
> Diffs
> -----
>
> samza-yarn/src/main/java/org/apache/samza/job/yarn/AbstractContainerAllocator.java 9ee2daccc3c44202308637207a084def81b49c09
> samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerAllocator.java 7c57a866114aabf76a36d3b7ca4c5810628e0c77
> samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerRequestState.java ab3061eae2cfc2da6681ce2034492b165d0d8b96
> samza-yarn/src/main/java/org/apache/samza/job/yarn/HostAwareContainerAllocator.java ff22dbfe5537ba2a3c55c4e6063680ff4c9e55f4
>
> Diff: https://reviews.apache.org/r/43074/diff/
>
>
> Testing
> -------
>
> Unit tests still pass.
>
>
> Thanks,
>
> Jake Maes
>
>
Re: Review Request 43074: SAMZA-866 Refactor and fix Container
allocation logic.
Posted by Jagadish Venkatraman <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43074/#review118101
-----------------------------------------------------------
Ship it!
Ship It!
- Jagadish Venkatraman
On Feb. 3, 2016, 8:04 p.m., Jake Maes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43074/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2016, 8:04 p.m.)
>
>
> Review request for samza, Navina Ramesh and Yi Pan (Data Infrastructure).
>
>
> Repository: samza
>
>
> Description
> -------
>
> Bug:
> ContainerAllocator and HostAwareContainerAllocator have different try-catch logic in their run() methods. The HostAwareContainerAllocator try-catch is outside the while loop, which would cause it to stop allocating containers in the event of an exception. ContainerAllocator correctly has the try-catch inside the loop
>
> Fix:
> * Refactor the loop and try-catch to a common run() method in AbstractContainerAllocator
> * Change the log type to WARN for the general exception case
> * Refactor some duplicate code in ContainerRequestState
>
>
> Diffs
> -----
>
> samza-yarn/src/main/java/org/apache/samza/job/yarn/AbstractContainerAllocator.java 9ee2daccc3c44202308637207a084def81b49c09
> samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerAllocator.java 7c57a866114aabf76a36d3b7ca4c5810628e0c77
> samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerRequestState.java ab3061eae2cfc2da6681ce2034492b165d0d8b96
> samza-yarn/src/main/java/org/apache/samza/job/yarn/HostAwareContainerAllocator.java ff22dbfe5537ba2a3c55c4e6063680ff4c9e55f4
>
> Diff: https://reviews.apache.org/r/43074/diff/
>
>
> Testing
> -------
>
> Unit tests still pass.
>
>
> Thanks,
>
> Jake Maes
>
>
Re: Review Request 43074: SAMZA-866 Refactor and fix Container
allocation logic.
Posted by Jake Maes <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43074/
-----------------------------------------------------------
(Updated Feb. 3, 2016, 8:04 p.m.)
Review request for samza, Navina Ramesh and Yi Pan (Data Infrastructure).
Repository: samza
Description (updated)
-------
Bug:
ContainerAllocator and HostAwareContainerAllocator have different try-catch logic in their run() methods. The HostAwareContainerAllocator try-catch is outside the while loop, which would cause it to stop allocating containers in the event of an exception. ContainerAllocator correctly has the try-catch inside the loop
Fix:
* Refactor the loop and try-catch to a common run() method in AbstractContainerAllocator
* Change the log type to WARN for the general exception case
* Refactor some duplicate code in ContainerRequestState
Diffs
-----
samza-yarn/src/main/java/org/apache/samza/job/yarn/AbstractContainerAllocator.java 9ee2daccc3c44202308637207a084def81b49c09
samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerAllocator.java 7c57a866114aabf76a36d3b7ca4c5810628e0c77
samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerRequestState.java ab3061eae2cfc2da6681ce2034492b165d0d8b96
samza-yarn/src/main/java/org/apache/samza/job/yarn/HostAwareContainerAllocator.java ff22dbfe5537ba2a3c55c4e6063680ff4c9e55f4
Diff: https://reviews.apache.org/r/43074/diff/
Testing
-------
Unit tests still pass.
Thanks,
Jake Maes
Re: Review Request 43074: SAMZA-866 Refactor and fix Container
allocation logic.
Posted by Jake Maes <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43074/
-----------------------------------------------------------
(Updated Feb. 3, 2016, 8:03 p.m.)
Review request for samza, Navina Ramesh and Yi Pan (Data Infrastructure).
Repository: samza
Description (updated)
-------
Fixed the code comments and formatting
Diffs (updated)
-----
samza-yarn/src/main/java/org/apache/samza/job/yarn/AbstractContainerAllocator.java 9ee2daccc3c44202308637207a084def81b49c09
samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerAllocator.java 7c57a866114aabf76a36d3b7ca4c5810628e0c77
samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerRequestState.java ab3061eae2cfc2da6681ce2034492b165d0d8b96
samza-yarn/src/main/java/org/apache/samza/job/yarn/HostAwareContainerAllocator.java ff22dbfe5537ba2a3c55c4e6063680ff4c9e55f4
Diff: https://reviews.apache.org/r/43074/diff/
Testing
-------
Unit tests still pass.
Thanks,
Jake Maes
Re: Review Request 43074: SAMZA-866 Refactor and fix Container
allocation logic.
Posted by Navina Ramesh <nr...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43074/#review117582
-----------------------------------------------------------
Ship it!
lgtm! Please fix the nits pointed out by Jagadish.
samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerRequestState.java (line 207)
<https://reviews.apache.org/r/43074/#comment178818>
Nicely refactored. Thanks! :)
- Navina Ramesh
On Feb. 1, 2016, 11:35 p.m., Jake Maes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43074/
> -----------------------------------------------------------
>
> (Updated Feb. 1, 2016, 11:35 p.m.)
>
>
> Review request for samza, Navina Ramesh and Yi Pan (Data Infrastructure).
>
>
> Repository: samza
>
>
> Description
> -------
>
> Bug:
> ContainerAllocator and HostAwareContainerAllocator have different try-catch logic in their run() methods. The HostAwareContainerAllocator try-catch is outside the while loop, which would cause it to stop allocating containers in the event of an exception. ContainerAllocator correctly has the try-catch inside the loop
>
> Fix:
> Refactor the loop and try-catch to a common run() method in AbstractContainerAllocator
> Change the log type to WARN for the general exception case
> Refactor some duplicate code in ContainerRequestState
>
>
> Diffs
> -----
>
> samza-yarn/src/main/java/org/apache/samza/job/yarn/AbstractContainerAllocator.java 9ee2daccc3c44202308637207a084def81b49c09
> samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerAllocator.java 7c57a866114aabf76a36d3b7ca4c5810628e0c77
> samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerRequestState.java ab3061eae2cfc2da6681ce2034492b165d0d8b96
> samza-yarn/src/main/java/org/apache/samza/job/yarn/HostAwareContainerAllocator.java ff22dbfe5537ba2a3c55c4e6063680ff4c9e55f4
>
> Diff: https://reviews.apache.org/r/43074/diff/
>
>
> Testing
> -------
>
> Unit tests still pass.
>
>
> Thanks,
>
> Jake Maes
>
>
Re: Review Request 43074: SAMZA-866 Refactor and fix Container
allocation logic.
Posted by Jake Maes <ja...@gmail.com>.
> On Feb. 3, 2016, 2:55 a.m., Jagadish Venkatraman wrote:
> > samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerAllocator.java, line 46
> > <https://reviews.apache.org/r/43074/diff/1/?file=1228848#file1228848line46>
> >
> > I think the javadoc about the run() method may be moved to the parent class. The javadoc for this abstract method can simply talk about allocator implementation - that is either host aware or not.
Thanks for catching that. Fixed it!
> On Feb. 3, 2016, 2:55 a.m., Jagadish Venkatraman wrote:
> > samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerAllocator.java, line 58
> > <https://reviews.apache.org/r/43074/diff/1/?file=1228848#file1228848line58>
> >
> > nit: Did IDE mess up with some formatting here?
Yeah, it was auto-format. It assumed the line was too long. It's a judgment call but I agree it's more readable as one line, so I fixed it.
- Jake
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43074/#review117555
-----------------------------------------------------------
On Feb. 3, 2016, 8:03 p.m., Jake Maes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43074/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2016, 8:03 p.m.)
>
>
> Review request for samza, Navina Ramesh and Yi Pan (Data Infrastructure).
>
>
> Repository: samza
>
>
> Description
> -------
>
> Fixed the code comments and formatting
>
>
> Diffs
> -----
>
> samza-yarn/src/main/java/org/apache/samza/job/yarn/AbstractContainerAllocator.java 9ee2daccc3c44202308637207a084def81b49c09
> samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerAllocator.java 7c57a866114aabf76a36d3b7ca4c5810628e0c77
> samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerRequestState.java ab3061eae2cfc2da6681ce2034492b165d0d8b96
> samza-yarn/src/main/java/org/apache/samza/job/yarn/HostAwareContainerAllocator.java ff22dbfe5537ba2a3c55c4e6063680ff4c9e55f4
>
> Diff: https://reviews.apache.org/r/43074/diff/
>
>
> Testing
> -------
>
> Unit tests still pass.
>
>
> Thanks,
>
> Jake Maes
>
>
Re: Review Request 43074: SAMZA-866 Refactor and fix Container
allocation logic.
Posted by Jagadish Venkatraman <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43074/#review117555
-----------------------------------------------------------
samza-yarn/src/main/java/org/apache/samza/job/yarn/AbstractContainerAllocator.java (line 94)
<https://reviews.apache.org/r/43074/#comment178787>
Great job on the refactoring. It's more readable when the overriden method is not run() but a more concrete method name - assignContainerRequests!
Also, it's cleaner to handle exceptions in the parent.
samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerAllocator.java (line 45)
<https://reviews.apache.org/r/43074/#comment178786>
I think the javadoc about the run() method may be moved to the parent class. The javadoc for this abstract method can simply talk about allocator implementation - that is either host aware or not.
samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerAllocator.java (line 55)
<https://reviews.apache.org/r/43074/#comment178788>
nit: Did IDE mess up with some formatting here?
- Jagadish Venkatraman
On Feb. 1, 2016, 11:35 p.m., Jake Maes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43074/
> -----------------------------------------------------------
>
> (Updated Feb. 1, 2016, 11:35 p.m.)
>
>
> Review request for samza, Navina Ramesh and Yi Pan (Data Infrastructure).
>
>
> Repository: samza
>
>
> Description
> -------
>
> Bug:
> ContainerAllocator and HostAwareContainerAllocator have different try-catch logic in their run() methods. The HostAwareContainerAllocator try-catch is outside the while loop, which would cause it to stop allocating containers in the event of an exception. ContainerAllocator correctly has the try-catch inside the loop
>
> Fix:
> Refactor the loop and try-catch to a common run() method in AbstractContainerAllocator
> Change the log type to WARN for the general exception case
> Refactor some duplicate code in ContainerRequestState
>
>
> Diffs
> -----
>
> samza-yarn/src/main/java/org/apache/samza/job/yarn/AbstractContainerAllocator.java 9ee2daccc3c44202308637207a084def81b49c09
> samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerAllocator.java 7c57a866114aabf76a36d3b7ca4c5810628e0c77
> samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerRequestState.java ab3061eae2cfc2da6681ce2034492b165d0d8b96
> samza-yarn/src/main/java/org/apache/samza/job/yarn/HostAwareContainerAllocator.java ff22dbfe5537ba2a3c55c4e6063680ff4c9e55f4
>
> Diff: https://reviews.apache.org/r/43074/diff/
>
>
> Testing
> -------
>
> Unit tests still pass.
>
>
> Thanks,
>
> Jake Maes
>
>
Re: Review Request 43074: SAMZA-866 Refactor and fix Container
allocation logic.
Posted by Jake Maes <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43074/
-----------------------------------------------------------
(Updated Feb. 1, 2016, 11:35 p.m.)
Review request for samza, Navina Ramesh and Yi Pan (Data Infrastructure).
Repository: samza
Description
-------
Bug:
ContainerAllocator and HostAwareContainerAllocator have different try-catch logic in their run() methods. The HostAwareContainerAllocator try-catch is outside the while loop, which would cause it to stop allocating containers in the event of an exception. ContainerAllocator correctly has the try-catch inside the loop
Fix:
Refactor the loop and try-catch to a common run() method in AbstractContainerAllocator
Change the log type to WARN for the general exception case
Refactor some duplicate code in ContainerRequestState
Diffs
-----
samza-yarn/src/main/java/org/apache/samza/job/yarn/AbstractContainerAllocator.java 9ee2daccc3c44202308637207a084def81b49c09
samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerAllocator.java 7c57a866114aabf76a36d3b7ca4c5810628e0c77
samza-yarn/src/main/java/org/apache/samza/job/yarn/ContainerRequestState.java ab3061eae2cfc2da6681ce2034492b165d0d8b96
samza-yarn/src/main/java/org/apache/samza/job/yarn/HostAwareContainerAllocator.java ff22dbfe5537ba2a3c55c4e6063680ff4c9e55f4
Diff: https://reviews.apache.org/r/43074/diff/
Testing (updated)
-------
Unit tests still pass.
Thanks,
Jake Maes