Re: Review Request 70666: Introduced a function for validating a `FrameworkInfo` update.

[Benjamin Mahler]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70666/#review215375
---


Fix it, then Ship it!




Looks good, but some clarity in the description and some TODOs indicating that 
this does not validate all `user` and `checkpoint` would be great.


src/master/master.cpp
Line 2574 (original), 2574 (patched)


This is not accurate, if the framework attempts to change 'user' or 
'checkpoint', which are immutable, this validation doesn't catch it.

Can you comment on the intention here more clearly? I see 
https://reviews.apache.org/r/70669/ updates it later to be more coherent: so, 
some appropriate TODOs here are needed to indicate where this is going.



src/master/master.cpp
Line 2585 (original), 2585 (patched)


newline and space before the {

Not yours, I realize it's copied from the condition above, but probably the 
following is more readable?

```
if (validationError.isSome()) {
```

i.e. "if there is an error" instead of "if there isn't no error"



src/master/validation.hpp
Lines 115 (patched)


Ditto, a TODO here is needed to indicate that this doesn't actually check 
all immutable fields.



src/master/validation.cpp
Lines 574-582 (patched)


no need for the std:: prefix anymore



src/master/validation.cpp
Lines 576 (patched)


not yours, but 2 space indent here



src/master/validation.cpp
Lines 579 (patched)


Perhaps the following for a bit more clarity:

```
  Option newPrincipal = None();
```



src/master/validation.cpp
Lines 590-594 (patched)


not yours, but just a comment, we usually try to put the opening and 
closing single quotes on the same line if possible, to reduce accidental 
omission of the closing quote (which has happened quite often)



src/master/validation.cpp
Lines 591 (patched)


Not yours, but we can do the following to clean it up a bit:

```
LOG(WARNING)
  << "Framework " << oldInfo.id() << " which had a principal "
  << " '" << oldPrincipal.getOrElse("") << "'"
  << " tried to (re)subscribe with a new principal "
  << " '" << newPrincipal.getOrElse("") << "'";
```



src/master/validation.cpp
Lines 595 (patched)


newline

Also, not yours, but we generally don't add the period at the end.


- Benjamin Mahler


On May 17, 2019, 3:05 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70666/
> ---
> 
> (Updated May 17, 2019, 3:05 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7258
> https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch moves the code validating a new `FrameworkInfo` against
> the current one into a separate function.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp c72b92656dddca63ee89abf676da66bd76f58a6d 
>   src/master/validation.hpp 95638a17052ece6c957aa76e4cead8d7bfe82024 
>   src/master/validation.cpp 9fb0850987ce385d345302cac9721adead7181b8 
> 
> 
> Diff: https://reviews.apache.org/r/70666/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Review Request 70666: Introduced a function for validating a `FrameworkInfo` update.

[Andrei Sekretenko]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70666/
---

Review request for mesos and Benjamin Mahler.


Bugs: MESOS-7258
https://issues.apache.org/jira/browse/MESOS-7258


Repository: mesos


Description
---

This patch moves the code validating a new `FrameworkInfo` against
the current one into a separate function.


Diffs
-

  src/master/master.cpp c72b92656dddca63ee89abf676da66bd76f58a6d 
  src/master/validation.hpp 95638a17052ece6c957aa76e4cead8d7bfe82024 
  src/master/validation.cpp 9fb0850987ce385d345302cac9721adead7181b8 


Diff: https://reviews.apache.org/r/70666/diff/1/


Testing
---


Thanks,

Andrei Sekretenko