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

Reply via email to