-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Application Plugin Interface #7473
Conversation
+cc @billonahill @dabaitu |
for (Map.Entry<String, Application> entry : applications.entrySet()) { | ||
Application application = entry.getValue(); | ||
try (ThreadContextClassLoader ignored = new ThreadContextClassLoader(application.getClass().getClassLoader())) { | ||
application.run(injector); |
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.
Passing Injector
here won't work, since plugins can't see Presto's Guice from inside their class loader, by design. We intentionally do not expose the internal details of Presto to plugins. Anything needed by a plugin service should be explicitly part of the SPI.
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.
How about setting couple context groups as different ApplicationContexts in SPI just like ConnectorContext and initialize them in ApplicationManager?
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.
What is a "context group"? I feel like I'm missing a lot of context around the purpose of this pull request. Can you explain more about the use case?
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.
Initially, I suppose to bind UI on the port other than the API port here: #7106. In that PR, I just add a new module inside the PrestoServer and rebind the instance in the initialized injector to another Bootstrap, which looks hacky. I feel it would be reasonable adding an application plugin which runs on top of presto-main, and potentially passing some context instances from the initialized injector in some way. I agree that it shouldn't pass the whole injector and it shouldn't expose all internal instance to plugins.
At Facebook, we run an extended version of Presto. Create a module that depends on public class ExtendedPresto
extends PrestoServer
{
public static void main(String[] args)
{
new ExtendedPresto().run();
}
@Override
protected Iterable<? extends Module> getAdditionalModules()
{
return ImmutableList.of(...);
}
} Package it up by creating a module that is basically a copy of (To be clear, we don't have a fork of Presto internally. We depend on |
The idea of the application plugin is that the additional modules can be built within an isolated Bootstrap and with an isolated config file while are still able to refer some instances in the PrestoServer. In the example of #7106, if the config file is isolated, it would be much easier to config the port for the UI server; if the Bootstrap is isolated, it would be much easier to install the HttpServerModule. Do you think we can achieve the same goal with ExtendedPresto? |
You can do this with return ImmutableList.of(
new ThriftCodecModule(),
new ThriftClientModule(),
new ThriftServerModule(),
new Fb303StatusModule(),
...); |
I think the scenarios here are a little bit different. In your scenario, the additional modules are brand new, the config keys are different to those from other modules. However, I wish to add the
already has |
Are you saying you want to replace the Airlift Alternatively, and/or as an interim solution, you can just fork the |
I could say cleaner. The goal is letting two Airlift |
Introducing an application plugin interface can provide flexible context reference for components(for example, UI) which are built on top of Presto main.