You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Santhosh Kumar Shanmugham <sa...@gmail.com> on 2018/06/06 19:44:46 UTC
Review Request 67479: Remove maintenance request after a host is
drained.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67479/
-----------------------------------------------------------
Review request for Aurora, David McLaughlin, Jordan Ly, Renan DelValle, and Stephan Erb.
Repository: aurora
Description
-------
Delete the `HostMaintenaceRequest` once the host has been
`DRAINED`.
Diffs
-----
src/main/java/org/apache/aurora/scheduler/maintenance/MaintenanceController.java dd2462d98a04c9ab6fdd79ccdb25cd309278267e
src/test/java/org/apache/aurora/scheduler/maintenance/MaintenanceControllerImplTest.java 28c62a17db33b16d084b59cf40ca299f322d05e7
Diff: https://reviews.apache.org/r/67479/diff/1/
Testing
-------
./build-support/jenkins/build.sh
./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
Thanks,
Santhosh Kumar Shanmugham
Re: Review Request 67479: Remove maintenance request after a host is
drained.
Posted by Renan DelValle <re...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67479/#review204430
-----------------------------------------------------------
Ship it!
Ship It!
- Renan DelValle
On June 6, 2018, 12:44 p.m., Santhosh Kumar Shanmugham wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67479/
> -----------------------------------------------------------
>
> (Updated June 6, 2018, 12:44 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Jordan Ly, Renan DelValle, and Stephan Erb.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Delete the `HostMaintenaceRequest` once the host has been
> `DRAINED`.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/maintenance/MaintenanceController.java dd2462d98a04c9ab6fdd79ccdb25cd309278267e
> src/test/java/org/apache/aurora/scheduler/maintenance/MaintenanceControllerImplTest.java 28c62a17db33b16d084b59cf40ca299f322d05e7
>
>
> Diff: https://reviews.apache.org/r/67479/diff/1/
>
>
> Testing
> -------
>
> ./build-support/jenkins/build.sh
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
>
>
> Thanks,
>
> Santhosh Kumar Shanmugham
>
>
Re: Review Request 67479: Remove maintenance request after a host is
drained.
Posted by Santhosh Kumar Shanmugham <sa...@gmail.com>.
> On June 6, 2018, 1:50 p.m., Renan DelValle wrote:
> > src/main/java/org/apache/aurora/scheduler/maintenance/MaintenanceController.java
> > Lines 259 (patched)
> > <https://reviews.apache.org/r/67479/diff/1/?file=2036108#file2036108line259>
> >
> > Quick question, the existing behavior is to keep hosts in the DRAINED status until the scheduler receives an end maintenance call. Will this modify the current behavior?
The host will continue to remain in `DRAINED` mode, blocking any new tasks from getting scheduled on it. This is true even when the host is removed and re-registers with a new slave id.
We are only removing the maintenance request here. Since the work for draining the tasks is already done and there is nothing more to be done here. We need to do this otherwise the HostMaintenanceStore can keep growing, unless end maintenance is called for each host. This may not be ideal for cases where hosts are being returned and are not expected to re-enter the cluster.
- Santhosh Kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67479/#review204427
-----------------------------------------------------------
On June 6, 2018, 12:44 p.m., Santhosh Kumar Shanmugham wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67479/
> -----------------------------------------------------------
>
> (Updated June 6, 2018, 12:44 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Jordan Ly, Renan DelValle, and Stephan Erb.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Delete the `HostMaintenaceRequest` once the host has been
> `DRAINED`.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/maintenance/MaintenanceController.java dd2462d98a04c9ab6fdd79ccdb25cd309278267e
> src/test/java/org/apache/aurora/scheduler/maintenance/MaintenanceControllerImplTest.java 28c62a17db33b16d084b59cf40ca299f322d05e7
>
>
> Diff: https://reviews.apache.org/r/67479/diff/1/
>
>
> Testing
> -------
>
> ./build-support/jenkins/build.sh
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
>
>
> Thanks,
>
> Santhosh Kumar Shanmugham
>
>
Re: Review Request 67479: Remove maintenance request after a host is
drained.
Posted by Renan DelValle <re...@apache.org>.
> On June 6, 2018, 1:50 p.m., Renan DelValle wrote:
> > src/main/java/org/apache/aurora/scheduler/maintenance/MaintenanceController.java
> > Lines 259 (patched)
> > <https://reviews.apache.org/r/67479/diff/1/?file=2036108#file2036108line259>
> >
> > Quick question, the existing behavior is to keep hosts in the DRAINED status until the scheduler receives an end maintenance call. Will this modify the current behavior?
>
> Santhosh Kumar Shanmugham wrote:
> The host will continue to remain in `DRAINED` mode, blocking any new tasks from getting scheduled on it. This is true even when the host is removed and re-registers with a new slave id.
>
> We are only removing the maintenance request here. Since the work for draining the tasks is already done and there is nothing more to be done here. We need to do this otherwise the HostMaintenanceStore can keep growing, unless end maintenance is called for each host. This may not be ideal for cases where hosts are being returned and are not expected to re-enter the cluster.
Awesome, makes sense to me. I don't think it'll make a difference since nothing should be a ble to get scheduled on hosts in this state. Patch looks good to me!
- Renan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67479/#review204427
-----------------------------------------------------------
On June 6, 2018, 12:44 p.m., Santhosh Kumar Shanmugham wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67479/
> -----------------------------------------------------------
>
> (Updated June 6, 2018, 12:44 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Jordan Ly, Renan DelValle, and Stephan Erb.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Delete the `HostMaintenaceRequest` once the host has been
> `DRAINED`.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/maintenance/MaintenanceController.java dd2462d98a04c9ab6fdd79ccdb25cd309278267e
> src/test/java/org/apache/aurora/scheduler/maintenance/MaintenanceControllerImplTest.java 28c62a17db33b16d084b59cf40ca299f322d05e7
>
>
> Diff: https://reviews.apache.org/r/67479/diff/1/
>
>
> Testing
> -------
>
> ./build-support/jenkins/build.sh
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
>
>
> Thanks,
>
> Santhosh Kumar Shanmugham
>
>
Re: Review Request 67479: Remove maintenance request after a host is
drained.
Posted by Renan DelValle <re...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67479/#review204427
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/maintenance/MaintenanceController.java
Lines 259 (patched)
<https://reviews.apache.org/r/67479/#comment286927>
Quick question, the existing behavior is to keep hosts in the DRAINED status until the scheduler receives an end maintenance call. Will this modify the current behavior?
- Renan DelValle
On June 6, 2018, 12:44 p.m., Santhosh Kumar Shanmugham wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67479/
> -----------------------------------------------------------
>
> (Updated June 6, 2018, 12:44 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Jordan Ly, Renan DelValle, and Stephan Erb.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Delete the `HostMaintenaceRequest` once the host has been
> `DRAINED`.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/maintenance/MaintenanceController.java dd2462d98a04c9ab6fdd79ccdb25cd309278267e
> src/test/java/org/apache/aurora/scheduler/maintenance/MaintenanceControllerImplTest.java 28c62a17db33b16d084b59cf40ca299f322d05e7
>
>
> Diff: https://reviews.apache.org/r/67479/diff/1/
>
>
> Testing
> -------
>
> ./build-support/jenkins/build.sh
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
>
>
> Thanks,
>
> Santhosh Kumar Shanmugham
>
>
Re: Review Request 67479: Remove maintenance request after a host is
drained.
Posted by David McLaughlin <da...@dmclaughlin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67479/#review204435
-----------------------------------------------------------
Ship it!
Ship It!
- David McLaughlin
On June 6, 2018, 7:44 p.m., Santhosh Kumar Shanmugham wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67479/
> -----------------------------------------------------------
>
> (Updated June 6, 2018, 7:44 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Jordan Ly, Renan DelValle, and Stephan Erb.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Delete the `HostMaintenaceRequest` once the host has been
> `DRAINED`.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/maintenance/MaintenanceController.java dd2462d98a04c9ab6fdd79ccdb25cd309278267e
> src/test/java/org/apache/aurora/scheduler/maintenance/MaintenanceControllerImplTest.java 28c62a17db33b16d084b59cf40ca299f322d05e7
>
>
> Diff: https://reviews.apache.org/r/67479/diff/1/
>
>
> Testing
> -------
>
> ./build-support/jenkins/build.sh
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
>
>
> Thanks,
>
> Santhosh Kumar Shanmugham
>
>
Re: Review Request 67479: Remove maintenance request after a host is
drained.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67479/#review204425
-----------------------------------------------------------
Ship it!
Master (f2acf53) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On June 6, 2018, 12:44 p.m., Santhosh Kumar Shanmugham wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67479/
> -----------------------------------------------------------
>
> (Updated June 6, 2018, 12:44 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Jordan Ly, Renan DelValle, and Stephan Erb.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Delete the `HostMaintenaceRequest` once the host has been
> `DRAINED`.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/maintenance/MaintenanceController.java dd2462d98a04c9ab6fdd79ccdb25cd309278267e
> src/test/java/org/apache/aurora/scheduler/maintenance/MaintenanceControllerImplTest.java 28c62a17db33b16d084b59cf40ca299f322d05e7
>
>
> Diff: https://reviews.apache.org/r/67479/diff/1/
>
>
> Testing
> -------
>
> ./build-support/jenkins/build.sh
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
>
>
> Thanks,
>
> Santhosh Kumar Shanmugham
>
>