On Sep 19, 2014, at 10:33 AM, Brian Rectanus <brect...@gmail.com> wrote:
> This is a followup on the API review I posted earlier. I got quite a bit > of feedback (thanks for that) and want to summarize and verify I understood > comments correctly. I don't think there were any -1s for the existing pull > request, but just some recommendations on how this could be improved. > > 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 the following to wrap the existing > LogObject:set_rolling_size_mb() > similar to existing TSTextLogObjectRollingIntervalSecSet() and > TSTextLogObjectRollingOffsetHrSet() APIs. > > /** > 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); > > > There were two main comments: > > * The "int" parameter is signed and perhaps should be unsigned. > > The other APIs in the same family all use int as well as the underlying > LogObject. To keep this consistent this needs to remain an int. Validation > is done in the underlying LogObject. > > * The API should return TSReturnCode if the values violate validation. > > The underlying LogObject does not return errors, but instead just modifies > the values to be valid. For example if you pass a negative value, it will > not be in range and be adjusted to minimal value. This generates a Note in > diags.log similar to: > > NOTE: <LogObject.cc:704 (_setup_rolling)> Rolling size invalid(-1) for > /path/to/my.log, setting it to 10 MB This is all fine, I'll pull the patch later today. My only reservation is that LogObject::set_rolling_size_mb doen't call LogObject:_setup_rolling, so you only clip it to a valid size *after* some other logging operation. We can fix that after landing this. > > Additionally the other APIs in this family ( > TSTextLogObjectRollingIntervalSecSet() > and TSTextLogObjectRollingOffsetHrSet() ) also do not return errors. While > I agree they all *should* return errors, I do not want to introduce > inconsistencies for this new API and also do not want to break ABI. I'd > like to get this into 5.1.1 if possible and changing the other APIs to make > this consistent would prevent this. > > How I'd like to move forward: > > * Apply the pull request as-is for master and backport to 5.1.1. The > existing pull request does not seem to violate any inconsistencies with the > other APIs in this family and does not introduce any ABI issues. > > * In the future (6.x?), look at updating the underlying LogObject functions > to return errors on validation issues as this will break ABI. As I am not > sure what this may affect at this point, I am not sure if this is a good > idea or not, but worth a look. > > Cheers! > -B > > -- > Brian Rectanus
