You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by zhou xing <xi...@cn.ibm.com> on 2016/04/26 08:06:24 UTC
Review Request 46676: Slave/Agent Rename Phase I: Rename
'/include/mesos/slave' folder.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46676/
-----------------------------------------------------------
Review request for mesos, Kevin Klues and Vinod Kone.
Bugs: mesos-5230
https://issues.apache.org/jira/browse/mesos-5230
Repository: mesos
Description
-------
[#MESOS-5230]
Change the 'make install' includedir name from slave to agent.
Diffs
-----
CHANGELOG 57f5457640c9b7ca2cb7707f2962dd0421f090f7
src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
Diff: https://reviews.apache.org/r/46676/diff/
Testing
-------
cd mesos
./bootstrap
mkdir build
cd build
../configure --prefix=${HOME}/install/mesos
make
make check
make install
Thanks,
zhou xing
Re: Review Request 46676: Slave/Agent Rename Phase I: Rename
'/include/mesos/slave' folder.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46676/#review130557
-----------------------------------------------------------
Patch looks great!
Reviews applied: [46676]
Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On April 26, 2016, 6:06 a.m., zhou xing wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46676/
> -----------------------------------------------------------
>
> (Updated April 26, 2016, 6:06 a.m.)
>
>
> Review request for mesos, Kevin Klues and Vinod Kone.
>
>
> Bugs: mesos-5230
> https://issues.apache.org/jira/browse/mesos-5230
>
>
> Repository: mesos
>
>
> Description
> -------
>
> [#MESOS-5230]
> Change the 'make install' includedir name from slave to agent.
>
>
> Diffs
> -----
>
> CHANGELOG 57f5457640c9b7ca2cb7707f2962dd0421f090f7
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
>
> Diff: https://reviews.apache.org/r/46676/diff/
>
>
> Testing
> -------
>
> cd mesos
> ./bootstrap
> mkdir build
> cd build
> ../configure --prefix=${HOME}/install/mesos
> make
> make check
> make install
>
>
> Thanks,
>
> zhou xing
>
>
Re: Review Request 46676: Slave/Agent Rename Phase I: Rename
'/include/mesos/slave' folder.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46676/#review130716
-----------------------------------------------------------
Patch looks great!
Reviews applied: [46720, 46676]
Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On April 27, 2016, 1:57 a.m., zhou xing wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46676/
> -----------------------------------------------------------
>
> (Updated April 27, 2016, 1:57 a.m.)
>
>
> Review request for mesos, Kevin Klues and Vinod Kone.
>
>
> Bugs: mesos-5230
> https://issues.apache.org/jira/browse/mesos-5230
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch did the following changes:
>
> 1. Change the 'make install' headers installation location from
> '$(DESTDIR)/include/mesos/slave' to '$(DESTDIR)include/mesos/agent'
>
> 2. As we do not change the folder name from 'include/mesos/slave'
> to 'include/mesos/agent', create a 'slave -> agent' symlink in dir
> '$(DESTDIR)/include/mesos'
>
>
> Diffs
> -----
>
> CHANGELOG 57f5457640c9b7ca2cb7707f2962dd0421f090f7
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
>
> Diff: https://reviews.apache.org/r/46676/diff/
>
>
> Testing
> -------
>
> cd mesos
> ./bootstrap
> mkdir build
> cd build
> ../configure --prefix=${HOME}/install/mesos
> make
> make check
> make install
>
>
> Thanks,
>
> zhou xing
>
>
Re: Review Request 46676: Slave/Agent Rename Phase I: Rename
'/include/mesos/slave' folder.
Posted by zhou xing <xi...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46676/
-----------------------------------------------------------
(Updated 四月 27, 2016, 1:57 a.m.)
Review request for mesos, Kevin Klues and Vinod Kone.
Changes
-------
Rebase code, change the commit description and update the makefile target comments
Bugs: mesos-5230
https://issues.apache.org/jira/browse/mesos-5230
Repository: mesos
Description (updated)
-------
This patch did the following changes:
1. Change the 'make install' headers installation location from
'$(DESTDIR)/include/mesos/slave' to '$(DESTDIR)include/mesos/agent'
2. As we do not change the folder name from 'include/mesos/slave'
to 'include/mesos/agent', create a 'slave -> agent' symlink in dir
'$(DESTDIR)/include/mesos'
Diffs (updated)
-----
CHANGELOG 57f5457640c9b7ca2cb7707f2962dd0421f090f7
src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
Diff: https://reviews.apache.org/r/46676/diff/
Testing
-------
cd mesos
./bootstrap
mkdir build
cd build
../configure --prefix=${HOME}/install/mesos
make
make check
make install
Thanks,
zhou xing
Re: Review Request 46676: Slave/Agent Rename Phase I: Rename
'/include/mesos/slave' folder.
Posted by zhou xing <xi...@cn.ibm.com>.
> On 四月 26, 2016, 7:08 p.m., Vinod Kone wrote:
> > src/Makefile.am, lines 2151-2152
> > <https://reviews.apache.org/r/46676/diff/1/?file=1361012#file1361012line2151>
> >
> > IIUC, when someone does a `make install` there will be 2 directories in the installation directory.
> >
> > ...../mesos/agent
> > ...../mesos/slave (this is a symlink to ../agent)
> >
> > In the git repo itself we will still have `include/mesos/slave` directory? I guess renaming this directory to `incluee/mesos/agent`? is too much work?
>
> Kevin Klues wrote:
> The `include/mesos/slave` directory is decoupled from the actually install directory specified by `agentdir`. We can decide to change `include/mesos/slave` to `include/mesos/agent` in the future, but it has nothing to do with this installation directory.
If we want to change include/mesos/slave to include/mesos/agent in the future, for the files that depend on the headers in include/mesos/slave, we need to keep the path, is that ok?
- zhou
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46676/#review130664
-----------------------------------------------------------
On 四月 26, 2016, 6:06 a.m., zhou xing wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46676/
> -----------------------------------------------------------
>
> (Updated 四月 26, 2016, 6:06 a.m.)
>
>
> Review request for mesos, Kevin Klues and Vinod Kone.
>
>
> Bugs: mesos-5230
> https://issues.apache.org/jira/browse/mesos-5230
>
>
> Repository: mesos
>
>
> Description
> -------
>
> [#MESOS-5230]
> Change the 'make install' includedir name from slave to agent.
>
>
> Diffs
> -----
>
> CHANGELOG 57f5457640c9b7ca2cb7707f2962dd0421f090f7
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
>
> Diff: https://reviews.apache.org/r/46676/diff/
>
>
> Testing
> -------
>
> cd mesos
> ./bootstrap
> mkdir build
> cd build
> ../configure --prefix=${HOME}/install/mesos
> make
> make check
> make install
>
>
> Thanks,
>
> zhou xing
>
>
Re: Review Request 46676: Slave/Agent Rename Phase I: Rename
'/include/mesos/slave' folder.
Posted by Kevin Klues <kl...@gmail.com>.
> On April 26, 2016, 7:08 p.m., Vinod Kone wrote:
> > src/Makefile.am, line 535
> > <https://reviews.apache.org/r/46676/diff/1/?file=1361012#file1361012line535>
> >
> > what does setting this variable do?
This is an autotools thing. If you define this and then list which headers to installe here with e.g. `agent_HEADERS`, they will be installed here. This is the right thing to do, in my opinion.
> On April 26, 2016, 7:08 p.m., Vinod Kone wrote:
> > src/Makefile.am, lines 2151-2152
> > <https://reviews.apache.org/r/46676/diff/1/?file=1361012#file1361012line2151>
> >
> > IIUC, when someone does a `make install` there will be 2 directories in the installation directory.
> >
> > ...../mesos/agent
> > ...../mesos/slave (this is a symlink to ../agent)
> >
> > In the git repo itself we will still have `include/mesos/slave` directory? I guess renaming this directory to `incluee/mesos/agent`? is too much work?
The `include/mesos/slave` directory is decoupled from the actually install directory specified by `agentdir`. We can decide to change `include/mesos/slave` to `include/mesos/agent` in the future, but it has nothing to do with this installation directory.
- Kevin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46676/#review130664
-----------------------------------------------------------
On April 26, 2016, 6:06 a.m., zhou xing wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46676/
> -----------------------------------------------------------
>
> (Updated April 26, 2016, 6:06 a.m.)
>
>
> Review request for mesos, Kevin Klues and Vinod Kone.
>
>
> Bugs: mesos-5230
> https://issues.apache.org/jira/browse/mesos-5230
>
>
> Repository: mesos
>
>
> Description
> -------
>
> [#MESOS-5230]
> Change the 'make install' includedir name from slave to agent.
>
>
> Diffs
> -----
>
> CHANGELOG 57f5457640c9b7ca2cb7707f2962dd0421f090f7
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
>
> Diff: https://reviews.apache.org/r/46676/diff/
>
>
> Testing
> -------
>
> cd mesos
> ./bootstrap
> mkdir build
> cd build
> ../configure --prefix=${HOME}/install/mesos
> make
> make check
> make install
>
>
> Thanks,
>
> zhou xing
>
>
Re: Review Request 46676: Slave/Agent Rename Phase I: Rename
'/include/mesos/slave' folder.
Posted by Vinod Kone <vi...@gmail.com>.
> On April 26, 2016, 7:08 p.m., Vinod Kone wrote:
> > src/Makefile.am, lines 2151-2152
> > <https://reviews.apache.org/r/46676/diff/1/?file=1361012#file1361012line2151>
> >
> > IIUC, when someone does a `make install` there will be 2 directories in the installation directory.
> >
> > ...../mesos/agent
> > ...../mesos/slave (this is a symlink to ../agent)
> >
> > In the git repo itself we will still have `include/mesos/slave` directory? I guess renaming this directory to `incluee/mesos/agent`? is too much work?
>
> Kevin Klues wrote:
> The `include/mesos/slave` directory is decoupled from the actually install directory specified by `agentdir`. We can decide to change `include/mesos/slave` to `include/mesos/agent` in the future, but it has nothing to do with this installation directory.
>
> zhou xing wrote:
> If we want to change include/mesos/slave to include/mesos/agent in the future, for the files that depend on the headers in include/mesos/slave, we need to keep the path, is that ok?
I think we should send an email to dev@/user@/modules@ mailing list when we do change "include/mesos/slave" to "include/mesos/agent" in the repo. It's not just changing the path name, we need to change the package names of the protos in this file from mesos.slave to mesos.agent. @zhou can you send that email and start the conversation?
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46676/#review130664
-----------------------------------------------------------
On April 27, 2016, 1:57 a.m., zhou xing wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46676/
> -----------------------------------------------------------
>
> (Updated April 27, 2016, 1:57 a.m.)
>
>
> Review request for mesos, Kevin Klues and Vinod Kone.
>
>
> Bugs: mesos-5230
> https://issues.apache.org/jira/browse/mesos-5230
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch did the following changes:
>
> 1. Change the 'make install' headers installation location from
> '$(DESTDIR)/include/mesos/slave' to '$(DESTDIR)include/mesos/agent'
>
> 2. As we do not change the folder name from 'include/mesos/slave'
> to 'include/mesos/agent', create a 'slave -> agent' symlink in dir
> '$(DESTDIR)/include/mesos'
>
>
> Diffs
> -----
>
> CHANGELOG 57f5457640c9b7ca2cb7707f2962dd0421f090f7
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
>
> Diff: https://reviews.apache.org/r/46676/diff/
>
>
> Testing
> -------
>
> cd mesos
> ./bootstrap
> mkdir build
> cd build
> ../configure --prefix=${HOME}/install/mesos
> make
> make check
> make install
>
>
> Thanks,
>
> zhou xing
>
>
Re: Review Request 46676: Slave/Agent Rename Phase I: Rename
'/include/mesos/slave' folder.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46676/#review130664
-----------------------------------------------------------
Can you make the description more descriptive please? Make it clear what the change is here. Also, no need for including bug-id in the description. It is already attached to the review.
CHANGELOG (line 76)
<https://reviews.apache.org/r/46676/#comment194497>
"Slave/Agent" is correct. The earlier ones called it "Replace Master/Slave Terminology". Can you fix the earlier ones? You need to fix the name of the actual tickets in the JIRA and also this changelog. Please send a separate review for the CHANGELOG.
src/Makefile.am (line 535)
<https://reviews.apache.org/r/46676/#comment194499>
what does setting this variable do?
src/Makefile.am (line 2144)
<https://reviews.apache.org/r/46676/#comment194498>
Update the comment.
src/Makefile.am (lines 2151 - 2152)
<https://reviews.apache.org/r/46676/#comment194501>
IIUC, when someone does a `make install` there will be 2 directories in the installation directory.
...../mesos/agent
...../mesos/slave (this is a symlink to ../agent)
In the git repo itself we will still have `include/mesos/slave` directory? I guess renaming this directory to `incluee/mesos/agent`? is too much work?
- Vinod Kone
On April 26, 2016, 6:06 a.m., zhou xing wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46676/
> -----------------------------------------------------------
>
> (Updated April 26, 2016, 6:06 a.m.)
>
>
> Review request for mesos, Kevin Klues and Vinod Kone.
>
>
> Bugs: mesos-5230
> https://issues.apache.org/jira/browse/mesos-5230
>
>
> Repository: mesos
>
>
> Description
> -------
>
> [#MESOS-5230]
> Change the 'make install' includedir name from slave to agent.
>
>
> Diffs
> -----
>
> CHANGELOG 57f5457640c9b7ca2cb7707f2962dd0421f090f7
> src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca
>
> Diff: https://reviews.apache.org/r/46676/diff/
>
>
> Testing
> -------
>
> cd mesos
> ./bootstrap
> mkdir build
> cd build
> ../configure --prefix=${HOME}/install/mesos
> make
> make check
> make install
>
>
> Thanks,
>
> zhou xing
>
>