Skip to content
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

Close ping handler's executor service properly #7903

Conversation

Projects
None yet
4 participants
@martijnvg
Copy link
Member

martijnvg commented Sep 26, 2014

For each ping collect phase a send ping listener with a executor service is created.
If there in flight ping rounds while a node shutdowns this executor service isn't shutdown correctly leaving threads behind.

This PR keeps track of this send ping handlers and closes the executor service. Also the thread pool is now final in SendPingsHandler. There was a possibility that multiple executor service could be created if during the close call the variable was set to null and a ping was still going on.

@s1monw

View changes

src/main/java/org/elasticsearch/discovery/zen/ping/unicast/UnicastZenPing.java Outdated
receivedResponses.put(sendPingsHandler.id(), sendPingsHandler);
try {
sendPings(timeout, null, sendPingsHandler);
threadPool.schedule(TimeValue.timeValueMillis(timeout.millis() / 2), ThreadPool.Names.GENERIC, new Runnable() {

This comment has been minimized.

Copy link
@s1monw

s1monw Sep 26, 2014

Contributor

you can use AbstractRunnable here now where you can implement onFailure that is called even on rejection etc. maybe that makes it easier?

@s1monw

View changes

src/main/java/org/elasticsearch/discovery/zen/ping/unicast/UnicastZenPing.java Outdated
private final Set<DiscoveryNode> nodeToDisconnect = ConcurrentCollections.newConcurrentSet();
private final PingCollection pingCollection;

private volatile boolean closed;

This comment has been minimized.

Copy link
@s1monw

s1monw Sep 26, 2014

Contributor

maybe make this a private final AtomicBoolean closed and in close you can then protect double closing by

public void close() {
  if (closed.compareAndSet(false, true)) {
    //do the things
  }
}
@s1monw

View changes

src/main/java/org/elasticsearch/discovery/zen/ping/unicast/UnicastZenPing.java Outdated
@@ -141,6 +140,9 @@ protected void doStop() throws ElasticsearchException {
@Override
protected void doClose() throws ElasticsearchException {
transportService.removeHandler(ACTION_NAME);
for (SendPingsHandler sendPingsHandler : receivedResponses.values()) {

This comment has been minimized.

Copy link
@s1monw

s1monw Sep 26, 2014

Contributor

make SendPingHandler implement Closeable and then use IOUtils.close(receivedResponses.values())

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Sep 26, 2014

left some comments

@@ -315,7 +323,7 @@ void sendPings(final TimeValue timeout, @Nullable TimeValue waitTime, final Send
}
// fork the connection to another thread
final DiscoveryNode finalNodeToSend = nodeToSend;
sendPingsHandler.executor().execute(new Runnable() {
unicastConnectExecutor.execute(new Runnable() {

This comment has been minimized.

Copy link
@bleskes

bleskes Sep 26, 2014

Member

We should deal with rejections if the executor is closed.

This comment has been minimized.

Copy link
@martijnvg

martijnvg Sep 26, 2014

Author Member

We deal with up higher up the stack (in #ping)

@@ -141,6 +145,7 @@ protected void doStop() throws ElasticsearchException {
@Override
protected void doClose() throws ElasticsearchException {
transportService.removeHandler(ACTION_NAME);
ThreadPool.terminate(unicastConnectExecutor, 0, TimeUnit.SECONDS);

This comment has been minimized.

Copy link
@bleskes

bleskes Sep 26, 2014

Member

Should we also close all "open" ping handlers?

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Sep 26, 2014

LGTM @bleskes your call

@bleskes

View changes

src/main/java/org/elasticsearch/discovery/zen/ping/unicast/UnicastZenPing.java Outdated
sendPings(timeout, null, sendPingsHandler);
} catch (RejectedExecutionException e) {
logger.debug("Ping execution rejected", e);
}

This comment has been minimized.

Copy link
@bleskes

bleskes Sep 26, 2014

Member

we should probably bail here. One nit pick - I would prefer having this rejection logic closer to where it holds. I think there is only one method that can cause this.

This comment has been minimized.

Copy link
@bleskes

bleskes Sep 26, 2014

Member

ignore the nit pick - I see how it helps in error handling on other places. Maybe a comment saying where this can come from?

@bleskes

View changes

src/main/java/org/elasticsearch/discovery/zen/ping/unicast/UnicastZenPing.java Outdated
PingCollection responses = receivedResponses.get(response.id);
if (responses == null) {
SendPingsHandler sendPingsHandler = receivedResponses.get(response.id);
if (sendPingsHandler == null) {
logger.warn("received ping response {} with no matching handler id [{}]", pingResponse, response.id);

This comment has been minimized.

Copy link
@bleskes

bleskes Sep 26, 2014

Member

with the new close logic, I'm worried about unneeded warn messages in the logs during shutdown. I wonder if we should add a global close flag to the entire class that can be used to suppress this.

@bleskes

This comment has been minimized.

Copy link
Member

bleskes commented Sep 26, 2014

If left two little comments. o.w. LGTM

@martijnvg martijnvg force-pushed the martijnvg:bugs/unicast_send_ping_handler_thread_pool_proper_cleanup branch 2 times, most recently Sep 26, 2014

If a node is being shutdown some in flight ping request may be execut…
…ed. Make sure to keep track of those ping requests and close the unicast connect executor service.

Closes #7903

@martijnvg martijnvg force-pushed the martijnvg:bugs/unicast_send_ping_handler_thread_pool_proper_cleanup branch to 71adb3a Sep 26, 2014

martijnvg added a commit to martijnvg/elasticsearch that referenced this pull request Sep 26, 2014

If a node is being shutdown some in flight ping request may be execut…
…ed. Make sure to keep track of those ping requests and close the unicast connect executor service.

Closes elastic#7903

@martijnvg martijnvg merged commit 71adb3a into elastic:master Sep 26, 2014

martijnvg added a commit to martijnvg/elasticsearch that referenced this pull request Sep 26, 2014

If a node is being shutdown some in flight ping request may be execut…
…ed. Make sure to keep track of those ping requests and close the unicast connect executor service.

Closes elastic#7903

@spinscale spinscale changed the title Close ping handler's executor service properly Discovery: Close ping handler's executor service properly Oct 1, 2014

@martijnvg martijnvg deleted the martijnvg:bugs/unicast_send_ping_handler_thread_pool_proper_cleanup branch May 18, 2015

@clintongormley clintongormley changed the title Discovery: Close ping handler's executor service properly Close ping handler's executor service properly Jun 6, 2015

mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015

If a node is being shutdown some in flight ping request may be execut…
…ed. Make sure to keep track of those ping requests and close the unicast connect executor service.

Closes elastic#7903
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.