-
Notifications
You must be signed in to change notification settings - Fork 92
deprecate setIdleTimeout and force it to do the same as setKeepAliveTimeSeconds #79
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Sammers21 have you done a quick test to check that the problem is gone. It's clear but just to be sure.
@ppatierno, just done. |
*/ | ||
@Deprecated | ||
@Override | ||
public NetClientOptions setIdleTimeout(int idleTimeout) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the return type has to be MqttClientOptions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
@@ -82,6 +83,7 @@ | |||
private static final int MIN_TOPIC_LEN = 1; | |||
private static final String PROTOCOL_NAME = "MQTT"; | |||
private static final int PROTOCOL_VERSION = 4; | |||
private static final int DEFAULT_INTERNAL_NET_CLIENT_IDLE_TIMEOUT = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that DEFAULT_IDLE_TIMEOUT
is enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree
@@ -59,9 +59,11 @@ public void connectDisconnect(TestContext context) throws InterruptedException { | |||
} | |||
|
|||
@Test | |||
public void connectDisconnectNoOptions(TestContext context) { | |||
public void connectDisconnectWithIdleOption(TestContext context) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you change the connectDisconnectNoOptions
already in place ? Please create a new test for this fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The funny thing is that connectDisconnectNoOptions is a full duplicate of connectDisconnect so i have decided to modify it instead of removing as duplicate :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah !! You are right :-) So it's ok !!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Sammers21 can you squash the commits ... then I'll merge the PR ! Thanks !
88f4136
to
4b30352
Compare
squashed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM ! Thanks !
A solution for #78