mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From zhou xing <xingz...@cn.ibm.com>
Subject Re: Review Request 45806: Replace Master/Slave Terminology Phase I - Duplicate/Rename binaries.
Date Wed, 13 Apr 2016 03:00:15 GMT


> On 四月 8, 2016, 12:40 a.m., Kevin Klues wrote:
> > 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!

Kevin, thanks for your comments!

We have updated the code, please take a look. for the CMake part, as we are not quite familiar
with it, just paste your suggestions in the code, if anything is wrong, please let us know
so that we can update the patch.

for slave folder rename part, we will create another ticket and track the changes there, thanks


- zhou


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


On 四月 13, 2016, 2:57 a.m., zhou xing wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45806/
> -----------------------------------------------------------
> 
> (Updated 四月 13, 2016, 2:57 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.
> 
> Please note that afull bootstrap is required once this patch has
> been applied. Otherwise, configure will break because of the
> missing mesos-slave-*.sh scripts. Please follow the steps to
> re-build the project:
> 
> ```
> cd mesos
> ./bootstrap
> mkdir build
> cd build
> ../configure --prefix=${HOME}/install/mesos
> make
> ```
> 
> 
> Diffs
> -----
> 
>   CHANGELOG 02b6094a6d4c096d7bee3bae294469fd8942998f 
>   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 4392909873e588b93e435d260276b0f1b0814c08 
>   src/Makefile.am a8f68316c5d4d4c82d99363535a97fa5b1caafc5 
>   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 
>   src/slave/containerizer/mesos/launch.cpp 54079c37143f4b5c22e0c9a8fe4bacb31f60ed1b 
>   src/tests/balloon_framework_test.sh ae32753e8942f77f94752543c384d218d6e4d48d 
>   support/generate-endpoint-help.py 5d23b10f0bab9c0e5848c0c1c26855522cf44a70 
>   support/test-upgrade.py 2c4061d71338f66e432dfa4ac86a9693f3ad38bf 
> 
> Diff: https://reviews.apache.org/r/45806/diff/
> 
> 
> Testing
> -------
> 
> cd mesos
> ./bootstrap
> mkdir build
> cd build
> ../configure --prefix=${HOME}/install/mesos
> make
> make check
> 
> 
> Thanks,
> 
> zhou xing
> 
>


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