You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Maged Michael <ma...@gmail.com> on 2016/02/04 20:09:08 UTC
Re: Review Request 43144: Added env var to set upper bound on number
of libprocess worker threads.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43144/
-----------------------------------------------------------
(Updated Feb. 4, 2016, 7:09 p.m.)
Review request for mesos and Klaus Ma.
Summary (updated)
-----------------
Added env var to set upper bound on number of libprocess worker threads.
Repository: mesos
Description (updated)
-------
Added env var to set upper bound on number of libprocess worker threads.
Diffs (updated)
-----
3rdparty/libprocess/src/process.cpp d8a74d7637d20c81f384e974e4fdeba22effb437
Diff: https://reviews.apache.org/r/43144/diff/
Testing (updated)
-------
Passed make check on x86_64 Ubuntu 14.04.
Invocations of mesos-master, mesos-slave, and mesos-tests with the LIBPROCESS_MAX_WORKER_THREADS set to the values "1", "7", "9", "1000", "0", "-1", and "abc" on a 4-core x86_64 Ubuntu 14.04 system. The results were as expected. 1, 7, 8, 8, 8, 8, and 8 worker threads were created, respectively. The last 3 tests generated warnings.
Thanks,
Maged Michael
Re: Review Request 43144: Added env var to set upper bound on number
of libprocess worker threads.
Posted by Maged Michael <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43144/
-----------------------------------------------------------
(Updated Feb. 5, 2016, 9:45 p.m.)
Review request for mesos and Klaus Ma.
Bugs: MESOS-4353
https://issues.apache.org/jira/browse/MESOS-4353
Repository: mesos
Description
-------
Added env var to set upper bound on number of libprocess worker threads.
Diffs (updated)
-----
3rdparty/libprocess/src/process.cpp d8a74d7
docs/configuration.md 4b5a394
Diff: https://reviews.apache.org/r/43144/diff/
Testing
-------
Passed make check on x86_64 Ubuntu 14.04.
Invocations of mesos-master, mesos-slave, and mesos-tests with the LIBPROCESS_MAX_WORKER_THREADS set to the values "1", "7", "9", "1000", "0", "-1", and "abc" on a 4-core x86_64 Ubuntu 14.04 system. The results were as expected. 1, 7, 8, 8, 8, 8, and 8 worker threads were created, respectively. The last 3 tests generated warnings.
Thanks,
Maged Michael
Re: Review Request 43144: Added env var to set upper bound on number
of libprocess worker threads.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43144/#review117975
-----------------------------------------------------------
3rdparty/libprocess/src/process.cpp (line 2222)
<https://reviews.apache.org/r/43144/#comment179285>
Can you please also log the system defined number into log message?
- Guangya Liu
On 二月 5, 2016, 3:39 a.m., Maged Michael wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43144/
> -----------------------------------------------------------
>
> (Updated 二月 5, 2016, 3:39 a.m.)
>
>
> Review request for mesos and Klaus Ma.
>
>
> Bugs: MESOS-4353
> https://issues.apache.org/jira/browse/MESOS-4353
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added env var to set upper bound on number of libprocess worker threads.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp d8a74d7
>
> Diff: https://reviews.apache.org/r/43144/diff/
>
>
> Testing
> -------
>
> Passed make check on x86_64 Ubuntu 14.04.
>
> Invocations of mesos-master, mesos-slave, and mesos-tests with the LIBPROCESS_MAX_WORKER_THREADS set to the values "1", "7", "9", "1000", "0", "-1", and "abc" on a 4-core x86_64 Ubuntu 14.04 system. The results were as expected. 1, 7, 8, 8, 8, 8, and 8 worker threads were created, respectively. The last 3 tests generated warnings.
>
>
> Thanks,
>
> Maged Michael
>
>
Re: Review Request 43144: Added env var to set upper bound on number
of libprocess worker threads.
Posted by Maged Michael <ma...@gmail.com>.
> On Feb. 5, 2016, 2:20 p.m., Qian Zhang wrote:
> > 1. Besides changing the code, you may also need to update the following doc to describe the newly introduced env var "LIBPROCESS_MAX_WORKER_THREADS"
> > https://github.com/apache/mesos/blob/master/docs/configuration.md#libprocess-options
> > 2. Have you test the patch with executor from E2E? For example, start Mesos agent with "LIBPROCESS_MAX_WORKER_THREADS" in its "--executor_environment_variables" option, launch a task, and check if the libprocess worker thread number of the related executor is the expected number.
1. Done.
2. No. Can someone point me to a simple test to view executor logs?
- Maged
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43144/#review118012
-----------------------------------------------------------
On Feb. 5, 2016, 9:46 p.m., Maged Michael wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43144/
> -----------------------------------------------------------
>
> (Updated Feb. 5, 2016, 9:46 p.m.)
>
>
> Review request for mesos, Guangya Liu, Klaus Ma, and Qian Zhang.
>
>
> Bugs: MESOS-4353
> https://issues.apache.org/jira/browse/MESOS-4353
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added env var to set upper bound on number of libprocess worker threads.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp d8a74d7
> docs/configuration.md 4b5a394
>
> Diff: https://reviews.apache.org/r/43144/diff/
>
>
> Testing
> -------
>
> Passed make check on x86_64 Ubuntu 14.04.
>
> Invocations of mesos-master, mesos-slave, and mesos-tests with the LIBPROCESS_MAX_WORKER_THREADS set to the values "1", "7", "9", "1000", "0", "-1", and "abc" on a 4-core x86_64 Ubuntu 14.04 system. The results were as expected. 1, 7, 8, 8, 8, 8, and 8 worker threads were created, respectively. The last 3 tests generated warnings.
>
>
> Thanks,
>
> Maged Michael
>
>
Re: Review Request 43144: Added env var to set upper bound on number
of libprocess worker threads.
Posted by Qian Zhang <zh...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43144/#review118012
-----------------------------------------------------------
1. Besides changing the code, you may also need to update the following doc to describe the newly introduced env var "LIBPROCESS_MAX_WORKER_THREADS"
https://github.com/apache/mesos/blob/master/docs/configuration.md#libprocess-options
2. Have you test the patch with executor from E2E? For example, start Mesos agent with "LIBPROCESS_MAX_WORKER_THREADS" in its "--executor_environment_variables" option, launch a task, and check if the libprocess worker thread number of the related executor is the expected number.
- Qian Zhang
On Feb. 5, 2016, 11:39 a.m., Maged Michael wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43144/
> -----------------------------------------------------------
>
> (Updated Feb. 5, 2016, 11:39 a.m.)
>
>
> Review request for mesos and Klaus Ma.
>
>
> Bugs: MESOS-4353
> https://issues.apache.org/jira/browse/MESOS-4353
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added env var to set upper bound on number of libprocess worker threads.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp d8a74d7
>
> Diff: https://reviews.apache.org/r/43144/diff/
>
>
> Testing
> -------
>
> Passed make check on x86_64 Ubuntu 14.04.
>
> Invocations of mesos-master, mesos-slave, and mesos-tests with the LIBPROCESS_MAX_WORKER_THREADS set to the values "1", "7", "9", "1000", "0", "-1", and "abc" on a 4-core x86_64 Ubuntu 14.04 system. The results were as expected. 1, 7, 8, 8, 8, 8, and 8 worker threads were created, respectively. The last 3 tests generated warnings.
>
>
> Thanks,
>
> Maged Michael
>
>
Re: Review Request 43144: Added env var to set upper bound on number
of libprocess worker threads.
Posted by Maged Michael <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43144/
-----------------------------------------------------------
(Updated Feb. 5, 2016, 3:39 a.m.)
Review request for mesos and Klaus Ma.
Bugs: MESOS-4353
https://issues.apache.org/jira/browse/MESOS-4353
Repository: mesos
Description
-------
Added env var to set upper bound on number of libprocess worker threads.
Diffs (updated)
-----
3rdparty/libprocess/src/process.cpp d8a74d7
Diff: https://reviews.apache.org/r/43144/diff/
Testing
-------
Passed make check on x86_64 Ubuntu 14.04.
Invocations of mesos-master, mesos-slave, and mesos-tests with the LIBPROCESS_MAX_WORKER_THREADS set to the values "1", "7", "9", "1000", "0", "-1", and "abc" on a 4-core x86_64 Ubuntu 14.04 system. The results were as expected. 1, 7, 8, 8, 8, 8, and 8 worker threads were created, respectively. The last 3 tests generated warnings.
Thanks,
Maged Michael
Re: Review Request 43144: Added env var to set upper bound on number
of libprocess worker threads.
Posted by Guangya Liu <gy...@gmail.com>.
> On 二月 5, 2016, 2:51 a.m., Guangya Liu wrote:
> > 3rdparty/libprocess/src/process.cpp, lines 2212-2213
> > <https://reviews.apache.org/r/43144/diff/3/?file=1232974#file1232974line2212>
> >
> > Why not
> >
> > Option<string> value = os::getenv("LIBPROCESS_MAX_WORKER_THREADS");
I see what you mean: it will be used in the log message.
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43144/#review117947
-----------------------------------------------------------
On 二月 4, 2016, 7:55 p.m., Maged Michael wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43144/
> -----------------------------------------------------------
>
> (Updated 二月 4, 2016, 7:55 p.m.)
>
>
> Review request for mesos and Klaus Ma.
>
>
> Bugs: MESOS-4353
> https://issues.apache.org/jira/browse/MESOS-4353
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added env var to set upper bound on number of libprocess worker threads.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp d8a74d7
>
> Diff: https://reviews.apache.org/r/43144/diff/
>
>
> Testing
> -------
>
> Passed make check on x86_64 Ubuntu 14.04.
>
> Invocations of mesos-master, mesos-slave, and mesos-tests with the LIBPROCESS_MAX_WORKER_THREADS set to the values "1", "7", "9", "1000", "0", "-1", and "abc" on a 4-core x86_64 Ubuntu 14.04 system. The results were as expected. 1, 7, 8, 8, 8, 8, and 8 worker threads were created, respectively. The last 3 tests generated warnings.
>
>
> Thanks,
>
> Maged Michael
>
>
Re: Review Request 43144: Added env var to set upper bound on number
of libprocess worker threads.
Posted by Guangya Liu <gy...@gmail.com>.
> On 二月 5, 2016, 2:51 a.m., Guangya Liu wrote:
> > 3rdparty/libprocess/src/process.cpp, line 2222
> > <https://reviews.apache.org/r/43144/diff/3/?file=1232974#file1232974line2222>
> >
> > I think that warning message should also be enhanced that
> >
> > `"Ignoring invalid value " << value.get() << " for LIBPROCESS_MAX_WORKER_THREADS, using system defined value " << cpus;`
>
> Maged Michael wrote:
> Done.
@Maged, you can just click `Fixed` button here to mark that the issue has been resolved.
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43144/#review117947
-----------------------------------------------------------
On 二月 5, 2016, 3:39 a.m., Maged Michael wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43144/
> -----------------------------------------------------------
>
> (Updated 二月 5, 2016, 3:39 a.m.)
>
>
> Review request for mesos and Klaus Ma.
>
>
> Bugs: MESOS-4353
> https://issues.apache.org/jira/browse/MESOS-4353
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added env var to set upper bound on number of libprocess worker threads.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp d8a74d7
>
> Diff: https://reviews.apache.org/r/43144/diff/
>
>
> Testing
> -------
>
> Passed make check on x86_64 Ubuntu 14.04.
>
> Invocations of mesos-master, mesos-slave, and mesos-tests with the LIBPROCESS_MAX_WORKER_THREADS set to the values "1", "7", "9", "1000", "0", "-1", and "abc" on a 4-core x86_64 Ubuntu 14.04 system. The results were as expected. 1, 7, 8, 8, 8, 8, and 8 worker threads were created, respectively. The last 3 tests generated warnings.
>
>
> Thanks,
>
> Maged Michael
>
>
Re: Review Request 43144: Added env var to set upper bound on number
of libprocess worker threads.
Posted by Maged Michael <ma...@gmail.com>.
> On Feb. 5, 2016, 2:51 a.m., Guangya Liu wrote:
> > 3rdparty/libprocess/src/process.cpp, lines 2212-2213
> > <https://reviews.apache.org/r/43144/diff/3/?file=1232974#file1232974line2212>
> >
> > Why not
> >
> > Option<string> value = os::getenv("LIBPROCESS_MAX_WORKER_THREADS");
>
> Guangya Liu wrote:
> I see what you mean: it will be used in the log message.
The string is used twice, once as argument of getenv and another time in the warning. My concern about repeating the string is that there could be a mismatch (e.g., we decide to change the environment variabvle but forget to update the warning) that is not caught by the compiler.
> On Feb. 5, 2016, 2:51 a.m., Guangya Liu wrote:
> > 3rdparty/libprocess/src/process.cpp, line 2220
> > <https://reviews.apache.org/r/43144/diff/3/?file=1232974#file1232974line2220>
> >
> > I think we should add some logs here if the `cpus` override the `max_cpus.get()`, the end user may be confused for such case: Why my env does not take effect?
How about?
if (max_cpus.get() < cpus) {
cpus = max_cpus.get();
VLOG(1) << "Overriding the system defined number of worker threads, "
<< "using the value " << cpus;
} else {
VLOG(1) << "The system defined number of worker threads " << cpus
<< " is unchanged, as it is not greater than " << env_var
<< "=" << max_cpus.get();
}
> On Feb. 5, 2016, 2:51 a.m., Guangya Liu wrote:
> > 3rdparty/libprocess/src/process.cpp, line 2222
> > <https://reviews.apache.org/r/43144/diff/3/?file=1232974#file1232974line2222>
> >
> > I think that warning message should also be enhanced that
> >
> > `"Ignoring invalid value " << value.get() << " for LIBPROCESS_MAX_WORKER_THREADS, using system defined value " << cpus;`
Done.
- Maged
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43144/#review117947
-----------------------------------------------------------
On Feb. 5, 2016, 3:39 a.m., Maged Michael wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43144/
> -----------------------------------------------------------
>
> (Updated Feb. 5, 2016, 3:39 a.m.)
>
>
> Review request for mesos and Klaus Ma.
>
>
> Bugs: MESOS-4353
> https://issues.apache.org/jira/browse/MESOS-4353
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added env var to set upper bound on number of libprocess worker threads.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp d8a74d7
>
> Diff: https://reviews.apache.org/r/43144/diff/
>
>
> Testing
> -------
>
> Passed make check on x86_64 Ubuntu 14.04.
>
> Invocations of mesos-master, mesos-slave, and mesos-tests with the LIBPROCESS_MAX_WORKER_THREADS set to the values "1", "7", "9", "1000", "0", "-1", and "abc" on a 4-core x86_64 Ubuntu 14.04 system. The results were as expected. 1, 7, 8, 8, 8, 8, and 8 worker threads were created, respectively. The last 3 tests generated warnings.
>
>
> Thanks,
>
> Maged Michael
>
>
Re: Review Request 43144: Added env var to set upper bound on number
of libprocess worker threads.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43144/#review117947
-----------------------------------------------------------
The document should also be updated `docs/configuration.md` by adding some explanation for this parameter.
3rdparty/libprocess/src/process.cpp (lines 2212 - 2213)
<https://reviews.apache.org/r/43144/#comment179239>
Why not
Option<string> value = os::getenv("LIBPROCESS_MAX_WORKER_THREADS");
3rdparty/libprocess/src/process.cpp (line 2220)
<https://reviews.apache.org/r/43144/#comment179241>
I think we should add some logs here if the `cpus` override the `max_cpus.get()`, the end user may be confused for such case: Why my env does not take effect?
3rdparty/libprocess/src/process.cpp (line 2222)
<https://reviews.apache.org/r/43144/#comment179240>
I think that warning message should also be enhanced that
`"Ignoring invalid value " << value.get() << " for LIBPROCESS_MAX_WORKER_THREADS, using system defined value " << cpus;`
- Guangya Liu
On 二月 4, 2016, 7:55 p.m., Maged Michael wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43144/
> -----------------------------------------------------------
>
> (Updated 二月 4, 2016, 7:55 p.m.)
>
>
> Review request for mesos and Klaus Ma.
>
>
> Bugs: MESOS-4353
> https://issues.apache.org/jira/browse/MESOS-4353
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added env var to set upper bound on number of libprocess worker threads.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp d8a74d7
>
> Diff: https://reviews.apache.org/r/43144/diff/
>
>
> Testing
> -------
>
> Passed make check on x86_64 Ubuntu 14.04.
>
> Invocations of mesos-master, mesos-slave, and mesos-tests with the LIBPROCESS_MAX_WORKER_THREADS set to the values "1", "7", "9", "1000", "0", "-1", and "abc" on a 4-core x86_64 Ubuntu 14.04 system. The results were as expected. 1, 7, 8, 8, 8, 8, and 8 worker threads were created, respectively. The last 3 tests generated warnings.
>
>
> Thanks,
>
> Maged Michael
>
>
Re: Review Request 43144: Added env var to set upper bound on number
of libprocess worker threads.
Posted by Maged Michael <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43144/
-----------------------------------------------------------
(Updated Feb. 4, 2016, 7:55 p.m.)
Review request for mesos and Klaus Ma.
Bugs: MESOS-4353
https://issues.apache.org/jira/browse/MESOS-4353
Repository: mesos
Description
-------
Added env var to set upper bound on number of libprocess worker threads.
Diffs
-----
3rdparty/libprocess/src/process.cpp d8a74d7
Diff: https://reviews.apache.org/r/43144/diff/
Testing
-------
Passed make check on x86_64 Ubuntu 14.04.
Invocations of mesos-master, mesos-slave, and mesos-tests with the LIBPROCESS_MAX_WORKER_THREADS set to the values "1", "7", "9", "1000", "0", "-1", and "abc" on a 4-core x86_64 Ubuntu 14.04 system. The results were as expected. 1, 7, 8, 8, 8, 8, and 8 worker threads were created, respectively. The last 3 tests generated warnings.
Thanks,
Maged Michael
Re: Review Request 43144: Added env var to set upper bound on number
of libprocess worker threads.
Posted by Maged Michael <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43144/
-----------------------------------------------------------
(Updated Feb. 4, 2016, 7:25 p.m.)
Review request for mesos and Klaus Ma.
Repository: mesos
Description
-------
Added env var to set upper bound on number of libprocess worker threads.
Diffs (updated)
-----
3rdparty/libprocess/src/process.cpp d8a74d7
Diff: https://reviews.apache.org/r/43144/diff/
Testing
-------
Passed make check on x86_64 Ubuntu 14.04.
Invocations of mesos-master, mesos-slave, and mesos-tests with the LIBPROCESS_MAX_WORKER_THREADS set to the values "1", "7", "9", "1000", "0", "-1", and "abc" on a 4-core x86_64 Ubuntu 14.04 system. The results were as expected. 1, 7, 8, 8, 8, 8, and 8 worker threads were created, respectively. The last 3 tests generated warnings.
Thanks,
Maged Michael