feat: Add CredentialsProvider to Publisher and Subscriber settings by dpcollins-google · Pull Request #475 · googleapis/java-pubsublite

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

 <!-- 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

 <!-- 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

+      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

                  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


-  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

+    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 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 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

Conversation

dpcollins-google commented Feb 1, 2021

Uh oh!

codecov Bot commented Feb 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

manuelmenzella-google left a comment

Choose a reason for hiding this comment

Uh oh!

manuelmenzella-google Feb 1, 2021

Choose a reason for hiding this comment

Uh oh!

dpcollins-google Feb 1, 2021

Choose a reason for hiding this comment

Uh oh!

elharo Feb 1, 2021

Choose a reason for hiding this comment

Uh oh!

dpcollins-google Feb 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elharo Feb 1, 2021

Choose a reason for hiding this comment

Uh oh!

suztomo commented Apr 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

suztomo Apr 19, 2021

Choose a reason for hiding this comment

Uh oh!

suztomo Apr 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suztomo Apr 19, 2021

Choose a reason for hiding this comment

Uh oh!

suztomo Apr 19, 2021

Choose a reason for hiding this comment

Uh oh!

suztomo Apr 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov Bot commented Feb 1, 2021 •

edited

Loading

dpcollins-google Feb 1, 2021 •

edited

Loading

suztomo commented Apr 19, 2021 •

edited

Loading

suztomo Apr 19, 2021 •

edited

Loading

suztomo Apr 19, 2021 •

edited

Loading