Skip to content

Conversation

@sasbury
Copy link
Contributor

@sasbury sasbury commented Sep 6, 2018

Support for Drain on the connection and Subscription.
Fixed a null pointer in certain situations with ping/reconnect
Added more JDKs to travis build

Bumping version number for added drain API.

Stephen Asbury added 24 commits August 15, 2018 14:23
Subscriptions and Dispatchers can drain.

Connections can drain.

Added several tests for main and exceptional cases.

Fixed #170
Only upload snapshot from oracle jdk 8.
Updated dependency for NKey to implementation
Added support for drain in message queue
Added callback after unsub/flush to mark message queue
Improved flush timeout behavior
Added timeout to drain if unsub/flush times out
Added several tests for drain and flush
Removed support for R/R during drain
Converted natsbench to throttling - it was failing
More doc
fixed wrong exception from get private key
…gs on reconnect.

This is hard to test but likely happens when pings are sent during a slower reconnect.
Added a check for the null pointer.
Added a check to not ping the server if not connected (in the ping timer).

Fixes #172
Updated readme
Fixed issue in travis
Updated changelog
* messages have reached the server.
* Finally the connection is closed.
*
* In order to drain subscribers, an unsub is sent to the server followed by a flush.
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick. Maybe say UNSUB protocol message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

* were drained in the timeout, and false otherwise. The future is completed after the connection
* is closed, so any connection handler notifications will happen before the future completes.
*
* @param timeout The time to wait for the drain to succeed, pass 0 to wait
Copy link
Member

Choose a reason for hiding this comment

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

In other languages we use -1 for "forever".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since we don't support 0 time, i think this is ok. Also, changing would require a major revision bump.

try {
reply = incoming.get(timeout.toNanos(), TimeUnit.NANOSECONDS);
} catch (ExecutionException|TimeoutException e) {
} catch (ExecutionException | TimeoutException e) {
Copy link
Member

@ColinSullivan1 ColinSullivan1 Sep 6, 2018

Choose a reason for hiding this comment

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

Did you intend to leave interrupted and cancellation exceptions uncaught?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, interrupted exceptions should rarely be eaten as they tell the caller the thread was interrupted. Cancellation has the same meaning here for the future, so we don't want to eat it.

Copy link
Member

@ColinSullivan1 ColinSullivan1 left a comment

Choose a reason for hiding this comment

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

LGTM.

@sasbury sasbury merged commit 64ebc2a into master Sep 6, 2018
@sasbury sasbury deleted the v2.1.0 branch September 7, 2018 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants