Add MessageConsumerImpl class, implement pullAsync, add tests #1043

mziccard · 2016-06-07T21:50:26Z

This PR adds an implementation for the MessageConsumer interface and implements the MessageConsumer pullAsync(Subscription, MessageProcessor, PullOption...) method. This PR also adds unit and system tests.

A concern that I have about this PR is that the PullOption.executor option (used to provide a custom callback executor to the message consumer) cannot be made serializable. Can we live with that? Better solutions?

coveralls · 2016-06-08T09:20:52Z

Coverage increased (+0.1%) to 85.23% when pulling 8eafca6 on mziccard:pubsub-consumer into 2fd2d56 on GoogleCloudPlatform:pubsub-alpha.

mziccard · 2016-06-08T09:22:00Z

/cc @aozarov if you want to have a look

mziccard · 2016-06-08T21:41:58Z

Putting this on hold until changes discussed in #1041 are applied

coveralls · 2016-06-14T14:04:54Z

Coverage increased (+0.1%) to 85.849% when pulling 7693456 on mziccard:pubsub-consumer into d487307 on GoogleCloudPlatform:pubsub-alpha.

mziccard · 2016-06-14T14:32:49Z

@aozarov I rebased to get latest changes to the spi layer. PTAL

gcloud-java-pubsub/src/main/java/com/google/cloud/pubsub/MessageConsumerImpl.java

+
+  private static final int MAX_QUEUED_CALLBACKS = 100;
+  // shared scheduled executor, used to schedule pulls
+  private static final SharedResourceHolder.Resource<ScheduledExecutorService> TIMER =


coveralls · 2016-06-20T14:45:23Z

Coverage increased (+0.1%) to 85.846% when pulling ff4c0a2 on mziccard:pubsub-consumer into d487307 on GoogleCloudPlatform:pubsub-alpha.

mziccard · 2016-06-20T15:32:00Z

@aozarov I did some refactoring according to comments, PTAL

gcloud-java-pubsub/src/main/java/com/google/cloud/pubsub/MessageConsumerImpl.java

+    this.deadlineRenewer = builder.deadlineRenewer;
+    this.queuedCallbacks = new AtomicInteger();
+    this.timer = SharedResourceHolder.get(TIMER);
+    this.executorFactory = firstNonNull(builder.executorFactory, new DefaultExecutorFactory());


gcloud-java-pubsub/src/main/java/com/google/cloud/pubsub/MessageConsumerImpl.java

+    };
+  }
+
+  private PullRequest createPullRequest() {


coveralls · 2016-06-20T21:36:41Z

Coverage increased (+0.09%) to 85.832% when pulling d7ba0aa on mziccard:pubsub-consumer into d487307 on GoogleCloudPlatform:pubsub-alpha.

gcloud-java-pubsub/src/main/java/com/google/cloud/pubsub/MessageConsumerImpl.java

+  /**
+   * Interface for policies according to which the consumer should be restarted.
+   */
+  interface RestartPolicy {


aozarov · 2016-06-20T23:47:57Z

Few minor comments. Overall looks good.

coveralls · 2016-06-21T17:50:01Z

Coverage increased (+0.09%) to 85.832% when pulling 61bdd81 on mziccard:pubsub-consumer into d487307 on GoogleCloudPlatform:pubsub-alpha.

aozarov · 2016-06-21T18:11:14Z

Looks good to me. timer at this point does not need to be ScheduledExecutorService and can be just ExecutorService but I think it is fine to leave it as is if you wish to support future enhancement to NextPullPolicy.

We need someone to look more carefully at the docs and tests. Maybe @lesv ?

coveralls · 2016-06-22T06:55:21Z

Coverage increased (+0.09%) to 85.837% when pulling 66f273c on mziccard:pubsub-consumer into d487307 on GoogleCloudPlatform:pubsub-alpha.

mziccard · 2016-06-22T17:40:37Z

@lesv do you plan to have a look at this?

lesv · 2016-06-22T19:07:51Z

Yes - if I'm not cc'd, I don't see it at the moment. Trying to fix it, but I'm still digging out from something in Q1.

lesv · 2016-06-22T19:32:00Z

LGTM - 1. it seem like we should be able to do something like @SupressWarnings to Codacy
2. Do you write any higher level tests? That actually invoke the API's and talk to a server? It just seems wrong not to have those.

mziccard · 2016-06-22T19:43:02Z

it seem like we should be able to do something like @SupressWarnings to Codacy

Some of these warnings are false positives but in other cases they might still be helpful (I am thinking about the override warning here, I believe we could silent the one on raw exceptions).

Do you write any higher level tests? That actually invoke the API's and talk to a server?

Tests defined in BaseSystemTest are executed against the local emulator (via LocalSystemTest implementation). I haven't pushed an implementation of BaseSystemTest to run tests against the actual service (although I have it and use it before pushing changes) because they take really long (>10 minutes). That will make our build take ~40 minutes which seemed to me a bit too much (given that we already run PubSub tests against the emulator). @aozarov do you have some suggestions? Can we live with such long times?

lesv · 2016-06-22T19:53:43Z

We should at the very least have them in the repo, even if they don't execute regularly, they should be executed before push to maven. (Which should happen someday soon?)

aozarov · 2016-06-22T19:56:27Z

Why do the tests against production take that long? In any case, those should be integration test and not standard unit-tests.

mziccard · 2016-06-22T21:15:58Z

Why do the tests against production take that long?

Unit tests by themselves take around 3 minutes to run. Adding integration tests makes tests take around 10 minutes. It's no surprise given the increased cost of created/getting/deleting resources against the actual service. Also, for MessageConsumer tests, the remote service takes more time (when compared to the emulator) to send back to subscriptions messages that are published to a topic, forcing tests to hang longer waiting that published messages are actually pulled.

In any case, those should be integration test and not standard unit-tests.

This does not change that on PR merges Travis will take around 40 minutes.

I will add the IT tests and configure Travis with a larger timeout.

aozarov · 2016-06-22T21:51:21Z

Maybe it is about time to play with running the tests in parallel?

…apis#1043)

🤖 I have created a release *beep* *boop* --- ## [3.3.0](googleapis/java-aiplatform@v3.2.0...v3.3.0) (2022-09-15) ### Features * add input_artifacts to PipelineJob.runtime_config in aiplatform v1 pipeline_job ([ff7a4ab](googleapis/java-aiplatform@ff7a4ab)) * Add model_monitoring_stats_anomalies,model_monitoring_status to BatchPredictionJob in aiplatform v1beta1 batch_prediction_job.proto ([#1041](googleapis/java-aiplatform#1041)) ([fe03c60](googleapis/java-aiplatform@fe03c60)) * Add read_mask to ListPipelineJobsRequest in aiplatform v1 pipeline_service ([#1032](googleapis/java-aiplatform#1032)) ([ff7a4ab](googleapis/java-aiplatform@ff7a4ab)) * Add UpsertDatapoints and RemoveDatapoints rpcs to IndexService in aiplatform v1 index_service.proto ([#1030](googleapis/java-aiplatform#1030)) ([4114021](googleapis/java-aiplatform@4114021)) * add UpsertDatapoints and RemoveDatapoints rpcs to IndexService in aiplatform v1beta1 index_service.proto ([4114021](googleapis/java-aiplatform@4114021)) * Add WriteFeatureValues in aiplatform v1beta1 featurestore_online_service.proto ([#1022](googleapis/java-aiplatform#1022)) ([267827e](googleapis/java-aiplatform@267827e)) ### Dependencies * Update dependency com.google.api.grpc:proto-google-cloud-aiplatform-v1beta1 to v0.18.0 ([#1021](googleapis/java-aiplatform#1021)) ([c3d34a0](googleapis/java-aiplatform@c3d34a0)) * Update dependency com.google.cloud:google-cloud-bigquery to v2.14.5 ([#1016](googleapis/java-aiplatform#1016)) ([cc0af7b](googleapis/java-aiplatform@cc0af7b)) * Update dependency com.google.cloud:google-cloud-bigquery to v2.14.6 ([#1026](googleapis/java-aiplatform#1026)) ([6ed66cc](googleapis/java-aiplatform@6ed66cc)) * Update dependency com.google.cloud:google-cloud-bigquery to v2.14.7 ([#1031](googleapis/java-aiplatform#1031)) ([5adefd4](googleapis/java-aiplatform@5adefd4)) * Update dependency com.google.cloud:google-cloud-bigquery to v2.15.0 ([#1040](googleapis/java-aiplatform#1040)) ([b6c1b20](googleapis/java-aiplatform@b6c1b20)) * Update dependency com.google.cloud:google-cloud-bigquery to v2.16.0 ([#1044](googleapis/java-aiplatform#1044)) ([5ce52e5](googleapis/java-aiplatform@5ce52e5)) * Update dependency com.google.cloud:google-cloud-shared-dependencies to v3.0.2 ([#1043](googleapis/java-aiplatform#1043)) ([40eae55](googleapis/java-aiplatform@40eae55)) * Update dependency com.google.cloud:google-cloud-shared-dependencies to v3.0.3 ([#1045](googleapis/java-aiplatform#1045)) ([0f66302](googleapis/java-aiplatform@0f66302)) * Update dependency com.google.cloud:google-cloud-storage to v2.11.3 ([#1015](googleapis/java-aiplatform#1015)) ([7a2fcb0](googleapis/java-aiplatform@7a2fcb0)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

🤖 I have created a release *beep* *boop* --- ## [2.2.13](https://togithub.com/googleapis/java-video-intelligence/compare/v2.2.12...v2.2.13) (2022-10-07) ### Dependencies * Update dependency com.google.cloud:google-cloud-core to v2.8.20 ([googleapis#1038](https://togithub.com/googleapis/java-video-intelligence/issues/1038)) ([51e1f2c](https://togithub.com/googleapis/java-video-intelligence/commit/51e1f2c6dc330f60f4b1c317ee8e10f029dfc108)) * Update dependency com.google.cloud:google-cloud-storage to v2.13.0 ([googleapis#1042](https://togithub.com/googleapis/java-video-intelligence/issues/1042)) ([a6cbae0](https://togithub.com/googleapis/java-video-intelligence/commit/a6cbae02b8831249a7f200679bdbb7ed738f55ce)) --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).

….6.22 (#1043)

mziccard added the api: pubsub Issues related to the Pub/Sub API. label Jun 7, 2016

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 7, 2016

mziccard added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 7, 2016

mziccard force-pushed the pubsub-consumer branch 3 times, most recently from 1652076 to 8eafca6 Compare June 8, 2016 09:08

mziccard added do not merge Indicates a pull request not ready for merge, due to either quality or timing. and removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Jun 8, 2016

Add MessageConsumerImpl class, implement pullAsync, add tests

7693456

mziccard force-pushed the pubsub-consumer branch from 8eafca6 to 7693456 Compare June 14, 2016 13:51

mziccard removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 14, 2016

aozarov reviewed Jun 17, 2016
View reviewed changes

mziccard added 2 commits June 20, 2016 12:33

PullOption.executor option changed to PullOption.executorFactory

5260293

Refactor MessageConsumerImpl

ff4c0a2

mziccard force-pushed the pubsub-consumer branch from 28512a3 to ff4c0a2 Compare June 20, 2016 14:26

aozarov reviewed Jun 20, 2016
View reviewed changes

gcloud-java-pubsub/src/main/java/com/google/cloud/pubsub/MessageConsumerImpl.java

};

}

private PullRequest createPullRequest() {

This comment was marked as spam.

Sign in to view

Implement consumer restart based on processed callbacks

d7ba0aa

aozarov reviewed Jun 20, 2016
View reviewed changes

gcloud-java-pubsub/src/main/java/com/google/cloud/pubsub/MessageConsumerImpl.java

/**

* Interface for policies according to which the consumer should be restarted.

*/

interface RestartPolicy {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

mziccard added 2 commits June 21, 2016 18:52

Rename RestartPolicy to NextPullPolicy

092acfc

Fix MessageConsumerImpl tests

61bdd81

Rename TIMER to CONSUMER_EXECUTOR and make it an ExecutorService

66f273c

mziccard merged commit 7a7574d into googleapis:pubsub-alpha Jun 22, 2016

mziccard added a commit to mziccard/gcloud-java that referenced this pull request Jun 27, 2016

Add MessageConsumerImpl class, implement pullAsync, add tests (google…

f6f552e

…apis#1043)

suztomo pushed a commit that referenced this pull request Feb 1, 2023

deps: update dependency com.google.api.grpc:proto-google-iam-v1 to v1…

3f6f782

….6.22 (#1043)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MessageConsumerImpl class, implement pullAsync, add tests #1043

Add MessageConsumerImpl class, implement pullAsync, add tests #1043

mziccard commented Jun 7, 2016

coveralls commented Jun 8, 2016

mziccard commented Jun 8, 2016

mziccard commented Jun 8, 2016

coveralls commented Jun 14, 2016

mziccard commented Jun 14, 2016

This comment was marked as spam.

coveralls commented Jun 20, 2016

mziccard commented Jun 20, 2016

This comment was marked as spam.

This comment was marked as spam.

coveralls commented Jun 20, 2016 •

edited

Loading

This comment was marked as spam.

This comment was marked as spam.

aozarov commented Jun 20, 2016

coveralls commented Jun 21, 2016

aozarov commented Jun 21, 2016

coveralls commented Jun 22, 2016

mziccard commented Jun 22, 2016

lesv commented Jun 22, 2016

lesv commented Jun 22, 2016

mziccard commented Jun 22, 2016

lesv commented Jun 22, 2016

aozarov commented Jun 22, 2016

mziccard commented Jun 22, 2016

aozarov commented Jun 22, 2016

Add MessageConsumerImpl class, implement pullAsync, add tests #1043

Add MessageConsumerImpl class, implement pullAsync, add tests #1043

Conversation

mziccard commented Jun 7, 2016

coveralls commented Jun 8, 2016

mziccard commented Jun 8, 2016

mziccard commented Jun 8, 2016

coveralls commented Jun 14, 2016

mziccard commented Jun 14, 2016

This comment was marked as spam.

coveralls commented Jun 20, 2016

mziccard commented Jun 20, 2016

This comment was marked as spam.

This comment was marked as spam.

coveralls commented Jun 20, 2016 • edited Loading

This comment was marked as spam.

This comment was marked as spam.

aozarov commented Jun 20, 2016

coveralls commented Jun 21, 2016

aozarov commented Jun 21, 2016

coveralls commented Jun 22, 2016

mziccard commented Jun 22, 2016

lesv commented Jun 22, 2016

lesv commented Jun 22, 2016

mziccard commented Jun 22, 2016

lesv commented Jun 22, 2016

aozarov commented Jun 22, 2016

mziccard commented Jun 22, 2016

aozarov commented Jun 22, 2016

coveralls commented Jun 20, 2016 •

edited

Loading