Skip to content

Make CliPeon run shutdown hooks properly #1524

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
fjy merged 1 commit into from
Jul 15, 2015
Merged

Make CliPeon run shutdown hooks properly #1524

Show file tree
Hide file tree
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
1 change: 1 addition & 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 @@ -157,6 +157,7 @@ private static Server makeJettyServer(@Self DruidNode node, ServerConfig config)
final QueuedThreadPool threadPool = new QueuedThreadPool();
threadPool.setMinThreads(config.getNumThreads());
threadPool.setMaxThreads(config.getNumThreads());
threadPool.setDaemon(true);

final Server server = new Server(threadPool);

Expand Down
29 changes: 21 additions & 8 deletions services/src/main/java/io/druid/cli/CliPeon.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
import io.airlift.command.Arguments;
import io.airlift.command.Command;
import io.airlift.command.Option;
import io.druid.metadata.IndexerSQLMetadataStorageCoordinator;
import io.druid.guice.Binders;
import io.druid.guice.IndexingServiceFirehoseModule;
import io.druid.guice.Jerseys;
Expand All @@ -56,6 +55,7 @@
import io.druid.indexing.overlord.ThreadPoolTaskRunner;
import io.druid.indexing.worker.executor.ExecutorLifecycle;
import io.druid.indexing.worker.executor.ExecutorLifecycleConfig;
import io.druid.metadata.IndexerSQLMetadataStorageCoordinator;
import io.druid.query.QuerySegmentWalker;
import io.druid.segment.loading.DataSegmentArchiver;
import io.druid.segment.loading.DataSegmentKiller;
Expand All @@ -71,7 +71,6 @@
import io.druid.segment.realtime.firehose.ServiceAnnouncingChatHandlerProvider;
import io.druid.server.QueryResource;
import io.druid.server.initialization.jetty.JettyServerInitializer;

import org.eclipse.jetty.server.Server;

import java.io.File;
Expand Down Expand Up @@ -125,7 +124,8 @@ public void configure(Binder binder)
.to(ServiceAnnouncingChatHandlerProvider.class).in(LazySingleton.class);
handlerProviderBinder.addBinding("noop")
.to(NoopChatHandlerProvider.class).in(LazySingleton.class);
binder.bind(ServiceAnnouncingChatHandlerProvider.class).in(LazySingleton.class);;
binder.bind(ServiceAnnouncingChatHandlerProvider.class).in(LazySingleton.class);

binder.bind(NoopChatHandlerProvider.class).in(LazySingleton.class);

binder.bind(TaskToolboxFactory.class).in(LazySingleton.class);
Expand Down Expand Up @@ -190,7 +190,9 @@ private void configureTaskActionClient(Binder binder)
JsonConfigProvider.bind(binder, "druid.indexer.storage", TaskStorageConfig.class);
binder.bind(TaskStorage.class).to(HeapMemoryTaskStorage.class).in(LazySingleton.class);
binder.bind(TaskActionToolbox.class).in(LazySingleton.class);
binder.bind(IndexerMetadataStorageCoordinator.class).to(IndexerSQLMetadataStorageCoordinator.class).in(LazySingleton.class);
binder.bind(IndexerMetadataStorageCoordinator.class).to(IndexerSQLMetadataStorageCoordinator.class).in(
LazySingleton.class
);
taskActionBinder.addBinding("remote")
.to(RemoteTaskActionClientFactory.class).in(LazySingleton.class);

Expand All @@ -205,17 +207,28 @@ public void run()
{
try {
Injector injector = makeInjector();

try {
Lifecycle lifecycle = initLifecycle(injector);

final Lifecycle lifecycle = initLifecycle(injector);
Runtime.getRuntime().addShutdownHook(
new Thread(
Copy link
Copy Markdown
Contributor

@gianm gianm Jul 14, 2015

Choose a reason for hiding this comment

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

I wonder if all of our regular task types are good about having every thread be non-daemon. If not, then I think they won't be able to shut down normally if you take out this lifecycle.stop().

Copy link
Copy Markdown
Contributor

@gianm gianm Jul 14, 2015

Choose a reason for hiding this comment

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

As in, many of the tasks spawn a bunch of threads internally.

Copy link
Copy Markdown
Member

@xvrl xvrl Jul 14, 2015

Choose a reason for hiding this comment

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

@gianm are you saying we should have both the shutdown hook and the lifecycle.stop() after the lifecycle.join()?

Copy link
Copy Markdown
Contributor

@gianm gianm Jul 14, 2015

Choose a reason for hiding this comment

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

I'm saying I'm not sure if it's needed or not, it depends on how good the existing tasks are about thread cleanliness on exit. If they're doing things right then the lifecycle.stop() is redundant to the shutdown hook.

@drcrallen suggested taking it out and letting the integration tests catch potential problems, which seems fair.

new Runnable()
{
@Override
public void run()
{
log.info("Running shutdown hook");
lifecycle.stop();
}
}
)
);
injector.getInstance(ExecutorLifecycle.class).join();
lifecycle.stop();
}
catch (Throwable t) {
log.error(t, "Error when starting up. Failing.");
System.exit(1);
}
log.info("Finished peon task");
}
catch (Exception e) {
throw Throwables.propagate(e);
Expand Down