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.

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

Jump to bottom

Make jetty scheduler threads daemon thread #1637

Merged
merged 1 commit into from
Sep 2, 2015
Merged

Make jetty scheduler threads daemon thread #1637

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 5 additions & 0 deletions server/src/main/java/io/druid/server/initialization/jetty/JettyServerModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
import org.eclipse.jetty.server.Server;
import org.eclipse.jetty.server.ServerConnector;
import org.eclipse.jetty.util.thread.QueuedThreadPool;
import org.eclipse.jetty.util.thread.ScheduledExecutorScheduler;

import javax.servlet.ServletException;
import java.util.Map;
Expand Down Expand Up @@ -161,6 +162,10 @@ private static Server makeJettyServer(@Self DruidNode node, ServerConfig config)

final Server server = new Server(threadPool);

// Without this bean set, the default ScheduledExecutorScheduler runs as non-daemon, causing lifecycle hooks to fail
// to fire on main exit. Related bug: https://github.com/druid-io/druid/pull/1627
server.addBean(new ScheduledExecutorScheduler("JettyScheduler", true), true);
Copy link
Contributor

Choose a reason for hiding this comment

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

does server.stop() not terminate these threads?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@himanshug If the stop is called then yes. The problem is that if you let io.druid.cli.Main naturally exit, then the lifecycle.stop which calls server.stop is never called because this is a daemon thread. With this patch, a clean exit out of Main can be accomplished because the jetty scheduler is not holding up the shutdown hook

Copy link
Contributor

Choose a reason for hiding this comment

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

looks like I'm missing something, but dint we decide to call lifecycle.stop() explicitly and via shutdown hook in cases where Main is expected to exit voluntarily (which is in case of CliPeon only for now) ? In other CliXXX classes, shutdown hook is always registered and they can only exit on a signal which will call the shutdown hook?

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. This patch eliminates the need to explicitly call for all known cases. As Eric mentioned, in general it should be harder to create non daemon threads than daemon threads. This PR is working towards that and a world where all Druid tasks can shutdown cleanly by simply returning from Main

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Stopping the life cycle will still be done explicitly until there are at least better integration tests in place

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM for me if that is the conclusion


ServerConnector connector = new ServerConnector(server);
connector.setPort(node.getPort());
connector.setIdleTimeout(Ints.checkedCast(config.getMaxIdleTime().toStandardDuration().getMillis()));
Expand Down