On Tue, Sep 9, 2014 at 10:42 AM, James Peach <jpe...@apache.org> wrote:
> > 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. > All I meant was that _setup_rolling() could return an error and this could pass back through set_rolling_size_mb() and then through the API if these were all changed to return an error. However, I did not want to mess with any of that - I did not even touch the LogObject.cc file, just added the missing API wrapper to allow this to be exposed like the other existing APIs. For some reason this one that I added for rolling size was just not exposed like the others. > > > 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. > That is the way it is now in the pull request. It matches the other APIs for RollingIntervalSec and RollingOffsetHr. I'd rather leave it this way as it is least disruptive. /** Set the rolling interval. */ tsapi void TSTextLogObjectRollingIntervalSecSet(TSTextLogObject the_object, int rolling_interval_sec); /** Set the rolling offset. rolling_offset_hr specifies the hour (between 0 and 23) when log rolling should take place. */ tsapi void TSTextLogObjectRollingOffsetHrSet(TSTextLogObject the_object, int rolling_offset_hr); I just added this additional one, also returning void: /** 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); > > > > > 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, LogObject.cc:704 in Note() You get something like this in diags.log: Sep 15 07:28:48.074] Server {0x2b100a441f80} NOTE: <LogObject.cc:704 (_setup_rolling)> Rolling size invalid(3) for /opt/qualys/var/log/trafficserver/txlogs/tx-ironbee.log, setting it to 10 MB Cheers! -B
