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 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
