Skip to content

Deprecate AbstractLogstashTcpSocketAppender#secondaryConnectionTTL #634

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 7, 2021
Merged

Deprecate AbstractLogstashTcpSocketAppender#secondaryConnectionTTL #634

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4fdda7f
Deprecate AbstractLogstashTcpSocketAppender#secondaryConnectionTTL
brenuart Sep 6, 2021
454388d
Review deprecated warning message emitted by #setSecondaryConnectionTTL
brenuart Sep 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion src/main/java/net/logstash/logback/appender/AbstractLogstashTcpSocketAppender.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1173,15 +1173,30 @@ public Duration getReconnectionDelay() {
* @see PreferPrimaryDestinationConnectionStrategy#setSecondaryConnectionTTL(Duration)
* @param secondaryConnectionTTL the TTL of a connection when connected to a secondary destination
* @throws IllegalStateException if the {@link #connectionStrategy} is not a {@link PreferPrimaryDestinationConnectionStrategy}
*
* @deprecated use {@link PreferPrimaryDestinationConnectionStrategy#setSecondaryConnectionTTL(Duration)} instead.
*/
@Deprecated
public void setSecondaryConnectionTTL(Duration secondaryConnectionTTL) {
addWarn("Setting <secondaryConnectionTTL> on the appender is deprecated, set it on the connection strategy using <preferPrimary.secondaryConnectionTTL> instead.");
Copy link
Collaborator

@philsttr philsttr Sep 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<preferPrimary.secondaryConnectionTTL> is a little confusing, since it looks like xml, but is not the way the value would be set in xml.

Perhaps <preferPrimary><secondaryConnectionTTL> would be better? I'm looking for better ideas. (?)

Copy link
Collaborator Author

@brenuart brenuart Sep 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I came across this issue several times already when throwing exceptions for invalid properties for instance.

I initially though that using an XML-like name was a good idea certainly if we consider that most users will use an XML file to configure their logging system. If we follow this approach then <preferPrimary><secondaryConnectionTTL> is certainly better.

Another option is to opt for a JavaBean-style format and do something like "preferPrimary.secondaryConnectionTTL". But this looks weird to me...

In this particular case, maybe we could simply say Setting <secondaryConnectionTTL> directly on the appender is deprecated. Instead you should explicitly set the connection strategy to <preferPrimary> and set its <secondaryConnectionTTL> property to the desired value.

What ever option we choose, we should probably review the other classes and standardise on the chosen naming convention...

Copy link
Collaborator

@philsttr philsttr Sep 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting <secondaryConnectionTTL> directly on the appender is deprecated. Instead you should explicitly set the connection strategy to <preferPrimary> and set its <secondaryConnectionTTL> property to the desired value.

^ I like that.

What ever option we choose, we should probably review the other classes and standardise on the chosen naming convention

Agreed.


if (connectionStrategy instanceof PreferPrimaryDestinationConnectionStrategy) {
((PreferPrimaryDestinationConnectionStrategy) connectionStrategy).setSecondaryConnectionTTL(secondaryConnectionTTL);
} else {
throw new IllegalStateException(String.format("When setting the secondaryConnectionTTL, the strategy must be a %s. It is currently a %s", PreferPrimaryDestinationConnectionStrategy.class, connectionStrategy));
}
}

/**
* Convenience method for accessing {@link PreferPrimaryDestinationConnectionStrategy#getSecondaryConnectionTTL()}.
*
* @return the secondary connection TTL or {@code null} if the connection strategy is not a {@link PreferPrimaryDestinationConnectionStrategy}.
* @deprecated use {@link PreferPrimaryDestinationConnectionStrategy#getSecondaryConnectionTTL()} instead.
*
* @see #getConnectionStrategy()
* @see PreferPrimaryDestinationConnectionStrategy#getSecondaryConnectionTTL()
*/
@Deprecated
public Duration getSecondaryConnectionTTL() {
if (connectionStrategy instanceof PreferPrimaryDestinationConnectionStrategy) {
return ((PreferPrimaryDestinationConnectionStrategy) connectionStrategy).getSecondaryConnectionTTL();
Expand Down Expand Up @@ -1237,7 +1252,7 @@ public void setWriteBufferSize(int writeBufferSize) {
* Alias for {@link #getRingBufferSize()}.
*
* @return the size of the ring buffer
* @deprecated use {@link #getRingBufferSize()} instead
* @deprecated use {@link #getRingBufferSize()} instead.
*/
@Deprecated
public int getQueueSize() {
Expand Down
10 changes: 8 additions & 2 deletions src/test/java/net/logstash/logback/appender/LogstashTcpSocketAppenderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@
import ch.qos.logback.classic.spi.ILoggingEvent;
import ch.qos.logback.core.BasicStatusManager;
import ch.qos.logback.core.encoder.Encoder;
import ch.qos.logback.core.status.OnConsoleStatusListener;
import ch.qos.logback.core.status.Status;
import ch.qos.logback.core.status.StatusManager;
import ch.qos.logback.core.util.Duration;
Expand All @@ -84,8 +85,6 @@ public class LogstashTcpSocketAppenderTest {
@InjectMocks
private final LogstashTcpSocketAppender appender = new TestableLogstashTcpSocketAppender();

private LoggerContext context = new LoggerContext();

private StatusManager statusManager = new BasicStatusManager();

@Mock
Expand Down Expand Up @@ -121,6 +120,13 @@ protected Future<?> scheduleReaderCallable(Callable<Void> readerCallable) {

@BeforeEach
public void setup() throws IOException {
// Output statuses on the console for easy debugging. Must be initialized early to capture
// warnings emitted by setter/getter methods before the appender is started.
OnConsoleStatusListener consoleListener = new OnConsoleStatusListener();
consoleListener.start();
statusManager.add(consoleListener);

LoggerContext context = new LoggerContext();
context.setStatusManager(statusManager);

when(socketFactory.createSocket()).thenReturn(socket);
Expand Down