On Monday, September 8, 2014, Leif Hedstrom <zw...@apache.org <javascript:_e(%7B%7D,'cvml','zw...@apache.org');>> wrote:
> > On Sep 8, 2014, at 9:43 PM, James Peach <jpe...@apache.org> wrote: > > > On Sep 5, 2014, at 2:59 PM, Brian Rectanus <brect...@gmail.com> wrote: > > > >> All, > >> > >> I have a pull request in that is a trivial patch to add a long missing > >> TSTextLogObjectRollingSizeMbSet(int rolling_size_mb) API function to > allow > >> setting the rolling size of custom log objects. This API just wraps the > >> existing TextLogObject::set_rolling_size_mb(int rolling_size_mb) which > is > >> used internally to set the log rolling size as you can through > >> records.config (proxy.config.log.rolling_size_mb) and in the > >> logs_xml.config (<RollingSizeMb = "size_in_MB"/>). All other rolling > >> functions are already exposed in the API, but this one was missing and I > >> have need to use it in the IronBee project (github.com/ironbee) for our > >> custom log. There should be no ABI or compatibility issues as this is > just > >> the addition of a function. Look forward to your comments. > >> > >> This is TS-3059: https://issues.apache.org/jira/browse/TS-3059 > >> Pull request #107: https://github.com/apache/trafficserver/pull/107 > >> Patch: https://github.com/apache/trafficserver/pull/107.patch > >> > >> Simple addition of: > >> > >> /** > >> Set the rolling size. rolling_size_mb specifies the size in MB when > >> log rolling > >> should take place. > >> */ > >> tsapi void TSTextLogObjectRollingSizeMbSet(TSTextLogObject the_object, > int > >> rolling_size_mb); > > > > According to LogObject::_setup_rolling(), the minimum for > rolling_size_mb is 10. I think that this API should return TSReturnCode, > and fail if "the_object" is NULL, or "rolling_size_mb" is less than the > minimum. Otherwise, this looks fine to me. > > > +1. > > — Leif Agreed, all these APIs should have done this for values out of range. However, the other APIs in this group do not, so having this API return an error is odd when not doing so for time rotation and offset hour rotation as well. Changing the other APIs seems resonable, but it does mean an ABI change. I feel fixing the API is a separate issue out of scope to adding a missing function. I can do this in a separate issue if you wish, though. Additionally, the value checks are done by the LogObject functions. They are internal values, so I would be hard coding the checks in two places (api and LogObject). Better would be to alter LogObject functions to return error and then pass this through the API. That, though, I think is really out of scope for what I am proposing. Note the system does log an error already if you set a value out of range and it reverts to the minimum, so not a huge problem. There is even a comment stating this is fine. Cheers! -B -- Brian Rectanus
