Skip to content

Add WorkflowStubCallsInterceptor which provides an access to Headers #382

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

Merged

Conversation

Spikhalskiy
Copy link
Contributor

@Spikhalskiy Spikhalskiy commented Mar 11, 2021

What was changed:

Added new WorkflowClientCallsInterceptor with the same idea and implemented in the same way as WorkflowOutboundCallsInterceptor, WorkflowInboundCallsInterceptor, ActivityInboundCallsInterceptor that provides Header access.
Part of WorkflowStubImpl is refactored to be implemented in a similar manner with SyncWorkflowContext implements WorkflowOutboundCallsInterceptor.

Why?

We need to have client interceptors with access to Headers.
The reason to add a new interceptor interface is an inability to wire Headers in the existing WorkflowClientInterceptor/WorkflowStub, because it would expose Headers to WorkflowStub client code and break compatibility. Also, this solution adds an Interceptor that is organized the same way as Worker Interceptors.

Closes

Partially addresses #373.

@Spikhalskiy Spikhalskiy force-pushed the workflow-stub-interceptor-headers branch from b4b89bd to a5f1d1c Compare March 11, 2021 07:29
@Spikhalskiy Spikhalskiy changed the title Add WorkflowStubOutboundInterceptor which provides and access to Headers Add WorkflowStubOutboundInterceptor which provides an access to Headers Mar 11, 2021
@Spikhalskiy Spikhalskiy force-pushed the workflow-stub-interceptor-headers branch 3 times, most recently from 17ea722 to 300a66b Compare March 11, 2021 20:07
@Spikhalskiy Spikhalskiy changed the title Add WorkflowStubOutboundInterceptor which provides an access to Headers Add WorkflowStubCallsInterceptor which provides an access to Headers Mar 11, 2021
@Spikhalskiy Spikhalskiy force-pushed the workflow-stub-interceptor-headers branch from 300a66b to cc6df07 Compare March 11, 2021 20:16
@vitarb
Copy link
Contributor

vitarb commented Mar 12, 2021

I might be missing something, but there is already a way to pass headers to the workflow start using context propagator.
In case if we don't want to use context propagators, should we considered adding headers into WorkflowOptions instead?

@Spikhalskiy
Copy link
Contributor Author

Spikhalskiy commented Mar 12, 2021

@vitarb Yeah, probably you are missing a context.
I need to implement opentracing support. After discussion with Maxim and based on rejected PR #263 it's clear that it should be implemented in interceptors. Also, we need to propagate the opentracing context on all stages. Client code -> workflow execution -> activity execution.
I can achieve it using context propagator + interceptors combination, but after discussion with @mfateev we decided that we should be able to implement it fully in interceptors.
And ContextPropagator itself in the future should be implemented using interceptors and all custom code for ContextPropagator should be removed from the main SDK code and be moved to some combination of ContextPropagator*Interceptor s. Like this:

result.putAll(propagator.serializeContext(propagator.getCurrentContext()));

That's the context of Issue #373.

This PR is the last missing piece for Issue #373. Adding an ability to modify headers in client stub interceptors. This was the first PR in this series #375

we considered adding headers into WorkflowOptions instead?

Nah, as far as I know.
The use case I care about is an ability to modify headers "silently", not by explicitly passing anything in WorkflowOptions.
Also, as far as I see from the code, we do our best to don't expose Headers even for workflow code, we want to keep it an implementation detail and expose them only for interceptors.

An alternative solution that is lighter will actually be bothering of Worflow stub interface and passing Headers there in a start method. But it will 1. break backward compatibility 2. Expose Headers as a part of WorkflowStub interface that is not internal and a part of public API. That's why I chose this way of implementing it, especially because it's the closest way to Worker Interceptors by how it works.

@vitarb
Copy link
Contributor

vitarb commented Mar 12, 2021

Discussed over zoom call, concluded that there is an alternative path through setting headers in workflow options (either using context propagators or adding a separate field(?)), which we can access through workflow stub in the WorkflowClientInterceptor.

My main concern with this PR is that it looks like a bit of an overkill for simply setting headers. On the other hand I do see value in having more generic solution. If we aim for a more generic approach though we should consider other use cases and make sure that it is actually generic enough to handle them.

Dmitry is going to implement this approach as well and then we can discuss both of them with @mfateev and decide which one we want.

Spikhalskiy added a commit to Spikhalskiy/java-sdk that referenced this pull request Mar 12, 2021
@Spikhalskiy
Copy link
Contributor Author

Spikhalskiy commented Mar 12, 2021

Drafted an alternative solution #384 with an example of how the actual interceptor could look like with that solution. From my point of view, it looks as bad as it could get. Let's discuss next week. @vitarb @mfateev

Spikhalskiy added a commit to Spikhalskiy/java-sdk that referenced this pull request Mar 12, 2021
@Spikhalskiy Spikhalskiy force-pushed the workflow-stub-interceptor-headers branch 2 times, most recently from 36430fe to 9d54b25 Compare March 19, 2021 20:22
@Spikhalskiy Spikhalskiy force-pushed the workflow-stub-interceptor-headers branch from 9d54b25 to fb8363a Compare March 19, 2021 23:01
@Spikhalskiy
Copy link
Contributor Author

Spikhalskiy commented Mar 22, 2021

@vitarb @mfateev could you guys please revisit this PR? It's pretty much done.
There is some more work to do around, especially

  1. WorkflowExecutionUtils needs a revisit and reorganization and because of that, exceptions handling in this PR is not fully systemic, but it has to be done as a separate PR.
  2. TimeLocking code can't be reworked on the new interceptors because of an issue highlighted in a separate bug report: TimeLockingFuture implementation is incorrect and easy to break  #392
  3. WorkflowClientInterceptor#newActivityCompletionClient(ActivityCompletionClient next); is not reworked in this PR. It can be done in its own independent PR and I don't want to make this PR scope overwhelming and hard to review.

Please feel free to leave any naming suggestions for anything created in this PR, I will revisit your comments and amend the names that I used.

Copy link
Contributor

@vitarb vitarb left a comment

Choose a reason for hiding this comment

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

I've fixed the build failure, it was due to warnings because of a couple of missing annotations.
Otherwise everything is looking good to me, thanks for the contribution!
Before merging in, I would like Maxim to take a glimpse too.

*/
WorkflowSignalOutput signal(WorkflowSignalInput input);

WorkflowStartOutput signalWithStart(WorkflowStartWithSignalInput input);
Copy link
Member

Choose a reason for hiding this comment

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

The input class name is not consistent with the method name.
I would create a separate structure for output as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Created WorkflowSignalWithStartOutput that has WorkflowStartOutput as a field to mirror the input structure.

@Spikhalskiy Spikhalskiy force-pushed the workflow-stub-interceptor-headers branch from ba6def0 to 0809cab Compare March 24, 2021 19:50
@vitarb vitarb merged commit fd6bb90 into temporalio:master Mar 24, 2021
Spikhalskiy added a commit to Spikhalskiy/java-sdk that referenced this pull request Mar 25, 2021
Spikhalskiy added a commit to Spikhalskiy/java-sdk that referenced this pull request Mar 25, 2021
@Spikhalskiy Spikhalskiy deleted the workflow-stub-interceptor-headers branch April 15, 2022 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants