Replace PublisherInterface with Publisher #900
Conversation
|
I think I might be able to remove |
There was a problem hiding this comment.
My main concern here is that the PublisherCustomizer is part of the public API now, and nothing really requires it to be used for trace instrumentation only. There is a risk that users will provide a PublisherCustomizer and inadvertently disable tracing during publishing. You might be able to get around this by supporting multiple PublisherCustomizer beans, but then things get a bit more complicated.
| @@ -347,7 +348,8 @@ public PublisherFactory defaultPublisherFactory( | |||
| @Qualifier("publisherBatchSettings") ObjectProvider<BatchingSettings> batchingSettings, | |||
| @Qualifier("publisherRetrySettings") ObjectProvider<RetrySettings> retrySettings, | |||
| @Qualifier("publisherTransportChannelProvider") | |||
| TransportChannelProvider publisherTransportChannelProvider) { | |||
| TransportChannelProvider publisherTransportChannelProvider, | |||
| Optional<PublisherCustomizer> customizer) { | |||
There was a problem hiding this comment.
Why not ObjectProvider like the other arguments? Also, do we need to add the @Qualifier like for the others.
There was a problem hiding this comment.
ObjectProvider is useful when you don't know whether you have one or multiple.
There was a problem hiding this comment.
Qualifier is there for non-pubsub-specific types, to distinguish, for example, a Spanner RetrySettings object from a Pub/Sub one. PublisherCustomizer is already a pubsub-specific interface.
There was a problem hiding this comment.
Why do you think there will never be multiple? It's a very generic customizer.
...-pubsub/src/main/java/com/google/cloud/spring/pubsub/core/publisher/PublisherCustomizer.java
Show resolved
Hide resolved
...-pubsub/src/main/java/com/google/cloud/spring/pubsub/core/publisher/PublisherCustomizer.java
Outdated
Show resolved
Hide resolved
| PublisherFactory factory = | ||
| ctx.getBean("defaultPublisherFactory", PublisherFactory.class); | ||
| Publisher testPublisher = factory.createPublisher("unused"); | ||
| assertThat(testPublisher.getBatchingSettings()).isSameAs(testBatchingSettings); |
There was a problem hiding this comment.
Should you also verify that endpoint is "unused:433"?
There was a problem hiding this comment.
It was a result of an older experiment; I'll remove the line that sets it.
There is no getter for host/port, and while it's possible to use reflection, I went with something that's easy to validate.
...in/java/com/google/cloud/spring/autoconfigure/trace/pubsub/TracePubSubBeanPostProcessor.java
Show resolved
Hide resolved
|
|
||
| @Bean | ||
| @ConditionalOnMissingBean | ||
| PublisherCustomizer publisherCustomizer(PubSubTracing pubSubTracing) { |
There was a problem hiding this comment.
This is specifically the tracePublisherCustomizer. So, I think it needs to be named that way.
|
@elefeint Maybe what you can do is just keep |
…pubsub/core/publisher/PublisherCustomizer.java Co-authored-by: Mike Eltsufin <meltsufin@google.com>
|
Kudos, SonarCloud Quality Gate passed! |
After the discussion in googleapis/java-pubsub#949, reverting back to using `Publisher` for all API return types. I've ported the trace header injection functionality from `TracingPublisher` into `TracingPublisherFactory`. The Trace information will now get into a standard `Publisher` through a builder callback `PubsubMessage.Builder.setTransform()`.
After the discussion in googleapis/java-pubsub#949, reverting back to using `Publisher` for all API return types. I've ported the trace header injection functionality from `TracingPublisher` into `TracingPublisherFactory`. The Trace information will now get into a standard `Publisher` through a builder callback `PubsubMessage.Builder.setTransform()`.
After the discussion in googleapis/java-pubsub#949, reverting back to using
Publisherfor all API return types. I've ported the trace header injection functionality fromTracingPublisherintoTracingPublisherFactory.The Trace information will now get into a standard
Publisherthrough a builder callbackPubsubMessage.Builder.setTransform().This PR is a pre-requisite for fixing #864.
Fixes #790