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/19 22:30:29 UTC
Review Request 67657: Introduce a `countdown-ms` param in Coordinator
request.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67657/
-----------------------------------------------------------
Review request for Aurora, David McLaughlin, Franck Cuny, and Jordan Ly.
Repository: aurora
Description
-------
With the introduction of `timeoutSecs` for HostMaintenanceRequest
and the `CoordinatorSlaPolicy`, it will be beneficial to expose the
time remaining until forced maintenance to the Coordinator. Send
the time remaining until force task maintenance as an extra query
param to the Coordinator.
Diffs
-----
docs/features/sla-requirements.md 555b174d2324b0b1b596a3da72b0a5a67fcca153
src/main/java/org/apache/aurora/scheduler/maintenance/MaintenanceController.java 626a68263d6118f138cd6012fd49e033b09b75f0
src/main/java/org/apache/aurora/scheduler/sla/SlaManager.java 9c5caf4af8e8c8bad408100af2bc4fe045603340
src/test/java/org/apache/aurora/scheduler/maintenance/MaintenanceControllerImplTest.java c9390df25f7eacbab14a508b1926a05aac8112d6
src/test/java/org/apache/aurora/scheduler/sla/SlaManagerTest.java 759a1bca4814b2cf70a20eac26aaabfcef682332
Diff: https://reviews.apache.org/r/67657/diff/1/
Testing
-------
./gradlew test
**Tested on Vagrant**
***Logs from Coordinator***
Request received for {'countdown-ms': ['94784'], 'task': ['devcluster/vagrant/test/coordinator/1']}
Responded: False
Request received for {'countdown-ms': ['34777'], 'task': ['devcluster/vagrant/test/coordinator/1']}
Responded: False
Thanks,
Santhosh Kumar Shanmugham
Re: Review Request 67657: Introduce a `countdown-ms` param in
Coordinator request.
Posted by David McLaughlin <da...@dmclaughlin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67657/#review205132
-----------------------------------------------------------
Ship it!
Ship It!
- David McLaughlin
On June 20, 2018, 10:53 p.m., Santhosh Kumar Shanmugham wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67657/
> -----------------------------------------------------------
>
> (Updated June 20, 2018, 10:53 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Franck Cuny, and Jordan Ly.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> With the introduction of `timeoutSecs` for HostMaintenanceRequest
> and the `CoordinatorSlaPolicy`, it will be beneficial to expose the
> time remaining until forced maintenance to the Coordinator. Send
> the time remaining until force task maintenance as an extra query
> param to the Coordinator.
>
>
> Diffs
> -----
>
> docs/features/sla-requirements.md 555b174d2324b0b1b596a3da72b0a5a67fcca153
> src/main/java/org/apache/aurora/scheduler/maintenance/MaintenanceController.java 626a68263d6118f138cd6012fd49e033b09b75f0
> src/main/java/org/apache/aurora/scheduler/sla/SlaManager.java 9c5caf4af8e8c8bad408100af2bc4fe045603340
> src/test/java/org/apache/aurora/scheduler/maintenance/MaintenanceControllerImplTest.java c9390df25f7eacbab14a508b1926a05aac8112d6
> src/test/java/org/apache/aurora/scheduler/sla/SlaManagerTest.java 759a1bca4814b2cf70a20eac26aaabfcef682332
>
>
> Diff: https://reviews.apache.org/r/67657/diff/3/
>
>
> Testing
> -------
>
> ./gradlew test
> ./build-support/jenkins/build.sh
>
> **Tested on Vagrant**
>
> ***Logs from Coordinator***
> Request received for {'task': ['devcluster/vagrant/test/coordinator/0']}
> {
> "forceMaintenanceCountdownMs": "604755646",
> "task": "devcluster/vagrant/test/coordinator/0",
> "taskConfig": {
> "assignedTask": {
> "assignedPorts": {},
> "instanceId": 0,
> "slaveHost": "192.168.33.7",
> "slaveId": "f0336813-864b-4c8f-914c-80f8cef3b61d-S0",
> "task": {
> ...<SNIPPED>
> }
> Responded: True
>
>
> Thanks,
>
> Santhosh Kumar Shanmugham
>
>
Re: Review Request 67657: Introduce a `countdown-ms` param in
Coordinator request.
Posted by Santhosh Kumar Shanmugham <sa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67657/#review205133
-----------------------------------------------------------
@ReviewBot retry
- Santhosh Kumar Shanmugham
On June 20, 2018, 3:53 p.m., Santhosh Kumar Shanmugham wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67657/
> -----------------------------------------------------------
>
> (Updated June 20, 2018, 3:53 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Franck Cuny, and Jordan Ly.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> With the introduction of `timeoutSecs` for HostMaintenanceRequest
> and the `CoordinatorSlaPolicy`, it will be beneficial to expose the
> time remaining until forced maintenance to the Coordinator. Send
> the time remaining until force task maintenance as an extra query
> param to the Coordinator.
>
>
> Diffs
> -----
>
> docs/features/sla-requirements.md 555b174d2324b0b1b596a3da72b0a5a67fcca153
> src/main/java/org/apache/aurora/scheduler/maintenance/MaintenanceController.java 626a68263d6118f138cd6012fd49e033b09b75f0
> src/main/java/org/apache/aurora/scheduler/sla/SlaManager.java 9c5caf4af8e8c8bad408100af2bc4fe045603340
> src/test/java/org/apache/aurora/scheduler/maintenance/MaintenanceControllerImplTest.java c9390df25f7eacbab14a508b1926a05aac8112d6
> src/test/java/org/apache/aurora/scheduler/sla/SlaManagerTest.java 759a1bca4814b2cf70a20eac26aaabfcef682332
>
>
> Diff: https://reviews.apache.org/r/67657/diff/3/
>
>
> Testing
> -------
>
> ./gradlew test
> ./build-support/jenkins/build.sh
>
> **Tested on Vagrant**
>
> ***Logs from Coordinator***
> Request received for {'task': ['devcluster/vagrant/test/coordinator/0']}
> {
> "forceMaintenanceCountdownMs": "604755646",
> "task": "devcluster/vagrant/test/coordinator/0",
> "taskConfig": {
> "assignedTask": {
> "assignedPorts": {},
> "instanceId": 0,
> "slaveHost": "192.168.33.7",
> "slaveId": "f0336813-864b-4c8f-914c-80f8cef3b61d-S0",
> "task": {
> ...<SNIPPED>
> }
> Responded: True
>
>
> Thanks,
>
> Santhosh Kumar Shanmugham
>
>
Re: Review Request 67657: Introduce a `countdown-ms` param in
Coordinator request.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67657/#review205136
-----------------------------------------------------------
Ship it!
Master (3140448) 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 20, 2018, 3:53 p.m., Santhosh Kumar Shanmugham wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67657/
> -----------------------------------------------------------
>
> (Updated June 20, 2018, 3:53 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Franck Cuny, and Jordan Ly.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> With the introduction of `timeoutSecs` for HostMaintenanceRequest
> and the `CoordinatorSlaPolicy`, it will be beneficial to expose the
> time remaining until forced maintenance to the Coordinator. Send
> the time remaining until force task maintenance as an extra query
> param to the Coordinator.
>
>
> Diffs
> -----
>
> docs/features/sla-requirements.md 555b174d2324b0b1b596a3da72b0a5a67fcca153
> src/main/java/org/apache/aurora/scheduler/maintenance/MaintenanceController.java 626a68263d6118f138cd6012fd49e033b09b75f0
> src/main/java/org/apache/aurora/scheduler/sla/SlaManager.java 9c5caf4af8e8c8bad408100af2bc4fe045603340
> src/test/java/org/apache/aurora/scheduler/maintenance/MaintenanceControllerImplTest.java c9390df25f7eacbab14a508b1926a05aac8112d6
> src/test/java/org/apache/aurora/scheduler/sla/SlaManagerTest.java 759a1bca4814b2cf70a20eac26aaabfcef682332
>
>
> Diff: https://reviews.apache.org/r/67657/diff/3/
>
>
> Testing
> -------
>
> ./gradlew test
> ./build-support/jenkins/build.sh
>
> **Tested on Vagrant**
>
> ***Logs from Coordinator***
> Request received for {'task': ['devcluster/vagrant/test/coordinator/0']}
> {
> "forceMaintenanceCountdownMs": "604755646",
> "task": "devcluster/vagrant/test/coordinator/0",
> "taskConfig": {
> "assignedTask": {
> "assignedPorts": {},
> "instanceId": 0,
> "slaveHost": "192.168.33.7",
> "slaveId": "f0336813-864b-4c8f-914c-80f8cef3b61d-S0",
> "task": {
> ...<SNIPPED>
> }
> Responded: True
>
>
> Thanks,
>
> Santhosh Kumar Shanmugham
>
>
Re: Review Request 67657: Introduce a `countdown-ms` param in
Coordinator request.
Posted by Santhosh Kumar Shanmugham <sa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67657/
-----------------------------------------------------------
(Updated June 20, 2018, 3:53 p.m.)
Review request for Aurora, David McLaughlin, Franck Cuny, and Jordan Ly.
Changes
-------
style fix
Repository: aurora
Description
-------
With the introduction of `timeoutSecs` for HostMaintenanceRequest
and the `CoordinatorSlaPolicy`, it will be beneficial to expose the
time remaining until forced maintenance to the Coordinator. Send
the time remaining until force task maintenance as an extra query
param to the Coordinator.
Diffs (updated)
-----
docs/features/sla-requirements.md 555b174d2324b0b1b596a3da72b0a5a67fcca153
src/main/java/org/apache/aurora/scheduler/maintenance/MaintenanceController.java 626a68263d6118f138cd6012fd49e033b09b75f0
src/main/java/org/apache/aurora/scheduler/sla/SlaManager.java 9c5caf4af8e8c8bad408100af2bc4fe045603340
src/test/java/org/apache/aurora/scheduler/maintenance/MaintenanceControllerImplTest.java c9390df25f7eacbab14a508b1926a05aac8112d6
src/test/java/org/apache/aurora/scheduler/sla/SlaManagerTest.java 759a1bca4814b2cf70a20eac26aaabfcef682332
Diff: https://reviews.apache.org/r/67657/diff/3/
Changes: https://reviews.apache.org/r/67657/diff/2-3/
Testing (updated)
-------
./gradlew test
./build-support/jenkins/build.sh
**Tested on Vagrant**
***Logs from Coordinator***
Request received for {'task': ['devcluster/vagrant/test/coordinator/0']}
{
"forceMaintenanceCountdownMs": "604755646",
"task": "devcluster/vagrant/test/coordinator/0",
"taskConfig": {
"assignedTask": {
"assignedPorts": {},
"instanceId": 0,
"slaveHost": "192.168.33.7",
"slaveId": "f0336813-864b-4c8f-914c-80f8cef3b61d-S0",
"task": {
...<SNIPPED>
}
Responded: True
Thanks,
Santhosh Kumar Shanmugham
Re: Review Request 67657: Introduce a `countdown-ms` param in
Coordinator request.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67657/#review205126
-----------------------------------------------------------
Master (3140448) is red with this patch.
./build-support/jenkins/build.sh
[189] ./src/main/resources/source-sans-pro.css 1.05 kB {0} [built]
[221] ./src/main/js/index.js 3.43 kB {0} [built]
+ 264 hidden modules
:processResources
:classes
:jar
:moreStartScripts
:startScripts
:distTar
:distZip
:assemble
:compileTestJavaNote: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
:processTestResources
:testClasses
:compileJmhJavaNote: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
:processJmhResources NO-SOURCE
:jmhClasses
:checkstyleJmh
:checkstyleMain[ant:checkstyle] [ERROR] /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/sla/SlaManager.java:36:8: Unused import - com.google.gson.GsonBuilder. [UnusedImports]
FAILED
FAILURE: Build failed with an exception.
* What went wrong:
Execution failed for task ':checkstyleMain'.
> Checkstyle rule violations were found. See the report at: file:///home/jenkins/jenkins-slave/workspace/AuroraBot/dist/reports/checkstyle/main.html
* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output.
* Get more help at https://help.gradle.org
BUILD FAILED in 5m 47s
32 actionable tasks: 26 executed, 6 up-to-date
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On June 20, 2018, 10:14 p.m., Santhosh Kumar Shanmugham wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67657/
> -----------------------------------------------------------
>
> (Updated June 20, 2018, 10:14 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Franck Cuny, and Jordan Ly.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> With the introduction of `timeoutSecs` for HostMaintenanceRequest
> and the `CoordinatorSlaPolicy`, it will be beneficial to expose the
> time remaining until forced maintenance to the Coordinator. Send
> the time remaining until force task maintenance as an extra query
> param to the Coordinator.
>
>
> Diffs
> -----
>
> docs/features/sla-requirements.md 555b174d2324b0b1b596a3da72b0a5a67fcca153
> src/main/java/org/apache/aurora/scheduler/maintenance/MaintenanceController.java 626a68263d6118f138cd6012fd49e033b09b75f0
> src/main/java/org/apache/aurora/scheduler/sla/SlaManager.java 9c5caf4af8e8c8bad408100af2bc4fe045603340
> src/test/java/org/apache/aurora/scheduler/maintenance/MaintenanceControllerImplTest.java c9390df25f7eacbab14a508b1926a05aac8112d6
> src/test/java/org/apache/aurora/scheduler/sla/SlaManagerTest.java 759a1bca4814b2cf70a20eac26aaabfcef682332
>
>
> Diff: https://reviews.apache.org/r/67657/diff/2/
>
>
> Testing
> -------
>
> ./gradlew test
>
> **Tested on Vagrant**
>
> ***Logs from Coordinator***
> Request received for {'task': ['devcluster/vagrant/test/coordinator/0']}
> {
> "forceMaintenanceCountdownMs": "604755646",
> "task": "devcluster/vagrant/test/coordinator/0",
> "taskConfig": {
> "assignedTask": {
> "assignedPorts": {},
> "instanceId": 0,
> "slaveHost": "192.168.33.7",
> "slaveId": "f0336813-864b-4c8f-914c-80f8cef3b61d-S0",
> "task": {
> ...<SNIPPED>
> }
> Responded: True
>
>
> Thanks,
>
> Santhosh Kumar Shanmugham
>
>
Re: Review Request 67657: Introduce a `countdown-ms` param in
Coordinator request.
Posted by Santhosh Kumar Shanmugham <sa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67657/
-----------------------------------------------------------
(Updated June 20, 2018, 3:14 p.m.)
Review request for Aurora, David McLaughlin, Franck Cuny, and Jordan Ly.
Changes
-------
Moved the params to the request body.
Repository: aurora
Description
-------
With the introduction of `timeoutSecs` for HostMaintenanceRequest
and the `CoordinatorSlaPolicy`, it will be beneficial to expose the
time remaining until forced maintenance to the Coordinator. Send
the time remaining until force task maintenance as an extra query
param to the Coordinator.
Diffs (updated)
-----
docs/features/sla-requirements.md 555b174d2324b0b1b596a3da72b0a5a67fcca153
src/main/java/org/apache/aurora/scheduler/maintenance/MaintenanceController.java 626a68263d6118f138cd6012fd49e033b09b75f0
src/main/java/org/apache/aurora/scheduler/sla/SlaManager.java 9c5caf4af8e8c8bad408100af2bc4fe045603340
src/test/java/org/apache/aurora/scheduler/maintenance/MaintenanceControllerImplTest.java c9390df25f7eacbab14a508b1926a05aac8112d6
src/test/java/org/apache/aurora/scheduler/sla/SlaManagerTest.java 759a1bca4814b2cf70a20eac26aaabfcef682332
Diff: https://reviews.apache.org/r/67657/diff/2/
Changes: https://reviews.apache.org/r/67657/diff/1-2/
Testing (updated)
-------
./gradlew test
**Tested on Vagrant**
***Logs from Coordinator***
Request received for {'task': ['devcluster/vagrant/test/coordinator/0']}
{
"forceMaintenanceCountdownMs": "604755646",
"task": "devcluster/vagrant/test/coordinator/0",
"taskConfig": {
"assignedTask": {
"assignedPorts": {},
"instanceId": 0,
"slaveHost": "192.168.33.7",
"slaveId": "f0336813-864b-4c8f-914c-80f8cef3b61d-S0",
"task": {
...<SNIPPED>
}
Responded: True
Thanks,
Santhosh Kumar Shanmugham
Re: Review Request 67657: Introduce a `countdown-ms` param in
Coordinator request.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67657/#review205038
-----------------------------------------------------------
Ship it!
Master (3140448) 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 19, 2018, 10:30 p.m., Santhosh Kumar Shanmugham wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67657/
> -----------------------------------------------------------
>
> (Updated June 19, 2018, 10:30 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Franck Cuny, and Jordan Ly.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> With the introduction of `timeoutSecs` for HostMaintenanceRequest
> and the `CoordinatorSlaPolicy`, it will be beneficial to expose the
> time remaining until forced maintenance to the Coordinator. Send
> the time remaining until force task maintenance as an extra query
> param to the Coordinator.
>
>
> Diffs
> -----
>
> docs/features/sla-requirements.md 555b174d2324b0b1b596a3da72b0a5a67fcca153
> src/main/java/org/apache/aurora/scheduler/maintenance/MaintenanceController.java 626a68263d6118f138cd6012fd49e033b09b75f0
> src/main/java/org/apache/aurora/scheduler/sla/SlaManager.java 9c5caf4af8e8c8bad408100af2bc4fe045603340
> src/test/java/org/apache/aurora/scheduler/maintenance/MaintenanceControllerImplTest.java c9390df25f7eacbab14a508b1926a05aac8112d6
> src/test/java/org/apache/aurora/scheduler/sla/SlaManagerTest.java 759a1bca4814b2cf70a20eac26aaabfcef682332
>
>
> Diff: https://reviews.apache.org/r/67657/diff/1/
>
>
> Testing
> -------
>
> ./gradlew test
>
> **Tested on Vagrant**
>
> ***Logs from Coordinator***
> Request received for {'countdown-ms': ['94784'], 'task': ['devcluster/vagrant/test/coordinator/1']}
> Responded: False
> Request received for {'countdown-ms': ['34777'], 'task': ['devcluster/vagrant/test/coordinator/1']}
> Responded: False
>
>
> Thanks,
>
> Santhosh Kumar Shanmugham
>
>
Re: Review Request 67657: Introduce a `countdown-ms` param in
Coordinator request.
Posted by Santhosh Kumar Shanmugham <sa...@gmail.com>.
> On June 20, 2018, 10:24 a.m., Jordan Ly wrote:
> > The code LGTM.
> >
> > I think it is a bit odd to have metadata like countdown-ms inside the query param of a POST request. If we add more metadata, we could soon have very long query strings.
> >
> > Overall, I don't have a strong preference either way. I would probably err towards adding a metadata field to the json body, but no blocking concerns.
I had the same concern. Decided to go this route since I am not fully certain the number of params will grow beyond ~5. What customers would like to know if the `task` - that is going to undergo maintenance, `countdown-ms` - any forced maintenance countdown, `destructive` - if the maintenace will be destructive to state (hybrid usecases) and `message` - some human readable message.
We can do the elaborate Metadata in the JSON body approach if a need arises.
- Santhosh Kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67657/#review205102
-----------------------------------------------------------
On June 19, 2018, 3:30 p.m., Santhosh Kumar Shanmugham wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67657/
> -----------------------------------------------------------
>
> (Updated June 19, 2018, 3:30 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Franck Cuny, and Jordan Ly.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> With the introduction of `timeoutSecs` for HostMaintenanceRequest
> and the `CoordinatorSlaPolicy`, it will be beneficial to expose the
> time remaining until forced maintenance to the Coordinator. Send
> the time remaining until force task maintenance as an extra query
> param to the Coordinator.
>
>
> Diffs
> -----
>
> docs/features/sla-requirements.md 555b174d2324b0b1b596a3da72b0a5a67fcca153
> src/main/java/org/apache/aurora/scheduler/maintenance/MaintenanceController.java 626a68263d6118f138cd6012fd49e033b09b75f0
> src/main/java/org/apache/aurora/scheduler/sla/SlaManager.java 9c5caf4af8e8c8bad408100af2bc4fe045603340
> src/test/java/org/apache/aurora/scheduler/maintenance/MaintenanceControllerImplTest.java c9390df25f7eacbab14a508b1926a05aac8112d6
> src/test/java/org/apache/aurora/scheduler/sla/SlaManagerTest.java 759a1bca4814b2cf70a20eac26aaabfcef682332
>
>
> Diff: https://reviews.apache.org/r/67657/diff/1/
>
>
> Testing
> -------
>
> ./gradlew test
>
> **Tested on Vagrant**
>
> ***Logs from Coordinator***
> Request received for {'countdown-ms': ['94784'], 'task': ['devcluster/vagrant/test/coordinator/1']}
> Responded: False
> Request received for {'countdown-ms': ['34777'], 'task': ['devcluster/vagrant/test/coordinator/1']}
> Responded: False
>
>
> Thanks,
>
> Santhosh Kumar Shanmugham
>
>
Re: Review Request 67657: Introduce a `countdown-ms` param in
Coordinator request.
Posted by David McLaughlin <da...@dmclaughlin.com>.
> On June 20, 2018, 5:24 p.m., Jordan Ly wrote:
> > The code LGTM.
> >
> > I think it is a bit odd to have metadata like countdown-ms inside the query param of a POST request. If we add more metadata, we could soon have very long query strings.
> >
> > Overall, I don't have a strong preference either way. I would probably err towards adding a metadata field to the json body, but no blocking concerns.
>
> Santhosh Kumar Shanmugham wrote:
> I had the same concern. Decided to go this route since I am not fully certain the number of params will grow beyond ~5. What customers would like to know if the `task` - that is going to undergo maintenance, `countdown-ms` - any forced maintenance countdown, `destructive` - if the maintenace will be destructive to state (hybrid usecases) and `message` - some human readable message.
>
> We can do the elaborate Metadata in the JSON body approach if a need arises.
"if need arises" is a dangerous approach to take with APIs. It will be almost impossible to change this once we roll this out. I think if we have doubts now, we should go for the approach that is most future-proof.
- David
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67657/#review205102
-----------------------------------------------------------
On June 19, 2018, 10:30 p.m., Santhosh Kumar Shanmugham wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67657/
> -----------------------------------------------------------
>
> (Updated June 19, 2018, 10:30 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Franck Cuny, and Jordan Ly.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> With the introduction of `timeoutSecs` for HostMaintenanceRequest
> and the `CoordinatorSlaPolicy`, it will be beneficial to expose the
> time remaining until forced maintenance to the Coordinator. Send
> the time remaining until force task maintenance as an extra query
> param to the Coordinator.
>
>
> Diffs
> -----
>
> docs/features/sla-requirements.md 555b174d2324b0b1b596a3da72b0a5a67fcca153
> src/main/java/org/apache/aurora/scheduler/maintenance/MaintenanceController.java 626a68263d6118f138cd6012fd49e033b09b75f0
> src/main/java/org/apache/aurora/scheduler/sla/SlaManager.java 9c5caf4af8e8c8bad408100af2bc4fe045603340
> src/test/java/org/apache/aurora/scheduler/maintenance/MaintenanceControllerImplTest.java c9390df25f7eacbab14a508b1926a05aac8112d6
> src/test/java/org/apache/aurora/scheduler/sla/SlaManagerTest.java 759a1bca4814b2cf70a20eac26aaabfcef682332
>
>
> Diff: https://reviews.apache.org/r/67657/diff/1/
>
>
> Testing
> -------
>
> ./gradlew test
>
> **Tested on Vagrant**
>
> ***Logs from Coordinator***
> Request received for {'countdown-ms': ['94784'], 'task': ['devcluster/vagrant/test/coordinator/1']}
> Responded: False
> Request received for {'countdown-ms': ['34777'], 'task': ['devcluster/vagrant/test/coordinator/1']}
> Responded: False
>
>
> Thanks,
>
> Santhosh Kumar Shanmugham
>
>
Re: Review Request 67657: Introduce a `countdown-ms` param in
Coordinator request.
Posted by Jordan Ly <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67657/#review205102
-----------------------------------------------------------
Ship it!
The code LGTM.
I think it is a bit odd to have metadata like countdown-ms inside the query param of a POST request. If we add more metadata, we could soon have very long query strings.
Overall, I don't have a strong preference either way. I would probably err towards adding a metadata field to the json body, but no blocking concerns.
- Jordan Ly
On June 19, 2018, 10:30 p.m., Santhosh Kumar Shanmugham wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67657/
> -----------------------------------------------------------
>
> (Updated June 19, 2018, 10:30 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Franck Cuny, and Jordan Ly.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> With the introduction of `timeoutSecs` for HostMaintenanceRequest
> and the `CoordinatorSlaPolicy`, it will be beneficial to expose the
> time remaining until forced maintenance to the Coordinator. Send
> the time remaining until force task maintenance as an extra query
> param to the Coordinator.
>
>
> Diffs
> -----
>
> docs/features/sla-requirements.md 555b174d2324b0b1b596a3da72b0a5a67fcca153
> src/main/java/org/apache/aurora/scheduler/maintenance/MaintenanceController.java 626a68263d6118f138cd6012fd49e033b09b75f0
> src/main/java/org/apache/aurora/scheduler/sla/SlaManager.java 9c5caf4af8e8c8bad408100af2bc4fe045603340
> src/test/java/org/apache/aurora/scheduler/maintenance/MaintenanceControllerImplTest.java c9390df25f7eacbab14a508b1926a05aac8112d6
> src/test/java/org/apache/aurora/scheduler/sla/SlaManagerTest.java 759a1bca4814b2cf70a20eac26aaabfcef682332
>
>
> Diff: https://reviews.apache.org/r/67657/diff/1/
>
>
> Testing
> -------
>
> ./gradlew test
>
> **Tested on Vagrant**
>
> ***Logs from Coordinator***
> Request received for {'countdown-ms': ['94784'], 'task': ['devcluster/vagrant/test/coordinator/1']}
> Responded: False
> Request received for {'countdown-ms': ['34777'], 'task': ['devcluster/vagrant/test/coordinator/1']}
> Responded: False
>
>
> Thanks,
>
> Santhosh Kumar Shanmugham
>
>