Re: Review Request 45863: Updated error messages in weights handler.

[Alexander Rukletsov]

> On April 7, 2016, 11:07 a.m., Joerg Schad wrote:
> > src/master/weights_handler.cpp, line 90
> > 
> >
> > 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.

[Alexander Rukletsov]

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

[Alexander Rukletsov]

> On April 7, 2016, 11:07 a.m., Joerg Schad wrote:
> > src/master/weights_handler.cpp, line 90
> > 
> >
> > 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.

[Joerg Schad]

> On April 7, 2016, 11:07 a.m., Joerg Schad wrote:
> > src/master/weights_handler.cpp, line 90
> > 
> >
> > 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.

[Alexander Rukletsov]

> On April 7, 2016, 11:07 a.m., Joerg Schad wrote:
> > src/master/weights_handler.cpp, line 90
> > 
> >
> > 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.

[Yongqiao Wang]

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


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.

[Alexander Rukletsov]

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

[Mesos ReviewBot]

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

[Joerg Schad]

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


Not yours: Above the request.body is included in the output. Feels 
inconsistent here...



src/master/weights_handler.cpp (line 94)


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)


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

Review Request 45863: Updated error messages in weights handler.

[Alexander Rukletsov]

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