You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Bernd Mathiske <be...@mesosphere.io> on 2015/09/04 12:57:15 UTC
Review Request 38125: Included hostname in SlaveID.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38125/
-----------------------------------------------------------
Review request for mesos, Jan Schlicht and Till Toenshoff.
Bugs: MESOS-115
https://issues.apache.org/jira/browse/MESOS-115
Repository: mesos
Description
-------
Now including the hostname in the SlaveID, for human consumption. Also testing this in a unit test.
Diffs
-----
src/master/master.hpp e1331851c19e3372a4a525dcfd7ba2a01c3e97a6
src/master/master.cpp 5589eca4317b597de509f3387cfc349083b361ac
src/tests/master_tests.cpp 8a6b98b9f59ead20f537eb60b5084feed069a5b1
Diff: https://reviews.apache.org/r/38125/diff/
Testing
-------
make check
Thanks,
Bernd Mathiske
Re: Review Request 38125: Included hostname in SlaveID.
Posted by Bernd Mathiske <be...@mesosphere.io>.
> On Sept. 4, 2015, 5:43 a.m., Guangya Liu wrote:
> > src/master/master.cpp, line 5846
> > <https://reviews.apache.org/r/38125/diff/1/?file=1063972#file1063972line5846>
> >
> > I think that we cannot make the assumpation here as the host FQDN name may include the underscore, so how to handle the csae of if slave name is FQDN name? What about using other charactors to connect? Such as # or others?
> > http://stackoverflow.com/questions/10959757/the-use-of-the-underscore-in-host-names
There seems to be no perfect solution. I added a comment that explains it a bit better. Side condition: we must use a char that can also appear in a directory name.
- Bernd
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38125/#review97749
-----------------------------------------------------------
On Sept. 4, 2015, 3:57 a.m., Bernd Mathiske wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38125/
> -----------------------------------------------------------
>
> (Updated Sept. 4, 2015, 3:57 a.m.)
>
>
> Review request for mesos, Jan Schlicht and Till Toenshoff.
>
>
> Bugs: MESOS-115
> https://issues.apache.org/jira/browse/MESOS-115
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Now including the hostname in the SlaveID, for human consumption. Also testing this in a unit test.
>
>
> Diffs
> -----
>
> src/master/master.hpp e1331851c19e3372a4a525dcfd7ba2a01c3e97a6
> src/master/master.cpp 5589eca4317b597de509f3387cfc349083b361ac
> src/tests/master_tests.cpp 8a6b98b9f59ead20f537eb60b5084feed069a5b1
>
> Diff: https://reviews.apache.org/r/38125/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bernd Mathiske
>
>
Re: Review Request 38125: Included hostname in SlaveID.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38125/#review97749
-----------------------------------------------------------
src/master/master.cpp (line 3672)
<https://reviews.apache.org/r/38125/#comment153766>
Since you are also adding port to slaveID, so can you please also update the log message here by adding slaveInfo.port()?
src/master/master.cpp (line 5846)
<https://reviews.apache.org/r/38125/#comment153767>
I think that we cannot make the assumpation here as the host FQDN name may include the underscore, so how to handle the csae of if slave name is FQDN name? What about using other charactors to connect? Such as # or others?
http://stackoverflow.com/questions/10959757/the-use-of-the-underscore-in-host-names
- Guangya Liu
On 九月 4, 2015, 10:57 a.m., Bernd Mathiske wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38125/
> -----------------------------------------------------------
>
> (Updated 九月 4, 2015, 10:57 a.m.)
>
>
> Review request for mesos, Jan Schlicht and Till Toenshoff.
>
>
> Bugs: MESOS-115
> https://issues.apache.org/jira/browse/MESOS-115
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Now including the hostname in the SlaveID, for human consumption. Also testing this in a unit test.
>
>
> Diffs
> -----
>
> src/master/master.hpp e1331851c19e3372a4a525dcfd7ba2a01c3e97a6
> src/master/master.cpp 5589eca4317b597de509f3387cfc349083b361ac
> src/tests/master_tests.cpp 8a6b98b9f59ead20f537eb60b5084feed069a5b1
>
> Diff: https://reviews.apache.org/r/38125/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bernd Mathiske
>
>
Re: Review Request 38125: Included hostname in SlaveID.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38125/#review97744
-----------------------------------------------------------
src/master/master.cpp (line 5847)
<https://reviews.apache.org/r/38125/#comment153760>
Indent with 4 spaces and break after the opening brace to reduce jaggedness.
src/master/master.cpp (line 5849)
<https://reviews.apache.org/r/38125/#comment153763>
s/"_S"/"-S"
src/tests/master_tests.cpp (line 3672)
<https://reviews.apache.org/r/38125/#comment153762>
s/client/slave? Or what is meant with client?
- Jan Schlicht
On Sept. 4, 2015, 12:57 p.m., Bernd Mathiske wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38125/
> -----------------------------------------------------------
>
> (Updated Sept. 4, 2015, 12:57 p.m.)
>
>
> Review request for mesos, Jan Schlicht and Till Toenshoff.
>
>
> Bugs: MESOS-115
> https://issues.apache.org/jira/browse/MESOS-115
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Now including the hostname in the SlaveID, for human consumption. Also testing this in a unit test.
>
>
> Diffs
> -----
>
> src/master/master.hpp e1331851c19e3372a4a525dcfd7ba2a01c3e97a6
> src/master/master.cpp 5589eca4317b597de509f3387cfc349083b361ac
> src/tests/master_tests.cpp 8a6b98b9f59ead20f537eb60b5084feed069a5b1
>
> Diff: https://reviews.apache.org/r/38125/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bernd Mathiske
>
>
Re: Review Request 38125: Included hostname in SlaveID.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38125/#review97756
-----------------------------------------------------------
src/master/master.cpp (line 5846)
<https://reviews.apache.org/r/38125/#comment153773>
IMHO, adding comments here cannot help the one who is reading log messages, can we use some other characters to connnect? Thanks.
- Guangya Liu
On 九月 4, 2015, 10:57 a.m., Bernd Mathiske wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38125/
> -----------------------------------------------------------
>
> (Updated 九月 4, 2015, 10:57 a.m.)
>
>
> Review request for mesos, Jan Schlicht and Till Toenshoff.
>
>
> Bugs: MESOS-115
> https://issues.apache.org/jira/browse/MESOS-115
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Now including the hostname in the SlaveID, for human consumption. Also testing this in a unit test.
>
>
> Diffs
> -----
>
> src/master/master.hpp e1331851c19e3372a4a525dcfd7ba2a01c3e97a6
> src/master/master.cpp 5589eca4317b597de509f3387cfc349083b361ac
> src/tests/master_tests.cpp 8a6b98b9f59ead20f537eb60b5084feed069a5b1
>
> Diff: https://reviews.apache.org/r/38125/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bernd Mathiske
>
>
Re: Review Request 38125: Included hostname in SlaveID.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38125/#review97743
-----------------------------------------------------------
Looks very good to me.
src/master/master.cpp (lines 5847 - 5849)
<https://reviews.apache.org/r/38125/#comment153761>
How about preventing the variable indentation by concentrating on the fact that can be considered a parameter continuation;
```
slaveId.set_value(
info_.id() + "_" + stringify(slaveInfo.hostname()) + "_" +
stringify(slaveInfo.port()) + "_S" + stringify(nextSlaveId++));
```
src/tests/master_tests.cpp (line 43)
<https://reviews.apache.org/r/38125/#comment153757>
Missing include for net::IP
```
#include <stout/ip.hpp>
```
src/tests/master_tests.cpp (line 3655)
<https://reviews.apache.org/r/38125/#comment153759>
Please add a heading comment that briefly describes this test in 1 or 2 lines.
src/tests/master_tests.cpp (line 3657)
<https://reviews.apache.org/r/38125/#comment153755>
Seems rather obvious, does this comment really add any value?
Also the first two comments use this "step-scheme" but then you dont follow that pattern anymore - I think we can safely remove both (start master and start slave).
src/tests/master_tests.cpp (line 3676)
<https://reviews.apache.org/r/38125/#comment153756>
Lets add a blank line here.
src/tests/master_tests.cpp (line 3678)
<https://reviews.apache.org/r/38125/#comment153758>
Insert blank line here, please
- Till Toenshoff
On Sept. 4, 2015, 10:57 a.m., Bernd Mathiske wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38125/
> -----------------------------------------------------------
>
> (Updated Sept. 4, 2015, 10:57 a.m.)
>
>
> Review request for mesos, Jan Schlicht and Till Toenshoff.
>
>
> Bugs: MESOS-115
> https://issues.apache.org/jira/browse/MESOS-115
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Now including the hostname in the SlaveID, for human consumption. Also testing this in a unit test.
>
>
> Diffs
> -----
>
> src/master/master.hpp e1331851c19e3372a4a525dcfd7ba2a01c3e97a6
> src/master/master.cpp 5589eca4317b597de509f3387cfc349083b361ac
> src/tests/master_tests.cpp 8a6b98b9f59ead20f537eb60b5084feed069a5b1
>
> Diff: https://reviews.apache.org/r/38125/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bernd Mathiske
>
>
Re: Review Request 38125: Included hostname in SlaveID.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38125/#review97871
-----------------------------------------------------------
What is the benefit of this change? I'm a bit reluctant to conflate ID and additional information. A high-level question: can hostname change on the slave failover? If yes, we may end up in a situation, when the information in the `SlaveID` is misleading, which is worse than absence of it.
- Alexander Rukletsov
On Sept. 4, 2015, 10:57 a.m., Bernd Mathiske wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38125/
> -----------------------------------------------------------
>
> (Updated Sept. 4, 2015, 10:57 a.m.)
>
>
> Review request for mesos, Jan Schlicht and Till Toenshoff.
>
>
> Bugs: MESOS-115
> https://issues.apache.org/jira/browse/MESOS-115
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Now including the hostname in the SlaveID, for human consumption. Also testing this in a unit test.
>
>
> Diffs
> -----
>
> src/master/master.hpp e1331851c19e3372a4a525dcfd7ba2a01c3e97a6
> src/master/master.cpp 5589eca4317b597de509f3387cfc349083b361ac
> src/tests/master_tests.cpp 8a6b98b9f59ead20f537eb60b5084feed069a5b1
>
> Diff: https://reviews.apache.org/r/38125/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bernd Mathiske
>
>