[API REVIEW FOLLOWUP] Adding missing TSTextLogObjectRollingSizeMbSet() function

[Brian Rectanus <br...@gmail.com>]

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

Re: [API REVIEW FOLLOWUP] Adding missing TSTextLogObjectRollingSizeMbSet() function

[James Peach <jp...@apache.org>]

On Sep 19, 2014, at 10:33 AM, Brian Rectanus <br...@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