Eager validation of configuration parameters

[brenuart]

Configuration parameters should be validated eagerly by the setter method and an IllegalArgumentException thrown in case of failure. This PR targets the parameters that do not follow this strategy + some additional test cases to cover missing cases.

Reviewing the code raises the following additional questions:

(1) AbstractLogstashTcpSocketAppender#setWriteTimeout(Duration) accepts null as argument but converts it to the default value instead. I personally don't like this approach and prefer when "get" behaves the opposite of "set", i.e. returns the same value. The javadoc does not explicitly state that null is a valid value (0 should be used to disable the feature)... I think we should simply refuse it and throw an exception. If null must remain a valid option, then I would rather handle the case within isWriteTimeoutEnabled() just like what is done for keepAliveDuration.

(2) AbstractLogstashTcpSocketAppender#setSecondaryConnectionTTL() should probably throw an IllegalArgumentException instead of IllegalStateException just like the other setter methods.

(3) The default keep alive message uses the platform line ending. However, recipient like Elastic Logstash use \n by default (Unix style). Don't you think we should use the same default as well?

(4) AsyncDisruptorAppender uses ringBufferSize but AbstractLogstashTcpSocketAppender also declares queueSize as an alias for the same property. Don't you think we should stick to one and deprecate the other?

(5) The javadoc describing each parameter is often declared on the field itself rather than the setter. Shouldn't we at least have a "user friendly" version on the setter and keep a "developer" version on the field?

(6) I think it would be better to rename AbstractLogstashTcpSocketAppender#acceptConnectionTimeout to connectTimeout to best represent the actual use of the property (and deprecate the old name).

Note: don't merge this PR yet - I'd like to keep it open to discuss other additional points with you...

[philsttr]

(1) AbstractLogstashTcpSocketAppender#setWriteTimeout(Duration) accepts null as argument but converts it to the default value instead. I personally don't like this approach and prefer when "get" behaves the opposite of "set", i.e. returns the same value. The javadoc does not explicitly state that null is a valid value (0 should be used to disable the feature)... I think we should simply refuse it and throw an exception. If null must remain a valid option, then I would rather handle the case within isWriteTimeoutEnabled() just like what is done for keepAliveDuration.

I'm ok with changing it to throw if null in this major release, but this will need to be mentioned in the backwards incompatibilities in the release notes. Create a separate PR if you change this.

(2) AbstractLogstashTcpSocketAppender#setSecondaryConnectionTTL() should probably throw an IllegalArgumentException instead of IllegalStateException just like the other setter methods.

I think IllegalStateException was used because it indicates that the value is only invalid due to the current state of the object (i.e. the same value could be valid if the object was in a different state). Whereas IllegalArgumentException means the given argument is never valid, regardless of the object state.

(3) The default keep alive message uses the platform line ending. However, recipient like Elastic Logstash use \n by default (Unix style). Don't you think we should use the same default as well?

I'm ok with changing the default since this is a major release, but it will need to be mentioned in backwards incompatibilities section. Create a separate PR if you change this.

(4) AsyncDisruptorAppender uses ringBufferSize but AbstractLogstashTcpSocketAppender also declares queueSize as an alias for the same property. Don't you think we should stick to one and deprecate the other?

queueSize is leftover from before the appender used the disruptor. I'm fine with marking as deprecated.

The unfortunate thing about deprecating parameters is that the user gets no indication of deprecation (like via javac or ide), since these parameters are usually set via xml configuration. We could add a warning to provide more visibility.

(5) The javadoc describing each parameter is often declared on the field itself rather than the setter. Shouldn't we at least have a "user friendly" version on the setter and keep a "developer" version on the field?

Sure, feel free to update.

(6) I think it would be better to rename AbstractLogstashTcpSocketAppender#acceptConnectionTimeout to connectTimeout to best represent the actual use of the property (and deprecate the old name).

Feel free to deprecate, and add a warning for visibility.

[philsttr]

All changes so far look great!

[brenuart]

I created additional issues (#605, #606 and #607) to track changes that should be covered by separate PRs.

[brenuart]

(2) AbstractLogstashTcpSocketAppender#setSecondaryConnectionTTL() should probably throw an IllegalArgumentException instead of IllegalStateException just like the other setter methods.

I think IllegalStateException was used because it indicates that the value is only invalid due to the current state of the object (i.e. the same value could be valid if the object was in a different state). Whereas IllegalArgumentException means the given argument is never valid, regardless of the object state.

I see what you mean except this check relies on the order properties are set - it won't work as expected when secondaryConnectionTTL appears before connectionStrategy... According to me this case should be handled in the start method when all properties have been set and the overall configuration can be safely asserted.

According to me this property should not be repeated inside the AbstractLogstashTcpSocketAppender and should only be configurable from the PreferPrimaryDestinationConnectionStrategy as exposed in the README:

      <connectionStrategy>
          <preferPrimary>
              <secondaryConnectionTTL>5 minutes</secondaryConnectionTTL>
          </preferPrimary>
      </connectionStrategy>

I propose to remove it from AbstractLogstashTcpSocketAppender without deprecation at all - not even a warning - and add an entry for it in the "breaking changes" section of the release note. People using the XML configuration will immediately get an error saying the property does not exist while other with programatic configuration will get a compilation error.

With this change, configuration of the "primary" strategy will follow the same principles as others like the "roundrobin" strategy for instance.

[brenuart]

queueSize is leftover from before the appender used the disruptor. I'm fine with marking as deprecated.

queueSize seems more appropriate than ringBufferSize... The latter refers to implementation details (the ring buffer). I rather standardise all configurations around queueSize and "deprecate" the other...

[philsttr]

Since the class name is AsyncDisruptorAppender, and the disruptor uses a ring buffer, not a queue, I think ringBufferSize is more appropriate. It also implicitly explains the reason why the value must be a power of 2, since that is a constraint of the disruptor ring buffer.

[philsttr]

According to me this property should not be repeated inside the AbstractLogstashTcpSocketAppender and should only be configurable from the PreferPrimaryDestinationConnectionStrategy as exposed in the README:

It's in AbstractLogstashTcpSocketAppender for backwards compatibility purposes from before the connection strategies were implemented in #195 in 4.10. Previously (< 4.10) the concept of a connection strategy did not exist, and secondaryConnectionTTL was a just property on the appender. In 4.10, connection strategies were implemented, and the property remained on the appender for backwards compatibility.

I propose to remove it from AbstractLogstashTcpSocketAppender without deprecation at all - not even a warning - and add an entry for it in the "breaking changes" section of the release note. People using the XML configuration will immediately get an error saying the property does not exist while other with programatic configuration will get a compilation error.

I disagree with removing without deprecating first. For better usability, logstash-logback-encoder should always strive to deprecate first before removal. Not everyone will notice the deprecation warning, but some people will, and it provides a better experience for them.

With this change, configuration of the "primary" strategy will follow the same principles as others like the "roundrobin" strategy for instance.

I agree with the change, as long as the existing behavior is kept in a deprecated fashion until the next major release (8.0)