-
Notifications
You must be signed in to change notification settings - Fork 187
Plugin Support for Java SDK #2761
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
base: master
Are you sure you want to change the base?
Conversation
| * @see PluginBase | ||
| */ | ||
| @Experimental | ||
| public interface ClientPlugin extends ClientPluginCallback { |
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.
Is this WorkflowClientPlugin or is it meant to work for ScheduleClient too (and the upcoming standalone activity client and Nexus operation client)?
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.
Let's keep it scoped to just workflows. I think it should be clear now, after I moved it to the appropriate package.
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.
This means that schedule clients can't have plugins? Clients doing schedule things can have plugins in every other SDK (same concern for standalone activity clients coming and Nexus operation clients coming)
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.
Ah i see, i was missing that in Python the big huge Client also includes scheduling stuff, whereass in java they are separate. I'll take a closer look at that.
| */ | ||
| @Override | ||
| @Nonnull | ||
| default WorkflowServiceStubsOptions.Builder configureServiceStubs( |
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.
default
Even in places where we could provide default implementations (e.g. Python and Ruby), we intentionally chose not to because we want to force implementers to implement these (even if they choose no-op). Granted in the simple plugin it makes sense to have default implementations.
| * Callback interface for client plugins to participate in service stubs creation. This interface | ||
| * is implemented by {@code ClientPlugin} in the temporal-sdk module. | ||
| */ | ||
| interface ClientPluginCallback { |
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.
I think we should just consider making a ServiceStubPlugin separate from a WorkflowClientPlugin and if someone wants to implement both, they can. We can discuss whether a plugin here automatically propagates to clients if it implements the proper interface (I think it should, same way client plugins should automatically propagate to workers if the they implement the right interfaces).
| * @return the workflow service stubs | ||
| */ | ||
| static WorkflowServiceStubs newServiceStubs( | ||
| @Nonnull WorkflowServiceStubsOptions options, @Nonnull List<?> plugins) { |
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.
I think we should accept the plugins on the options and therefore only mutate existing stub construction methods instead of adding a new one. All other SDK implementations of plugins do (acknowledging that plugins should not mutate the plugin list or it's undefined behavior). This makes it easy for users building options separate from where they call this. It also makes it easy to properly apply to the existing overloads like newConnectedServiceStubs. This was done for client, I think it's worth doing here.
|
|
||
| /** Functional interface for the connection chain. */ | ||
| @FunctionalInterface | ||
| interface ServiceStubsSupplier { |
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.
Not much benefit to this separate interface vs just changing param type to Supplier<WorkflowServiceStubs> (we don't have WorkerPluginRunnable for worker). I also don't think it should throw a checked exception.
| } | ||
|
|
||
| /** Internal implementation of the simple plugin. */ | ||
| private static final class SimplePlugin extends PluginBase { |
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.
IMO this needs to be a public concept. People need to be able to use simple plugins and still be able to override maybe one method if they still need to. I think therefore PluginBase does not need to exist, it can be replaced by SimplePlugin and Builder can be an inner class of SimplePlugin (as we do elsewhere). Can still have the (public) constructor accept options though, but I think a public no-arg (or name-only arg) constructor should also be allowed for some use cases.
Plugin System for Java Temporal SDK
What was changed
Added a plugin system for the Java Temporal SDK, modeled after the Python SDK's plugin architecture but adapted to Java idioms.
New Files (
temporal-sdk/src/main/java/io/temporal/common/plugin/)ClientPlugin.java- Client-side plugin interfaceWorkerPlugin.java- Worker-side plugin interfacePluginBase.java- Convenience base class implementing bothSimplePluginBuilder.java- Builder for declarative plugin creationModified Files
WorkflowServiceStubs.java- AddednewServiceStubs(options, plugins)andClientPluginCallbackinterfaceWorkflowClientOptions.java- Addedpluginsfield with builder methodsWorkflowClientInternalImpl.java- AppliesClientPlugin.configureClient()during creationWorkerFactory.java- Full plugin lifecycle (configuration, execution, shutdown)Test Files (
temporal-sdk/src/test/java/io/temporal/common/plugin/)PluginTest.java- Core plugin interface testsSimplePluginBuilderTest.java- Builder API testsWorkflowClientOptionsPluginTest.java- Options integration testsWhy?
The plugin system provides a higher-level abstraction over the existing interceptor infrastructure, enabling users to:
Checklist
Closes Plugin support #2626, and tracked in Plugins to support controlling multiple configuration points at once features#652.
How was this tested:
PluginTest.java,SimplePluginBuilderTest.java,WorkflowClientOptionsPluginTest.java)Any docs updates needed?
Design Decisions
1. No Base
PluginInterfaceDecision:
ClientPluginandWorkerPlugineach define their owngetName()method independently, rather than sharing a basePlugininterface.Rationale: This matches the Python SDK's design. Python has separate
ClientPluginandWorkerPluginwithname()on each. I initially had a basePlugininterface but removed it to simplify.Alternative considered: I could add a shared
Plugininterface with justgetName(). This would allowList<Plugin>instead ofList<?>for storage. However, this adds an interface that serves no purpose other than type convenience, and Python doesn't have it.2.
ClientPluginCallbackInterface (Module Boundary)Decision: A
ClientPluginCallbackinterface exists intemporal-serviceclient, whichClientPlugin(intemporal-sdk) extends.Rationale: This is required due to Java's module architecture:
temporal-serviceclientcontainsWorkflowServiceStubstemporal-sdkdepends ontemporal-serviceclient(not vice versa)WorkflowServiceStubs.newServiceStubs(options, plugins)needs to call plugin methodsThis is the one structural difference from Python, which uses a single-package architecture where everything can import everything else.
3.
PluginBaseConvenience ClassDecision: I provide an abstract
PluginBaseclass that implements bothClientPluginandWorkerPlugin.Rationale: This is a common Java pattern (like
AbstractListforList).4.
SimplePluginBuilderwith PrivateSimplePluginDecision: I provide a builder for creating plugins declaratively, with the implementation class kept private.
Rationale:
SimplePluginis an implementation detail - users interact with the builder5. No ServiceLoader Discovery
Decision: I do not include ServiceLoader-based plugin discovery.
Rationale:
plugins=[]SimplePluginBuilderWe could consider adding it in though if there is interest.
Example Usage
Custom Plugin
Using SimplePluginBuilder
Client/Worker with Plugins
Open Questions
@Experimental? - I've marked all public APIs as@Experimentalto allow iteration. Is this appropriate?