> On Sept. 21, 2015, 7:39 p.m., Benjamin Hindman wrote:
> > src/master/master.cpp, line 345
> > <https://reviews.apache.org/r/38473/diff/3/?file=1077335#file1077335line345>
> >
> > Why are you reading LIBPROCESS_IP here? Why not `flags.ip`? There
> > appears to be a very non-obvious global invariant and I'd like to capture
> > the dependency here as a comment (for posterity!) as well as where we're
> > setting LIBPROCESS_IP in the main.cpp files.
I've added the following comment, please let me know if (a) I'm getting this
wrong and (b) whether it requires a more detailed explanation (but the
underlying rationale is - libprocess will use that (and not the flags) and we
need to be consistent with it).
```
// We need to use LIBPROCESS_IP here, instead of 'flags.ip' because the
// latter may not have been set, and the IP may have been set by other
// means (eg, auto-discovery; the --ip_discovery_command) and we need to
// be consistent with what 'libprocess' is initialized with.
```
> On Sept. 21, 2015, 7:39 p.m., Benjamin Hindman wrote:
> > src/master/master.cpp, line 349
> > <https://reviews.apache.org/r/38473/diff/3/?file=1077335#file1077335line349>
> >
> > In circumstances where a user can easily avoid the master from crashing
> > we prefer NOT to use LOG(FATAL) because it prints a stack trace which can
> > hide the actual error message. Instead, an EXIT(EXIT_FAILURE) is a good
> > thing to use here.
> >
> > Same for the LOG(FATAL) in the slave below.
ah! I was trying to be "consistent" with the (existing) code above.
Would you like me to change that one too?
> On Sept. 21, 2015, 7:39 p.m., Benjamin Hindman wrote:
> > src/tests/master_tests.cpp, line 1121
> > <https://reviews.apache.org/r/38473/diff/3/?file=1077340#file1077340line1121>
> >
> > Why not just do `cluster.find`? Not sure I understand the need for this
> > alias.
You are right, no need - I started out adding this, then eventually gone down
the rabbit hole of same-named classes and hadn't realized that I could actually
get rid of the alias.
It's gone.
> On Sept. 21, 2015, 7:39 p.m., Benjamin Hindman wrote:
> > src/tests/master_tests.cpp, lines 1122-1123
> > <https://reviews.apache.org/r/38473/diff/3/?file=1077340#file1077340line1122>
> >
> > What does someone running the test get from this extra output
> > information?
Mostly the hostname that was set up and then not found via the UPID lookup - it
may help debug weird network configuration issues and/or situations where we
have broken the hostname lookup/configuration.
(I think? I like to add extra info when tests go wrong, so as to take out (some
of) the guesswork from the poor soul who'll have to fix my code many
months/years in the future... I believe in karma :) )
Anyways, figured out that (a) this needs to be an `ASSERT_EQ` (as we
`master.get()` in the following EXPECT) and (b) the comment here may actually
be unnecessary, so I've removed it.
- Marco
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38473/#review99812
-----------------------------------------------------------
On Sept. 19, 2015, 1:25 a.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38473/
> -----------------------------------------------------------
>
> (Updated Sept. 19, 2015, 1:25 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Cody Maloney, and Neil Conway.
>
>
> Bugs: MESOS-3457
> https://issues.apache.org/jira/browse/MESOS-3457
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Under certain circumstances, dynamic lookup of hostname, while
> successful, provides undesirable results; we would also like, in
> those circumstances, be able to just set the hostname to the chosen
> IP address (possibly set via the --ip_discovery_command method).
>
> The flag we add here, --[no]-hostname_lookup is `true` by default
> (which is the existing behavior) and will work under most
> circumstances: however, by disabling lookup (using, for example,
> --no_hostname_lookup) the hostname used will be the same as the
> one configured (possibly, via --ip or MESOS_IP) in `LIBPROCESS_IP`.
>
> This change affects both Master/Agent nodes.
>
> WARNING - the `Address::hostname()` method always does a dynamic
> hostname lookup, which may in fact return inconsistent results
> wrt the Master's configuration (this is *not* affected by
> this change) and should be avoided; use instead
> `Master::info()::hostname()` which is always consistent with
> the Master's configuration.
>
>
> Diffs
> -----
>
> docs/configuration.md dd7f4aa806a3c1a8653a0fda9a326a3707308e7c
> src/master/flags.hpp e4b1df3f5a33049defff4688463274067f1f1ebf
> src/master/flags.cpp 80879611fbcfd764c9fc8f60a31613a9c8fc2364
> src/master/master.cpp ca4d5876dcd427964111428edc22d567ddaede0b
> src/slave/flags.hpp e31a4183170c3442ac4a15365c229391e7e91480
> src/slave/flags.cpp add4196dfd06c0f602ff5ebd39960dc05c4cd11f
> src/slave/slave.cpp ad710d7b930a2f115d503ceb8f8fd7421ad30287
> src/tests/cluster.hpp 114583de8c867495a2b7a953e6826708838e5d23
> src/tests/master_tests.cpp a477794df37c658b80d79dea8555b001415f7b6a
> src/tests/mesos.hpp 3db97aca921c9216d90384e1eb17030849516454
>
> Diff: https://reviews.apache.org/r/38473/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Marco Massenzio
>
>
