On Sep 9, 2014, at 6:28 AM, Brian Rectanus <brect...@gmail.com> wrote:
> 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. Both of these end up calling LogObject::_setup_rolling(), which does various adjustments. LogObject::set_rolling_size_mb() does not do this, though it might be reasonable to change that. > 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. I'm OK either way. If you update the behavior of TSTextLogObjectRollingSizeMbSet() to match the rest of the API, that's probably the least disruptive option. > > 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. Maybe I'm not looking in the right place, but I don't see where that happens. Can you point it out? thanks,
