You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alexander Rukletsov <ru...@gmail.com> on 2016/04/07 12:50:29 UTC
Review Request 45863: Updated error messages in weights handler.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45863/
-----------------------------------------------------------
Review request for mesos, Yongqiao Wang and Joerg Schad.
Repository: mesos
Description
-------
While writing log or error messages we adhere the following style:
* No period at the end (there are exceptions though, e.g., one is
when constructing request responses.
* When splitting a string over multiple lines, put a space at the
beginning of the following line in contrast to the end of the
previous line.
Diffs
-----
src/master/weights_handler.cpp e88bf2ab67ccadf35879b92f3280298a43d7cd0e
Diff: https://reviews.apache.org/r/45863/diff/
Testing
-------
On Mac OS 10.10.4:
`make check`
Thanks,
Alexander Rukletsov
Re: Review Request 45863: Updated error messages in weights handler.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45863/#review127581
-----------------------------------------------------------
Bad patch!
Reviews applied: [45863]
Failed command: ./support/apply-review.sh -n -r 45863
Error:
2016-04-07 11:28:15 URL:https://reviews.apache.org/r/45863/diff/raw/ [2452/2452] -> "45863.patch" [1]
Total errors found: 0
Checking 1 files
Error: No line in the commit message summary may exceed 72 characters.
Full log: https://builds.apache.org/job/mesos-reviewbot/12378/console
- Mesos ReviewBot
On April 7, 2016, 10:50 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45863/
> -----------------------------------------------------------
>
> (Updated April 7, 2016, 10:50 a.m.)
>
>
> Review request for mesos, Yongqiao Wang and Joerg Schad.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> While writing log or error messages we adhere the following style:
> * No period at the end (there are exceptions though, e.g., one is
> when constructing request responses.
> * When splitting a string over multiple lines, put a space at the
> beginning of the following line in contrast to the end of the
> previous line.
>
>
> Diffs
> -----
>
> src/master/weights_handler.cpp e88bf2ab67ccadf35879b92f3280298a43d7cd0e
>
> Diff: https://reviews.apache.org/r/45863/diff/
>
>
> Testing
> -------
>
> On Mac OS 10.10.4:
> `make check`
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 45863: Updated error messages in weights handler.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45863/#review127755
-----------------------------------------------------------
@ReviewBot retry
- Alexander Rukletsov
On April 7, 2016, 10:50 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45863/
> -----------------------------------------------------------
>
> (Updated April 7, 2016, 10:50 a.m.)
>
>
> Review request for mesos, Yongqiao Wang and Joerg Schad.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> While writing log or error messages we adhere the following style:
> * No period at the end (there are exceptions though, e.g., one is
> when constructing request responses.
> * When splitting a string over multiple lines, put a space at the
> beginning of the following line in contrast to the end of the
> previous line.
>
>
> Diffs
> -----
>
> src/master/weights_handler.cpp e88bf2ab67ccadf35879b92f3280298a43d7cd0e
>
> Diff: https://reviews.apache.org/r/45863/diff/
>
>
> Testing
> -------
>
> On Mac OS 10.10.4:
> `make check`
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 45863: Updated error messages in weights handler.
Posted by Yongqiao Wang <yq...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45863/#review128057
-----------------------------------------------------------
Fix it, then Ship it!
src/master/weights_handler.cpp (line 104)
<https://reviews.apache.org/r/45863/#comment191446>
Mesos operator can update weight for mutilple roles at one time, so it is better to show which role's weight is not positive to make this error message more clearly.
- Yongqiao Wang
On April 7, 2016, 10:50 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45863/
> -----------------------------------------------------------
>
> (Updated April 7, 2016, 10:50 a.m.)
>
>
> Review request for mesos, Yongqiao Wang and Joerg Schad.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> While writing log or error messages we adhere the following style:
> * No period at the end (there are exceptions though, e.g., one is
> when constructing request responses.
> * When splitting a string over multiple lines, put a space at the
> beginning of the following line in contrast to the end of the
> previous line.
>
>
> Diffs
> -----
>
> src/master/weights_handler.cpp e88bf2ab67ccadf35879b92f3280298a43d7cd0e
>
> Diff: https://reviews.apache.org/r/45863/diff/
>
>
> Testing
> -------
>
> On Mac OS 10.10.4:
> `make check`
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 45863: Updated error messages in weights handler.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45863/
-----------------------------------------------------------
(Updated April 11, 2016, 2:48 p.m.)
Review request for mesos, Yongqiao Wang and Joerg Schad.
Repository: mesos
Description
-------
While writing log or error messages we adhere the following style:
* No period at the end (there are exceptions though, e.g., one is
when constructing request responses.
* When splitting a string over multiple lines, put a space at the
beginning of the following line in contrast to the end of the
previous line.
Diffs (updated)
-----
src/master/weights_handler.cpp 10a1fbc5656f98bc893cf63f2653565ae7ebd125
Diff: https://reviews.apache.org/r/45863/diff/
Testing
-------
On Mac OS 10.10.4:
`make check`
Thanks,
Alexander Rukletsov
Re: Review Request 45863: Updated error messages in weights handler.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On April 7, 2016, 11:07 a.m., Joerg Schad wrote:
> > src/master/weights_handler.cpp, line 90
> > <https://reviews.apache.org/r/45863/diff/1/?file=1329646#file1329646line90>
> >
> > Not yours: Above the request.body is included in the output. Feels inconsistent here...
>
> Alexander Rukletsov wrote:
> Above are errors for the whole request, here they are per role basis.
>
> Joerg Schad wrote:
> Ok, (unrelated to this) would you then say we should to remove in for quota_handler as well?
> ```return BadRequest(
> "Failed to validate set quota request JSON '" + request.body +
> "': Unknown role '" + quotaInfo.role() + "'");
> }```
>
> Alexander Rukletsov wrote:
> Why should we?
Filed https://reviews.apache.org/r/46033/
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45863/#review127580
-----------------------------------------------------------
On April 7, 2016, 10:50 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45863/
> -----------------------------------------------------------
>
> (Updated April 7, 2016, 10:50 a.m.)
>
>
> Review request for mesos, Yongqiao Wang and Joerg Schad.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> While writing log or error messages we adhere the following style:
> * No period at the end (there are exceptions though, e.g., one is
> when constructing request responses.
> * When splitting a string over multiple lines, put a space at the
> beginning of the following line in contrast to the end of the
> previous line.
>
>
> Diffs
> -----
>
> src/master/weights_handler.cpp e88bf2ab67ccadf35879b92f3280298a43d7cd0e
>
> Diff: https://reviews.apache.org/r/45863/diff/
>
>
> Testing
> -------
>
> On Mac OS 10.10.4:
> `make check`
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 45863: Updated error messages in weights handler.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On April 7, 2016, 11:07 a.m., Joerg Schad wrote:
> > src/master/weights_handler.cpp, line 90
> > <https://reviews.apache.org/r/45863/diff/1/?file=1329646#file1329646line90>
> >
> > Not yours: Above the request.body is included in the output. Feels inconsistent here...
>
> Alexander Rukletsov wrote:
> Above are errors for the whole request, here they are per role basis.
>
> Joerg Schad wrote:
> Ok, (unrelated to this) would you then say we should to remove in for quota_handler as well?
> ```return BadRequest(
> "Failed to validate set quota request JSON '" + request.body +
> "': Unknown role '" + quotaInfo.role() + "'");
> }```
Why should we?
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45863/#review127580
-----------------------------------------------------------
On April 7, 2016, 10:50 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45863/
> -----------------------------------------------------------
>
> (Updated April 7, 2016, 10:50 a.m.)
>
>
> Review request for mesos, Yongqiao Wang and Joerg Schad.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> While writing log or error messages we adhere the following style:
> * No period at the end (there are exceptions though, e.g., one is
> when constructing request responses.
> * When splitting a string over multiple lines, put a space at the
> beginning of the following line in contrast to the end of the
> previous line.
>
>
> Diffs
> -----
>
> src/master/weights_handler.cpp e88bf2ab67ccadf35879b92f3280298a43d7cd0e
>
> Diff: https://reviews.apache.org/r/45863/diff/
>
>
> Testing
> -------
>
> On Mac OS 10.10.4:
> `make check`
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 45863: Updated error messages in weights handler.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On April 7, 2016, 11:07 a.m., Joerg Schad wrote:
> > src/master/weights_handler.cpp, line 90
> > <https://reviews.apache.org/r/45863/diff/1/?file=1329646#file1329646line90>
> >
> > Not yours: Above the request.body is included in the output. Feels inconsistent here...
Above are errors for the whole request, here they are per role basis.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45863/#review127580
-----------------------------------------------------------
On April 7, 2016, 10:50 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45863/
> -----------------------------------------------------------
>
> (Updated April 7, 2016, 10:50 a.m.)
>
>
> Review request for mesos, Yongqiao Wang and Joerg Schad.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> While writing log or error messages we adhere the following style:
> * No period at the end (there are exceptions though, e.g., one is
> when constructing request responses.
> * When splitting a string over multiple lines, put a space at the
> beginning of the following line in contrast to the end of the
> previous line.
>
>
> Diffs
> -----
>
> src/master/weights_handler.cpp e88bf2ab67ccadf35879b92f3280298a43d7cd0e
>
> Diff: https://reviews.apache.org/r/45863/diff/
>
>
> Testing
> -------
>
> On Mac OS 10.10.4:
> `make check`
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 45863: Updated error messages in weights handler.
Posted by Joerg Schad <jo...@mesosphere.io>.
> On April 7, 2016, 11:07 a.m., Joerg Schad wrote:
> > src/master/weights_handler.cpp, line 90
> > <https://reviews.apache.org/r/45863/diff/1/?file=1329646#file1329646line90>
> >
> > Not yours: Above the request.body is included in the output. Feels inconsistent here...
>
> Alexander Rukletsov wrote:
> Above are errors for the whole request, here they are per role basis.
Ok, (unrelated to this) would you then say we should to remove in for quota_handler as well?
```return BadRequest(
"Failed to validate set quota request JSON '" + request.body +
"': Unknown role '" + quotaInfo.role() + "'");
}```
- Joerg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45863/#review127580
-----------------------------------------------------------
On April 7, 2016, 10:50 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45863/
> -----------------------------------------------------------
>
> (Updated April 7, 2016, 10:50 a.m.)
>
>
> Review request for mesos, Yongqiao Wang and Joerg Schad.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> While writing log or error messages we adhere the following style:
> * No period at the end (there are exceptions though, e.g., one is
> when constructing request responses.
> * When splitting a string over multiple lines, put a space at the
> beginning of the following line in contrast to the end of the
> previous line.
>
>
> Diffs
> -----
>
> src/master/weights_handler.cpp e88bf2ab67ccadf35879b92f3280298a43d7cd0e
>
> Diff: https://reviews.apache.org/r/45863/diff/
>
>
> Testing
> -------
>
> On Mac OS 10.10.4:
> `make check`
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 45863: Updated error messages in weights handler.
Posted by Joerg Schad <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45863/#review127580
-----------------------------------------------------------
Fix it, then Ship it!
src/master/weights_handler.cpp (line 90)
<https://reviews.apache.org/r/45863/#comment190984>
Not yours: Above the request.body is included in the output. Feels inconsistent here...
src/master/weights_handler.cpp (line 94)
<https://reviews.apache.org/r/45863/#comment190982>
Could you add a similar comment as in quota_handler (// Check that the role is on the role whitelist, if it exists.). IMO for isWhitelistedRole it is not obvious from the name that it will be true if there is no whitelist.
src/master/weights_handler.cpp (line 97)
<https://reviews.apache.org/r/45863/#comment190983>
Not yours, still: This feels a little off as it must only exist in the whitelist if there is a whitelist specified.
- Joerg Schad
On April 7, 2016, 10:50 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45863/
> -----------------------------------------------------------
>
> (Updated April 7, 2016, 10:50 a.m.)
>
>
> Review request for mesos, Yongqiao Wang and Joerg Schad.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> While writing log or error messages we adhere the following style:
> * No period at the end (there are exceptions though, e.g., one is
> when constructing request responses.
> * When splitting a string over multiple lines, put a space at the
> beginning of the following line in contrast to the end of the
> previous line.
>
>
> Diffs
> -----
>
> src/master/weights_handler.cpp e88bf2ab67ccadf35879b92f3280298a43d7cd0e
>
> Diff: https://reviews.apache.org/r/45863/diff/
>
>
> Testing
> -------
>
> On Mac OS 10.10.4:
> `make check`
>
>
> Thanks,
>
> Alexander Rukletsov
>
>