> 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
> 
>

Reply via email to