mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kevin Klues <klue...@gmail.com>
Subject Re: Review Request 45806: Replace Master/Slave Terminology Phase I - Duplicate/Rename binaries.
Date Fri, 08 Apr 2016 00:40:30 GMT

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



Thanks for the update!

Doing a quick `git-grep` after applying the patch, I still see a few references to `mesos-slave`
throughout the code base.  Alot of these are in the documentation (which I think should be
covered by MESOS-3783).  However, the following other files still have references:
```
src/slave/containerizer/mesos/launch.cpp
src/tests/balloon_framework_test.sh
support/generate-endpoint-help.py
support/test-upgrade.py
```
These files should likely be updated to refer to `mesos-agent` as part of this patch.

There is also this file:
```
3rdparty/libprocess/cmake/Process3rdpartyConfigure.cmake
```
which it's not clear we should change as part of this patch, but I talked to @vinod and he
thinks we should. The Cmake stuff isn't really supported yet, so he thinks it's OK to just
sort of blindly change it and let those working on the Cmake port sort things out. I'd suggest:
```
# Define target for AGENT.
##########################
set(
  AGENT_TARGET mesos-agent
  CACHE STRING "Target we use to refer to the agent executable")
```

Also, we should probably make it clear in both the commit message and the "Testing" section
of the review that a full `bootstrap` is required once this patch has landed.  Otherwise,
`configure` will break because of the missing `mesos-slave-*.sh` scripts.
```
cd mesos
./bootstrap
mkdir build
cd build
../configure --prefix=${HOME}/install/mesos
make
```

I ran through this as well as doing a `make install` and inspecting the contents of `${HOME}/install/mesos`
I see:
```
[klueska@core-dev build]$ find ${HOME}/install/mesos/ -name "*slave*"
${HOME}/install/mesos/include/mesos/slave
${HOME}/install/mesos/sbin/mesos-start-slaves.sh
${HOME}/install/mesos/sbin/mesos-stop-slaves.sh
${HOME}/install/mesos/sbin/mesos-slave
${HOME}/install/mesos/share/mesos/webui/master/static/slave.html
${HOME}/install/mesos/share/mesos/webui/master/static/slave_executor.html
${HOME}/install/mesos/share/mesos/webui/master/static/slave_framework.html
${HOME}/install/mesos/share/mesos/webui/master/static/slaves.html
${HOME}/install/mesos/etc/mesos/mesos-slave-env.sh.template

[klueska@core-dev build]$ find ${HOME}/install/mesos/ -name "*agent*"
${HOME}/install/mesos/sbin/mesos-start-agents.sh
${HOME}/install/mesos/sbin/mesos-stop-agents.sh
${HOME}/install/mesos/sbin/mesos-agent
${HOME}/install/mesos/etc/mesos/mesos-agent-env.sh.template
```

which seems consistent with what we want for this patch.

However there are still references to the word 'slave' in places such as `include/mesos/slave`
and the webui stuff.  The webui stuff should be covered by MESOS-3779, but there is no ticket
covering a rename of the `include/mesos/slave` folder.  I talked to @vinod about this, and
he suggested making a separate ticket to do this rename. Glancing through `src/Makefile.am`,
it may be enough to just change:
```
slavedir = $(pkgincludedir)/slave

slave_HEADERS =               \
  $(top_srcdir)/include/mesos/slave/container_logger.hpp    \
  $(top_srcdir)/include/mesos/slave/isolator.hpp      \
  $(top_srcdir)/include/mesos/slave/isolator.proto      \
  $(top_srcdir)/include/mesos/slave/oversubscription.hpp    \
  $(top_srcdir)/include/mesos/slave/oversubscription.proto    \
  $(top_srcdir)/include/mesos/slave/qos_controller.hpp      \
  $(top_srcdir)/include/mesos/slave/resource_estimator.hpp

nodist_slave_HEADERS =              \
  ../include/mesos/slave/isolator.pb.h          \
  ../include/mesos/slave/oversubscription.pb.h
```
to
```
agentdir = $(pkgincludedir)/agent

agent_HEADERS =               \
  $(top_srcdir)/include/mesos/slave/container_logger.hpp    \
  $(top_srcdir)/include/mesos/slave/isolator.hpp      \
  $(top_srcdir)/include/mesos/slave/isolator.proto      \
  $(top_srcdir)/include/mesos/slave/oversubscription.hpp    \
  $(top_srcdir)/include/mesos/slave/oversubscription.proto    \
  $(top_srcdir)/include/mesos/slave/qos_controller.hpp      \
  $(top_srcdir)/include/mesos/slave/resource_estimator.hpp

nodist_agent_HEADERS =              \
  ../include/mesos/slave/isolator.pb.h          \
  ../include/mesos/slave/oversubscription.pb.h
```

and then add another command to your install/uninstall-hooks to create a symlink from `agent->slave`.
Possibly renaming `copy-agent-env-template` to something more meaningful to cover both the
existing stuff, plus setting up this symlink.

Finally, the CHANGELOG should be updated as part of this patch to point out all of the changes
that have been made.

Thanks!


src/Makefile.am (line 2097)
<https://reviews.apache.org/r/45806/#comment191077>

    I would probably put a space here after the colon.


- Kevin Klues


On April 7, 2016, 7:10 a.m., zhou xing wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45806/
> -----------------------------------------------------------
> 
> (Updated April 7, 2016, 7:10 a.m.)
> 
> 
> Review request for mesos, Kevin Klues and Vinod Kone.
> 
> 
> Bugs: MESOS-3782
>     https://issues.apache.org/jira/browse/MESOS-3782
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> [#mesos-3782]
> In this patch, we did the following changes:
> 1. Duplicate executable file 'mesos-slave' with 'mesos-agent'
> 2. Rename the scripts in folder 'mesos/bin' & 'mesos/src/deploy'
> to use term 'agent' instead of term 'slave'
> 3. Change the content of ths scripts to use term 'agent' instead
> of term 'slave'
> 4. Duplicate the renamed scripts renamed in #2 during the configure
> and make process, the duplicated scripts still use term 'slave'
> in their names.
> 
> 
> Diffs
> -----
> 
>   bin/gdb-mesos-slave.sh.in dbeec8464b26bd808f7a50b8412a2778f1458f22 
>   bin/lldb-mesos-slave.sh.in 896c411b2b05d3c4a14288002520a5391a88d955 
>   bin/mesos-local-flags.sh.in ab5b6c8bd8847485c5a47d637c9f4fe88c59ae65 
>   bin/mesos-slave-flags.sh.in  
>   bin/mesos-slave.sh.in 1e3b748ed4dc32ba6bd8adece20f439bce38abc3 
>   bin/valgrind-mesos-slave.sh.in 900c5883d96cf14e121e566bcf1ad4dc9a47abf6 
>   configure.ac c693b825294f82ace8a14563cf2229820e159e3c 
>   src/Makefile.am 71c4308cccfa5c56b93f6c3928dd2a1cf3ba9741 
>   src/deploy/mesos-deploy-env.sh.template bea5584fbcc68825b1c35b370aed17b0e432edd5 
>   src/deploy/mesos-slave-env.sh.template 31567d6a47e19385aed56edfc7e67457c8cdde3e 
>   src/deploy/mesos-start-cluster.sh.in f7a003d9a8e5bbb4f11353988e55e715da0b2b4f 
>   src/deploy/mesos-start-slaves.sh.in 50860f40e33fcbb1787be6c035873de4bcc83de5 
>   src/deploy/mesos-stop-cluster.sh.in e5f8c1fb400c56715774889632aa74d9eac33645 
>   src/deploy/mesos-stop-slaves.sh.in 3dd9b51edff2beb3ccc8d5dd44f0cdc265f623f9 
> 
> Diff: https://reviews.apache.org/r/45806/diff/
> 
> 
> Testing
> -------
> 
> make
> make check
> 
> 
> Thanks,
> 
> zhou xing
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message