Adds PubsubAdmin #69

joaoandremartins · 2017-07-11T19:24:49Z

A Spring abstraction to simplify Google Cloud Pub/Sub topic and
subscription programmatic management. Also adds helpers to instantiate
PublisherFactory and SubscriberFactory.

Fixes #26

A Spring abstraction to simplify Google Cloud Pub/Sub topic and subscription programmatic management. Also adds helpers to instantiate PublisherFactory and SubscriberFactory. Fixes #26

joaoandremartins · 2017-07-11T19:28:46Z

There were questions on our side about whether this should be two separate classes: TopicAdmin and SubscriptionAdmin. We went with single class PubsubAdmin just for ease of use, but I can be persuaded to go a different way.

artembilan · 2017-07-12T18:44:38Z

spring-cloud-gcp-pubsub/pom.xml

+			<!-- Perl5Util, required by UrlValidator -->
+			<groupId>oro</groupId>
+			<artifactId>oro</artifactId>
+			<version>2.0.8</version>


I would like to avoid extra dependencies as much as possible. Aren't there really a tool in Google or Spring which may do the same for us?

Thanks

artembilan · 2017-07-12T18:53:20Z

spring-cloud-gcp-pubsub/src/main/java/org/springframework/cloud/gcp/pubsub/PubsubAdmin.java

+	 * @param pushEndpoint URL of the service receiving the push messages. If not provided, set to
+	 *                    Google Cloud Pub/Sub default
+	 * @return the created subscription
+	 */


I would like to have JavaDocs for all the public API, even if it is copy/paste.

Thanks

artembilan · 2017-07-12T19:02:09Z

spring-cloud-gcp-pubsub/src/main/java/org/springframework/cloud/gcp/pubsub/PubsubAdmin.java

+	private static final int DEFAULT_ACK_DEADLINE = 10;
+
+	public PubsubAdmin(GcpProjectIdProvider projectIdProvider) throws IOException {
+		this(projectIdProvider, TopicAdminClient.create(), SubscriptionAdminClient.create());


Why do we have IOException in the ctor?

It is really anti-pattern to get access to low level resources from ctors

Also how those admins work without credentials?
I mostly mean you auto-config below

Thanjs

artembilan · 2017-07-12T19:06:21Z

spring-cloud-gcp-pubsub/src/main/java/org/springframework/cloud/gcp/pubsub/PubsubAdmin.java

+
+		PushConfig.Builder pushConfigBuilder = PushConfig.newBuilder();
+		if (pushEndpoint != null) {
+			Assert.isTrue(new UrlValidator().isValid(pushEndpoint),


Why really new Url() isn't enough here?

Why do we need to extra dependencies almost for nothing?

I share your pain with the extra dependencies for seemingly little benefit..

new URL() doesn't really cut it because it requires us to catch an exception instead of just returning a boolean (and the consequent performance hit, although that's not really important) and it doesn't perform validation by itself as evidenced by this comment.

Looking around the internet, this seems to be the most popular way of validating URLs.

We can also just let the Pubsub library blow up on invalid endpoint. I just tested and it does complain:

Exception in thread "main" com.google.api.gax.grpc.ApiException: io.grpc.StatusRuntimeException: INVALID_ARGUMENT: Invalid push endpoint given (endpoint=asdasd). Refer to https://cloud.google.com/pubsub/subscriber#create for more information. at com.google.api.gax.grpc.ExceptionTransformingCallable$ExceptionTransformingFuture.onFailure(ExceptionTransformingCallable.java:108) ...

validation

artembilan · 2017-07-13T17:24:31Z

...rc/main/java/org/springframework/cloud/gcp/pubsub/autoconfig/GcpPubsubAutoConfiguration.java

+	@ConditionalOnMissingBean
+	public PubsubAdmin pubsubAdmin(GcpProjectIdProvider projectIdProvider) {
+		try {
+			return new PubsubAdmin(projectIdProvider);


??? There is no such a ctor any more...

Oops! Should've installed it locally before..

artembilan · 2017-07-13T17:29:32Z

...rc/main/java/org/springframework/cloud/gcp/pubsub/autoconfig/GcpPubsubAutoConfiguration.java

+
+	@Bean
+	@ConditionalOnMissingBean
+	public PubsubAdmin pubsubAdmin(GcpProjectIdProvider projectIdProvider) {


BTW, the real auto-config is missed.
That is what I mean by the comment here: GoogleCloudPlatform/java-docs-samples#755 (comment)

Sorry, not sure what you mean with the autoconfig is missed?

I removed @IntegrationComponentScan and @ComponentScan and PubsubAdmin is correctly autowired. You were right that I didn't need them, they must had been there from previous experiments...

OK. Sorry. I thought the PubsubAdmin is in its own auto-config class.
But that is good to hear that everything is fine there already.
Since that is a sample it would be good to keep it as simple as possible to avoid unexpected questions in the future.

Totally agreed. It was exactly that kind of feedback I was seeking from you in the sample PR!

OK. Sorry. I thought the PubsubAdmin is in its own auto-config class.
But that is good to hear that everything is fine there already.
Since that is a sample it would be good to keep it as simple as possible to avoid unexpected questions in the future.

joaoandremartins · 2017-07-13T18:20:24Z

@artembilan @meltsufin I think I addressed all the comments.

There is the question of whether this should be two classes (a SubscriptionAdmin and a TopicAdmin), but I think this simplifies usage and we can probably go with this for now. Later we might even want to make two classes, and one central class that takes the two as inputs.

artembilan

Looks like that's all.
Only credentials concern.

artembilan · 2017-07-13T18:19:54Z

...rc/main/java/org/springframework/cloud/gcp/pubsub/autoconfig/GcpPubsubAutoConfiguration.java

+	public PubsubAdmin pubsubAdmin(GcpProjectIdProvider projectIdProvider) {
+		try {
+			return new PubsubAdmin(projectIdProvider, TopicAdminClient.create(),
+					SubscriptionAdminClient.create());


???
I asked before security.
How all these .create() understand with what GC user they work?

Why this default, auto-configured PubsubAdmin doesn't depend on the CredentialsProvider?

That was really my first concern when I saw the same void create() in the PubsubAdmin ctors before.

Thanks

artembilan · 2017-07-13T18:25:23Z

I would say that I don't need this wrapper - the SubscriptionAdminClient and TopicAdminClient looks good to me as they are.
Only a gain of this wrapper that it utilize projectId. So be it.

See my comment about CredentialsProvider.

Maybe we really ca consider a variant where we hide SubscriptionAdminClient and TopicAdminClient instances, but let end-user to specify their options externally?

I think I would go only with a single class anyway, if that is about my opinion.

joaoandremartins · 2017-07-13T18:47:04Z

Good point on the credentials aspect -- it was my oversight.

I think PubsubAdmin actually fits really well within the Spring context. The Google Pub/Sub library uses objects like SubscriptionName, TopicName, etc., that we can easily spare the end user and instead just have them give us Strings.

meltsufin · 2017-07-14T13:09:39Z

spring-cloud-gcp-pubsub/src/main/java/org/springframework/cloud/gcp/pubsub/PubsubAdmin.java

+	private SubscriptionAdminClient subscriptionAdminClient;
+
+	/** Default inspired in the subscription creation web UI. */
+	private static final int DEFAULT_ACK_DEADLINE = 10;


Would it make sense to expose it as a configuration property?
Something like @Value("${spring.cloud.gcp.pubsub.defaultAckDeadline:10}").

The createSubscription() API lets you specify an ackDeadline parameter. I didn't add the value here because the user can do that by themselves in another class (i.e., they can get it from the properties or any other source).

I understand that the user can call the version of the method that takes ackDeadline but being able to configure the default externally could still be useful. Hardcoded defaults seem to be a bit contrary to Spring's philosophy.

I agree that we should have setDefaultAckDeadline().
Like we have KafkaTemplate.setDefautTopic() or RabbitTemplate.setReplyTimeout()

defaultAckDeadline is now configurable through properties.
Does that address your concerns?

meltsufin · 2017-07-14T13:15:41Z

spring-cloud-gcp-pubsub/src/main/java/org/springframework/cloud/gcp/pubsub/PubsubAdmin.java

+		PagedResponseWrappers.ListTopicsPagedResponse topicListPage =
+				this.topicAdminClient.listTopics(ProjectName.create(this.projectId));
+
+		return Lists.newArrayList(topicListPage.iterateAll());


The name of the local variable topicListPage seems to imply that it only contains a single page of results. Are you sure that iterateAll() actually return all results rather than a single page? If so, maybe rename topicListPage to just topicsList.

It does only contain a single page of results. iterateAll() returns the full collection. Or, better yet, the full collection counting from the current page.

https://github.com/googleapis/gax-java/blob/master/gax-grpc/src/main/java/com/google/api/gax/grpc/AbstractPage.java#L135
https://github.com/googleapis/gax-java/blob/master/gax-grpc/src/main/java/com/google/api/gax/grpc/AbstractPage.java#L173

Ok, my comment about the variable name still applies I think.

It would make sense for a local variable containing the result of iterateAll() to be called topicsList, not the topicAdminClient.listTopics() result. Also, the object type is ListTopicsPagedResponse, which also hints to it being a page, rather than the whole thing.

"paged response" does not necessarily imply "page". You can also call it verbosely listTopicsPagedResponse. In any case, it's so minor, it's not worth discussing further. :)

artembilan · 2017-07-14T14:21:57Z

...rc/main/java/org/springframework/cloud/gcp/pubsub/autoconfig/GcpPubsubAutoConfiguration.java

+		try {
+			return SubscriptionAdminClient.create(
+					SubscriptionAdminSettings.defaultBuilder()
+							.setCredentialsProvider(credentialsProvider)


Can't we have an additional PubsubAdmin ctor based on the CredentialsProvider to instantiate TopicAdminClient and SubscriptionAdminClient internally similar way?

Just in case for flexibility to let end-user to avoid this boilerplate code

Well, I also like that kind of simplicity, but we would then need to have that kind of low level resource handling in the constructor you were opposed to earlier. i.e., we would need to instantiate TopicAdminClient and SubscriptionAdminClient in the constructor, and they can throw IOException.

But if we're OK with it, I can add it. I personally don't mind the IOException in the constructor.

Looking to the private SubscriptionAdminSettings(Builder settingsBuilder) throws IOException { it is really unclear why an IOException is there from the day first at all.

So, having all these builders I don't think we really get access to low-level resource.
Anyway what you have here via dependency injection is still " low level resource handling" if you are interested in my opinion.

This is only concern left.
At least give some answer, please.
Thanks

Sorry, didn't mean to not reply here, just went grab lunch in the meantime. :)

IOException is thrown because of the channel creation, and maybe other cases, although I'm not sure about the exact circumstances it can happen (internet endpoint is unreachable?). Because of that, I think you do have access to low level resources, like a TCP connection, when you're creating a Subscription.

I really like this way because you still have to handle low level resources, but you do that away from the constructor. If you need to handle the IOException, you do it away from the constructor. And it should still be easy enough for users to just call new PubsubAdmin(projectIdProvider, TopicAdminClient.create(), SubscriptionAdminClient.create()).

Although, to be honest, I'm not opposed to offer that constructor on the side too.

OK. Fine with me. Looks like we get IOException from the CredentialsProvider.getCredentials() when we open a file on the matter.

No more discussion on the matter! I can live with this. 😄

artembilan · 2017-07-14T14:26:07Z

spring-cloud-gcp-pubsub/src/main/java/org/springframework/cloud/gcp/pubsub/PubsubAdmin.java

@@ -46,7 +47,8 @@
 	private SubscriptionAdminClient subscriptionAdminClient;

 	/** Default inspired in the subscription creation web UI. */
-	private static final int DEFAULT_ACK_DEADLINE = 10;
+	@Value("${spring.cloud.gcp.pubsub.subscriber.ackDeadline:10}")


No, that's wrong.
To have @Value resolved your must have specific configuration in the application context.
Also this looks like a mix of auto-config and regular components concerns.
This is fully fine to have this property resolution in the auto-config, but here it must be setter.

At one point, I used a setter, but got rid of it because I don't want PubsubAdmin to have state. PubsubAdmin basically becomes not threadsafe.

??? So, in your terms anything with setter is evil?

All the components in Spring are based on setters.
Right, you should always keep in mind if you can change the state of an object it isn't thread-safe.
That's not only for Spring classes.

I insist that we should not use @Value in target components. That is end-user application feature or maximum auto-config.

And why do you care too much that it won't be thread-safe?.. More over the story is only about an int value, which can be volatile and that's all...

I don't know about other Spring components, but IMO PubsubAdmin being this central class with a bunch of functionality and singleton by default, it's likely to be used from several different threads and inconsistency here is an actual hazard. Also, the added value of setting different defaults hardly justifies risking inconsistency.

I'm not opposed to getting rid of the @Value either -- setting a new default is a nice to have, but not that useful, and users have the ability to call createSubscription() with a different default they have declared somewhere else.

Again, this is all IMO -- if you feel strongly about your point of view, I can add the setter...

FWIW I think the setter is fine. We make no claims about this class being thread-safe. Besides, this value would typically only be set on app initialization anyway.

artembilan · 2017-07-14T16:17:52Z

spring-cloud-gcp-pubsub/src/main/java/org/springframework/cloud/gcp/pubsub/PubsubAdmin.java

+		return this.defaultAckDeadline;
+	}
+
+	public void setDefaultAckDeadline(int defaultAckDeadline) {


JavaDocs for public API.

For setters and getters?

artembilan · 2017-07-14T16:28:10Z

spring-cloud-gcp-pubsub/src/main/java/org/springframework/cloud/gcp/pubsub/PubsubAdmin.java

 	public int getDefaultAckDeadline() {
 		return this.defaultAckDeadline;
 	}

+	/**
+	 * Sets the default acknowledgement deadline value.


Nit-pick.
Method JavaDocs should be imperative not descriptive.
In other words: Set the default ....

santhh · 2018-01-15T03:05:45Z

I was trying GCP pub sub form a spring boot app and get this error message on start up. It's relate to pub sub admin and auto configuration class. Has anyone seen this before?

org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'pubSubAdmin' defined in class path resource [org/springframework/cloud/gcp/pubsub/autoconfig/GcpPubSubAutoConfiguration.class]: Unsatisfied dependency expressed through method 'pubSubAdmin' parameter 0; nested exception is org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'topicAdminClient' defined in class path resource [org/springframework/cloud/gcp/pubsub/autoconfig/GcpPubSubAutoConfiguration.class]:

artembilan · 2018-01-15T03:12:45Z

@santhh,

Please, don’t repeat your problem in different places. We got your issue #307 and will come back to you as soon as possible, next week. Right now it is weekend and even I am typing here from the phone.

Thank you for understanding

Adds PubsubAdmin

f316503

A Spring abstraction to simplify Google Cloud Pub/Sub topic and subscription programmatic management. Also adds helpers to instantiate PublisherFactory and SubscriberFactory. Fixes #26

joaoandremartins requested review from artembilan and meltsufin July 11, 2017 19:24

joaoandremartins added 2 commits July 11, 2017 16:04

Adds topic and subscription deletion

b7f63fd

Merge branch 'master' into gh-26-pubsubadmin

8cf3e0e

spring-attic deleted a comment from meltsufin Jul 11, 2017

artembilan suggested changes Jul 12, 2017

View reviewed changes

joaoandremartins added 3 commits July 12, 2017 17:18

Adds ability to list topics and subscriptions

313bb70

Removes extra constructors, adds JavaDoc and removes explicit URL

02da16b

validation

Fix imports

513eefc

artembilan suggested changes Jul 13, 2017

View reviewed changes

Fix autoconfig

3aff062

artembilan reviewed Jul 13, 2017

View reviewed changes

artembilan suggested changes Jul 13, 2017

View reviewed changes

Parameterises TopicAdminClient and SubscriptionAdminClient

0a44e3a

meltsufin reviewed Jul 14, 2017

View reviewed changes

Parameterises defaultAckDeadline

c0fef68

artembilan reviewed Jul 14, 2017

View reviewed changes

joaoandremartins added 2 commits July 14, 2017 12:05

Getters and setters for defaultAckDeadline instead

d2f2fca

Checkstyle

e5e55cd

artembilan reviewed Jul 14, 2017

View reviewed changes

Getter and setter Javadoc

a663857

artembilan reviewed Jul 14, 2017

View reviewed changes

joaoandremartins added 2 commits July 14, 2017 12:30

Imperative Javadoc

6edcc0e

Adds another simpler constructor

ac3c3b0

artembilan approved these changes Jul 14, 2017

View reviewed changes

joaoandremartins merged commit 7fffaa5 into master Jul 14, 2017

joaoandremartins deleted the gh-26-pubsubadmin branch July 14, 2017 18:25

Adds PubsubAdmin #69

Adds PubsubAdmin #69

Conversation

joaoandremartins commented Jul 11, 2017

joaoandremartins commented Jul 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joaoandremartins commented Jul 13, 2017

artembilan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artembilan commented Jul 13, 2017

joaoandremartins commented Jul 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

santhh commented Jan 15, 2018 • edited Loading

artembilan commented Jan 15, 2018

santhh commented Jan 15, 2018 •

edited

Loading