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