You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Michael Park <mc...@gmail.com> on 2015/06/21 18:16:04 UTC
Review Request 35714: Added a new HTTP response type:
PreconditionFailed.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35714/
-----------------------------------------------------------
Review request for mesos, Adam B and Jie Yu.
Repository: mesos
Description
-------
Needed in subsequent patch for /reserve master endpoint.
Diffs
-----
3rdparty/libprocess/include/process/http.hpp e47cc7afbc8110759edf25a2dc05d09eda25c417
Diff: https://reviews.apache.org/r/35714/diff/
Testing
-------
`make check`
Thanks,
Michael Park
Re: Review Request 35714: Added a new HTTP response type:
PreconditionFailed.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35714/#review88759
-----------------------------------------------------------
Ship it!
Ship It!
- Alexander Rukletsov
On June 22, 2015, 5:08 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35714/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 5:08 a.m.)
>
>
> Review request for mesos, Adam B and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Needed in subsequent patch for /reserve master endpoint.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp e47cc7afbc8110759edf25a2dc05d09eda25c417
>
> Diff: https://reviews.apache.org/r/35714/diff/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 35714: Added a new HTTP response type:
PreconditionFailed.
Posted by Michael Park <mc...@gmail.com>.
> On June 22, 2015, 8:04 p.m., Ben Mahler wrote:
> > Adding this seems good since it's agnostic to how it is used. But for reservations specifically, any reason not to use just BadRequest?
> >
> > From what I can tell, this is generally used for conditional requests (based on headers or some other conditionals): http://stackoverflow.com/a/5369582
> > What will the pre-conditions be for /reserve? If the request does not contain explicit pre-conditions, it seems a little non-idiomatic to return 412..?
>
> Michael Park wrote:
> I was hoping to keep `BadRequest` to situations where the user has provided bad/invalid arguments. e.g. invalid JSON format, missing parameters, invalid resources, etc.
>
> I was aiming to use `PreconditionFailed` to represent the case where the user have provided perfectly valid arguments, but we don't currently have sufficient resources to satisfy the request. The precondition here is whether we have sufficient resources or not. I'm of course open to using `BadRequest` with an explanation or what went wrong, or perhaps using a different status. (e.g. `Conflict` was suggested)
>
> I'm not an expert on HTTP statuses and so I don't have a strong stance here, do you have a strong opinion or stance on this? I'm really open to anything.
>
> Alexander Rukletsov wrote:
> I think we should definitely distinguish between invalid JSON format, failed authorization (not in MVP) and inability to satisfy a valid request. For the latter case, I'm not sure `Conflict` is good choice, I would propose `403 Forbidden`.
Thanks Alex! `403 Forbidden` sounds reasonable to me as well.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35714/#review88839
-----------------------------------------------------------
On June 22, 2015, 5:08 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35714/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 5:08 a.m.)
>
>
> Review request for mesos, Adam B and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Needed in subsequent patch for /reserve master endpoint.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp e47cc7afbc8110759edf25a2dc05d09eda25c417
>
> Diff: https://reviews.apache.org/r/35714/diff/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 35714: Added a new HTTP response type:
PreconditionFailed.
Posted by Michael Park <mc...@gmail.com>.
> On June 22, 2015, 8:04 p.m., Ben Mahler wrote:
> > Adding this seems good since it's agnostic to how it is used. But for reservations specifically, any reason not to use just BadRequest?
> >
> > From what I can tell, this is generally used for conditional requests (based on headers or some other conditionals): http://stackoverflow.com/a/5369582
> > What will the pre-conditions be for /reserve? If the request does not contain explicit pre-conditions, it seems a little non-idiomatic to return 412..?
I was hoping to keep `BadRequest` to situations where the user has provided bad/invalid arguments. e.g. invalid JSON format, missing parameters, invalid resources, etc.
I was aiming to use `PreconditionFailed` to represent the case where the user have provided perfectly valid arguments, but we don't currently have sufficient resources to satisfy the request. The precondition here is whether we have sufficient resources or not. I'm of course open to using `BadRequest` with an explanation or what went wrong, or perhaps using a different status. (e.g. `Conflict` was suggested)
I'm not an expert on HTTP statuses and so I don't have a strong stance here, do you have a strong opinion or stance on this? I'm really open to anything.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35714/#review88839
-----------------------------------------------------------
On June 22, 2015, 5:08 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35714/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 5:08 a.m.)
>
>
> Review request for mesos, Adam B and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Needed in subsequent patch for /reserve master endpoint.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp e47cc7afbc8110759edf25a2dc05d09eda25c417
>
> Diff: https://reviews.apache.org/r/35714/diff/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 35714: Added a new HTTP response type:
PreconditionFailed.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On June 22, 2015, 8:04 p.m., Ben Mahler wrote:
> > Adding this seems good since it's agnostic to how it is used. But for reservations specifically, any reason not to use just BadRequest?
> >
> > From what I can tell, this is generally used for conditional requests (based on headers or some other conditionals): http://stackoverflow.com/a/5369582
> > What will the pre-conditions be for /reserve? If the request does not contain explicit pre-conditions, it seems a little non-idiomatic to return 412..?
>
> Michael Park wrote:
> I was hoping to keep `BadRequest` to situations where the user has provided bad/invalid arguments. e.g. invalid JSON format, missing parameters, invalid resources, etc.
>
> I was aiming to use `PreconditionFailed` to represent the case where the user have provided perfectly valid arguments, but we don't currently have sufficient resources to satisfy the request. The precondition here is whether we have sufficient resources or not. I'm of course open to using `BadRequest` with an explanation or what went wrong, or perhaps using a different status. (e.g. `Conflict` was suggested)
>
> I'm not an expert on HTTP statuses and so I don't have a strong stance here, do you have a strong opinion or stance on this? I'm really open to anything.
I think we should definitely distinguish between invalid JSON format, failed authorization (not in MVP) and inability to satisfy a valid request. For the latter case, I'm not sure `Conflict` is good choice, I would propose `403 Forbidden`.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35714/#review88839
-----------------------------------------------------------
On June 22, 2015, 5:08 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35714/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 5:08 a.m.)
>
>
> Review request for mesos, Adam B and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Needed in subsequent patch for /reserve master endpoint.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp e47cc7afbc8110759edf25a2dc05d09eda25c417
>
> Diff: https://reviews.apache.org/r/35714/diff/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 35714: Added a new HTTP response type:
PreconditionFailed.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35714/#review88839
-----------------------------------------------------------
Adding this seems good since it's agnostic to how it is used. But for reservations specifically, any reason not to use just BadRequest?
>From what I can tell, this is generally used for conditional requests (based on headers or some other conditionals): http://stackoverflow.com/a/5369582
What will the pre-conditions be for /reserve? If the request does not contain explicit pre-conditions, it seems a little non-idiomatic to return 412..?
- Ben Mahler
On June 22, 2015, 5:08 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35714/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 5:08 a.m.)
>
>
> Review request for mesos, Adam B and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Needed in subsequent patch for /reserve master endpoint.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp e47cc7afbc8110759edf25a2dc05d09eda25c417
>
> Diff: https://reviews.apache.org/r/35714/diff/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 35714: Added a new HTTP response type:
PreconditionFailed.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35714/#review89339
-----------------------------------------------------------
Ship it!
Ship It!
- Alexander Rojas
On June 22, 2015, 7:08 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35714/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 7:08 a.m.)
>
>
> Review request for mesos, Adam B and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Needed in subsequent patch for /reserve master endpoint.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp e47cc7afbc8110759edf25a2dc05d09eda25c417
>
> Diff: https://reviews.apache.org/r/35714/diff/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 35714: Added a new HTTP response type:
PreconditionFailed.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35714/
-----------------------------------------------------------
(Updated June 22, 2015, 5:08 a.m.)
Review request for mesos, Adam B and Jie Yu.
Changes
-------
Rebased.
Repository: mesos
Description
-------
Needed in subsequent patch for /reserve master endpoint.
Diffs (updated)
-----
3rdparty/libprocess/include/process/http.hpp e47cc7afbc8110759edf25a2dc05d09eda25c417
Diff: https://reviews.apache.org/r/35714/diff/
Testing
-------
`make check`
Thanks,
Michael Park
Re: Review Request 35714: Added a new HTTP response type:
PreconditionFailed.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35714/
-----------------------------------------------------------
(Updated June 22, 2015, 5:06 a.m.)
Review request for mesos, Adam B and Jie Yu.
Changes
-------
Addressed AdamB's comment.
Repository: mesos
Description
-------
Needed in subsequent patch for /reserve master endpoint.
Diffs (updated)
-----
3rdparty/libprocess/include/process/http.hpp e47cc7afbc8110759edf25a2dc05d09eda25c417
src/Makefile.am dfebd2b14c9cb45c437509809fdf5ac3b0c8838c
src/authentication/cram_md5/authenticatee.hpp 9d6293c32722272228652d0241c59c7eeb88d0a3
src/authentication/cram_md5/authenticatee.cpp 63ae17e5ed4702eb48feb2217e8f30556de78f42
src/slave/slave.cpp 40c0c33add392591af4767f76ce566196f24e6ee
src/tests/cram_md5_authentication_tests.cpp 9d15b55f881aaecba32c8ce0afa6fa1ccd236e21
Diff: https://reviews.apache.org/r/35714/diff/
Testing
-------
`make check`
Thanks,
Michael Park
Re: Review Request 35714: Added a new HTTP response type:
PreconditionFailed.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35714/#review88727
-----------------------------------------------------------
Ship it!
Out of order, but otherwise good. Fix, then ShipIt!
3rdparty/libprocess/include/process/http.hpp (line 473)
<https://reviews.apache.org/r/35714/#comment141299>
Let's keep these in numerical order, so put this after 404, before 500, please.
- Adam B
On June 21, 2015, 9:16 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35714/
> -----------------------------------------------------------
>
> (Updated June 21, 2015, 9:16 a.m.)
>
>
> Review request for mesos, Adam B and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Needed in subsequent patch for /reserve master endpoint.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp e47cc7afbc8110759edf25a2dc05d09eda25c417
>
> Diff: https://reviews.apache.org/r/35714/diff/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Michael Park
>
>