> 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 > >
