mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joseph Wu <jos...@mesosphere.io>
Subject Re: Review Request 51565: Added a way to set logrotate settings per executor.
Date Tue, 06 Sep 2016 20:13:23 GMT


> On Sept. 5, 2016, 10:14 a.m., Vinod Kone wrote:
> > src/slave/container_loggers/lib_logrotate.hpp, line 42
> > <https://reviews.apache.org/r/51565/diff/1/?file=1489410#file1489410line42>
> >
> >     s/Options/Flags/ ?
> >     
> >     Also, can you add a comment for why you factored these out into a separate struct?

Also replaced `LoggerOptions options;` with `LoggerFlags settings;`.


> On Sept. 5, 2016, 10:14 a.m., Vinod Kone wrote:
> > src/slave/container_loggers/lib_logrotate.hpp, line 111
> > <https://reviews.apache.org/r/51565/diff/1/?file=1489410#file1489410line111>
> >
> >     does it not look into tasks's command info as well for command tasks?

We don't have access to the TaskInfo in the ContainerLogger, so it might not be appropriate
to mention the TaskInfo.  But the environment for command tasks is copied into the ExecutorInfo.


> On Sept. 5, 2016, 10:14 a.m., Vinod Kone wrote:
> > src/tests/container_logger_tests.cpp, line 588
> > <https://reviews.apache.org/r/51565/diff/1/?file=1489412#file1489412line588>
> >
> >     hmm. "MESOS_LOGROTATE_LOGGER_LOGROTATE_" looks weird.

I suppose a prefix like `CONTAINER_LOGGER_` will be less weird.  That'll result in variables
like: `CONTAINER_LOGGER_LOGROTATE_STDOUT_OPTIONS` and `CONTAINER_LOGGER_MAX_STDOUT_SIZE`.


> On Sept. 5, 2016, 10:14 a.m., Vinod Kone wrote:
> > src/tests/container_logger_tests.cpp, line 530
> > <https://reviews.apache.org/r/51565/diff/1/?file=1489412#file1489412line530>
> >
> >     new line.

In all (most?) of our tests, we `ASSERT_SOME(master)` without a newline before the assert.


- Joseph


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


On Sept. 6, 2016, 1:13 p.m., Joseph Wu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51565/
> -----------------------------------------------------------
> 
> (Updated Sept. 6, 2016, 1:13 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Cody Maloney, Artem Harutyunyan, and Vinod
Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The provided `LogrotateContainerLogger` did not have enough granularity
> when setting log rotation settings.  This patch adds a way for each
> executor to set its own log rotation settings, using the global values
> as defaults.
> 
> The executor settings are provided via environment variables in the
> `ExecutorInfo`.
> 
> 
> Diffs
> -----
> 
>   docs/logging.md 3b36870c22336440b0d7bf1359e1d0a97986a0f6 
>   src/slave/container_loggers/lib_logrotate.hpp f216548ef37f5c2245ef64d21e84e06100e8e5ae

>   src/slave/container_loggers/lib_logrotate.cpp 01552752a56ee7377a631a783f2168ba0eea2799

>   src/tests/container_logger_tests.cpp e8f934106510fe02b8b92be19c918a1e5c0b78fd 
> 
> Diff: https://reviews.apache.org/r/51565/diff/
> 
> 
> Testing
> -------
> 
> Previewed documentation change via the website previewer.
> 
> make check
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>


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