feat: Add CredentialsProvider to Publisher and Subscriber settings #475

dpcollins-google · 2021-02-01T03:46:07Z

These objects require more than one client and are annoying to construct correctly. Add a commonly needed setting to the top level here.

Fixes #472

These objects require more than one client and are annoying to construct correctly. Add a commonly needed setting to the top level here. Fixes #472

codecov · 2021-02-01T04:17:03Z

Codecov Report

Merging #475 (72477ab) into master (cd3c41a) will decrease coverage by 0.42%.
The diff coverage is 42.94%.

@@             Coverage Diff              @@
##             master     #475      +/-   ##
============================================
- Coverage     73.09%   72.67%   -0.43%     
- Complexity      835      840       +5     
============================================
  Files           151      151              
  Lines          4368     4374       +6     
  Branches        222      214       -8     
============================================
- Hits           3193     3179      -14     
- Misses         1056     1078      +22     
+ Partials        119      117       -2

Impacted Files	Coverage Δ	Complexity Δ
...google/cloud/pubsublite/beam/PublisherOptions.java	`14.28% <0.00%> (-10.72%)`	`4.00 <0.00> (ø)`
...oogle/cloud/pubsublite/beam/SubscriberOptions.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...loud/pubsublite/cloudpubsub/PublisherSettings.java	`42.59% <13.33%> (-26.16%)`	`6.00 <1.00> (+1.00)`	⬇️
...oud/pubsublite/cloudpubsub/SubscriberSettings.java	`58.82% <53.57%> (-30.54%)`	`12.00 <10.00> (ø)`
...oud/pubsublite/internal/wire/AssignerSettings.java	`100.00% <100.00%> (ø)`	`4.00 <4.00> (?)`
...ud/pubsublite/internal/wire/CommitterSettings.java	`100.00% <100.00%> (ø)`	`3.00 <3.00> (?)`
...internal/wire/PartitionCountWatchingPublisher.java	`97.05% <100.00%> (-0.03%)`	`18.00 <0.00> (ø)`
.../wire/PartitionCountWatchingPublisherSettings.java	`100.00% <100.00%> (+53.33%)`	`3.00 <1.00> (+1.00)`
...oud/pubsublite/internal/wire/PublisherBuilder.java	`86.66% <100.00%> (+17.91%)`	`3.00 <1.00> (ø)`
...internal/wire/SinglePartitionPublisherBuilder.java	`100.00% <100.00%> (ø)`	`2.00 <0.00> (ø)`
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd3c41a...c1b304c. Read the comment docs.

manuelmenzella-google

LGTM if my comment is addressed.

manuelmenzella-google · 2021-02-01T14:16:57Z

google-cloud-pubsublite/clirr-ignored-differences.xml

@@ -1,15 +1,11 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <!-- see http://www.mojohaus.org/clirr-maven-plugin/examples/ignored-differences.html -->
 <differences>
-  <!-- Removed not needed API -->
+  <!-- Added abstract method to AutoValue.Builder class (Always okay) -->


Can you instead add a specific exclusion for what you are adding? Not all changes to Builders will be adding abstract methods. Will this trigger on removing methods?

This specifically triggers on adding methods to nested classes named "Builder". I think thats specific enough. (code 7013 is added abstract method to abstract class)

No, this isn't specific enough and would require a major version bump if this API had reached 1.0.

I disagree. Addition of a method to a Builder class, which is only overridden in an autogenerated AutoValue file, would not trigger a major version bump as it is not a backwards-incompatible API surface change.

elharo · 2021-02-01T14:59:24Z

google-cloud-pubsublite/clirr-ignored-differences.xml

@@ -1,15 +1,11 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <!-- see http://www.mojohaus.org/clirr-maven-plugin/examples/ignored-differences.html -->
 <differences>
-  <!-- Removed not needed API -->
+  <!-- Added abstract method to AutoValue.Builder class (Always okay) -->


No, this isn't specific enough and would require a major version bump if this API had reached 1.0.

Irrelevant to present library state. Possibly would be worth an out of band discussion, but dismissing to unblock a customer issue.

suztomo · 2021-04-19T16:13:58Z

@dpcollins-google I'm thinking to upgrade google-cloud-pubsublite dependency in Apache Beam (Google Dataflow) from 0.7.0 to 0.13.2. It has breaking changes including this PR475 (compilation errors). Before I try to fix Beam's code, do you happen to know anyone working on Beam's pubsublite dependency upgrade already?

suztomo · 2021-04-19T19:18:23Z

...oud-pubsublite/src/main/java/com/google/cloud/pubsublite/cloudpubsub/SubscriberSettings.java

+      Committer wireCommitter =
+          CommitterSettings.newBuilder()
+              .setSubscriptionPath(subscriptionPath())
+              .setPartition(partition)
+              .setServiceClient(newCursorServiceClient())
+              .build()
+              .instantiate();


Memo for myself. This seems the way to replace CommitterBuilder:

CommitterBuilder.Builder wireCommitterBuilder = CommitterBuilder.newBuilder(); wireCommitterBuilder.setSubscriptionPath(canonicalPath); cursorServiceClientSupplier() .ifPresent(supplier -> wireCommitterBuilder.setServiceClient(supplier.get())); wireCommitterBuilder.setPartition(partition);

suztomo · 2021-04-19T19:31:19Z

...loud-pubsublite/src/main/java/com/google/cloud/pubsublite/cloudpubsub/PublisherSettings.java

@@ -125,16 +188,12 @@ Publisher instantiate() throws ApiException {
                  SinglePartitionPublisherBuilder.Builder singlePartitionBuilder =
                      underlyingBuilder()
                          .setBatchingSettings(batchingSettings)
-                          .setContext(PubsubContext.of(FRAMEWORK))


Beam also touches this removed setContext method. Should it just remove the usage?

https://github.com/apache/beam/blob/6a70085/sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/pubsublite/SubscriberOptions.java#L104

This has been moved to newServiceClient method. (Thanks @dpcollins-google !)

suztomo · 2021-04-19T20:15:12Z

...src/main/java/com/google/cloud/pubsublite/internal/wire/PartitionCountWatchingPublisher.java

-  public PartitionCountWatchingPublisher(PartitionCountWatchingPublisherSettings settings) {
-    this.publisherFactory = settings.publisherFactory();
-    this.policyFactory = settings.routingPolicyFactory();
-    PartitionCountWatcher configWatcher =
-        settings.configWatcherFactory().newWatcher(this::handleConfig);
+  PartitionCountWatchingPublisher(
+      PartitionPublisherFactory publisherFactory,
+      RoutingPolicy.Factory policyFactory,
+      PartitionCountWatcher.Factory configWatcherFactory) {
+    this.publisherFactory = publisherFactory;
+    this.policyFactory = policyFactory;
+    PartitionCountWatcher configWatcher = configWatcherFactory.newWatcher(this::handleConfig);
    addServices(configWatcher);
  }


Memo for myself. This constructor is now invisible but used by Beam
https://github.com/apache/beam/blob/243128a8fc52798e1b58b0cf1a271d95ee7aa241/sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/pubsublite/Publishers.java#L46

suztomo · 2021-04-19T20:38:02Z

pubsublite-beam-io/src/main/java/com/google/cloud/pubsublite/beam/PublisherOptions.java

+    PublisherServiceSettings.Builder settingsBuilder = PublisherServiceSettings.newBuilder();
+    settingsBuilder =
+        addDefaultMetadata(
+            PubsubContext.of(FRAMEWORK),


Note for myself. This sets the context now.

feat: Add CredentialsProvider to Publisher and Subscriber settings

038cda4

These objects require more than one client and are annoying to construct correctly. Add a commonly needed setting to the top level here. Fixes #472

dpcollins-google requested a review from a team as a code owner February 1, 2021 03:46

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Feb 1, 2021

product-auto-label bot added the api: pubsublite Issues related to the googleapis/java-pubsublite API. label Feb 1, 2021

dpcollins-google added 3 commits January 31, 2021 22:57

feat: Add CredentialsProvider to Publisher and Subscriber settings

6dd21e0

These objects require more than one client and are annoying to construct correctly. Add a commonly needed setting to the top level here. Fixes #472

feat: Add CredentialsProvider to Publisher and Subscriber settings

b6fda04

These objects require more than one client and are annoying to construct correctly. Add a commonly needed setting to the top level here. Fixes #472

fix: clirr

056cb28

dpcollins-google requested a review from a team as a code owner February 1, 2021 04:15

dpcollins-google added 5 commits January 31, 2021 23:19

fix: flaky test

cbd2673

fix: deps

45ae1fb

fix: scopes

0f7be6c

fix: format

8144b32

fix: dependencies again

997a352

manuelmenzella-google approved these changes Feb 1, 2021

View reviewed changes

fix: admin endpoint

c1b304c

dpcollins-google added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 1, 2021

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 1, 2021

elharo previously requested changes Feb 1, 2021

View reviewed changes

dpcollins-google merged commit ba16af8 into master Feb 1, 2021

dpcollins-google deleted the creds_to_settings branch February 1, 2021 15:07

suztomo reviewed Apr 19, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add CredentialsProvider to Publisher and Subscriber settings #475

feat: Add CredentialsProvider to Publisher and Subscriber settings #475

dpcollins-google commented Feb 1, 2021

codecov bot commented Feb 1, 2021 •

edited

manuelmenzella-google left a comment

manuelmenzella-google Feb 1, 2021

dpcollins-google Feb 1, 2021

elharo Feb 1, 2021

dpcollins-google Feb 1, 2021 •

edited

elharo Feb 1, 2021

suztomo commented Apr 19, 2021 •

edited

suztomo Apr 19, 2021

suztomo Apr 19, 2021 •

edited

suztomo Apr 19, 2021

suztomo Apr 19, 2021

suztomo Apr 19, 2021 •

edited

feat: Add CredentialsProvider to Publisher and Subscriber settings #475

feat: Add CredentialsProvider to Publisher and Subscriber settings #475

Conversation

dpcollins-google commented Feb 1, 2021

codecov bot commented Feb 1, 2021 • edited

Codecov Report

manuelmenzella-google left a comment

Choose a reason for hiding this comment

manuelmenzella-google Feb 1, 2021

Choose a reason for hiding this comment

dpcollins-google Feb 1, 2021

Choose a reason for hiding this comment

elharo Feb 1, 2021

Choose a reason for hiding this comment

dpcollins-google Feb 1, 2021 • edited

Choose a reason for hiding this comment

elharo Feb 1, 2021

Choose a reason for hiding this comment

suztomo commented Apr 19, 2021 • edited

suztomo Apr 19, 2021

Choose a reason for hiding this comment

suztomo Apr 19, 2021 • edited

Choose a reason for hiding this comment

suztomo Apr 19, 2021

Choose a reason for hiding this comment

suztomo Apr 19, 2021

Choose a reason for hiding this comment

suztomo Apr 19, 2021 • edited

Choose a reason for hiding this comment

codecov bot commented Feb 1, 2021 •

edited

dpcollins-google Feb 1, 2021 •

edited

suztomo commented Apr 19, 2021 •

edited

suztomo Apr 19, 2021 •

edited

suztomo Apr 19, 2021 •

edited