-
Notifications
You must be signed in to change notification settings - Fork 2
PubSub background function adapter #14
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
…to pubsub-bg-function # Conflicts: # spring-cloud-function-adapters/spring-cloud-function-adapter-gcp/src/main/java/org/springframework/cloud/function/adapter/gcloud/GcfSpringBootHttpRequestHandler.java # spring-cloud-function-adapters/spring-cloud-function-adapter-gcp/src/test/java/org/springframework/cloud/function/adapter/gcloud/GcfSpringBootHttpRequestHandlerTests.java
| } | ||
|
|
||
| public void init() { | ||
| Thread.currentThread().setContextClassLoader(GcfSpringBootHttpRequestHandler.class.getClassLoader()); |
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.
Set classloader to the GcfHandler class
| import org.springframework.cloud.function.context.AbstractSpringFunctionAdapterInitializer; | ||
| import org.springframework.messaging.Message; | ||
|
|
||
| public class GcfHandler<O> extends AbstractSpringFunctionAdapterInitializer<Context> { |
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.
Would it be better to just have the background function and httpFunction extend from the AbstractSpringFunctionAdapterInitializer directly? I think right now for HttpFunction to extend from GcfHandler might not be ideal if it doesn't have access to the Context.
| return Flux.just(input); | ||
| } | ||
|
|
||
| protected O convertOutputAndHeaders(Object output, HttpResponse resp) { |
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.
Maybe for adding background function we should take a similar approach to the PR for the HttpFunction, like just keeping it minimal without headers etc. Would be easier to review and get in to the Spring team
You could make a branch from this branch to save your code as a checkpoint, then in this PR reduce it to the minimum functionality needed to get PubSub background function working? similar to what Mike sent to the Spring team?
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 agree. We should try to keep the PRs smaller.
Additionally, I think we should build on what's already in master. Why bring back the original HTTP handler here?
Lastly, since we're not sure what's going to happen to the HTTP handler during review, we may hold off on this PR until we get feedback on the other one.
No description provided.