----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51565/#review147765 -----------------------------------------------------------
src/slave/container_loggers/lib_logrotate.hpp (line 42) <https://reviews.apache.org/r/51565/#comment215014> s/Options/Flags/ ? Also, can you add a comment for why you factored these out into a separate struct? src/slave/container_loggers/lib_logrotate.hpp (line 102) <https://reviews.apache.org/r/51565/#comment215016> I wonder if we could better `Flags` and `LoggerOptions` to make it clear how the separation was made. I originally thought `AgentFlags` and `ExecutorFlags` respectively but that's confusing. src/slave/container_loggers/lib_logrotate.hpp (line 111) <https://reviews.apache.org/r/51565/#comment215013> does it not look into tasks's command info as well for command tasks? src/slave/container_loggers/lib_logrotate.cpp (line 118) <https://reviews.apache.org/r/51565/#comment215003> can this not be moved to the line above? src/slave/container_loggers/lib_logrotate.cpp (line 142) <https://reviews.apache.org/r/51565/#comment215002> why only first message? i would just log the warnings for now, like we do everywhere else. warnings are expected for deprecated flag names. src/tests/container_logger_tests.cpp (line 524) <https://reviews.apache.org/r/51565/#comment215004> new line. src/tests/container_logger_tests.cpp (line 530) <https://reviews.apache.org/r/51565/#comment215005> new line. src/tests/container_logger_tests.cpp (line 588) <https://reviews.apache.org/r/51565/#comment215010> hmm. "MESOS_LOGROTATE_LOGGER_LOGROTATE_" looks weird. - Vinod Kone On Aug. 31, 2016, 10:23 p.m., Joseph Wu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51565/ > ----------------------------------------------------------- > > (Updated Aug. 31, 2016, 10:23 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 > >
