Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add CredentialsProvider to Publisher and Subscriber settings #475

Merged
merged 10 commits into from
Feb 1, 2021

Conversation

dpcollins-google
Copy link
Collaborator

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
@dpcollins-google dpcollins-google requested a review from a team as a code owner February 1, 2021 03:46
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Feb 1, 2021
@product-auto-label product-auto-label bot added the api: pubsublite Issues related to the googleapis/java-pubsublite API. label Feb 1, 2021
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
@dpcollins-google dpcollins-google requested a review from a team as a code owner February 1, 2021 04:15
@codecov
Copy link

codecov bot commented Feb 1, 2021

Codecov Report

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

Impacted file tree graph

@@             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.

Copy link
Contributor

@manuelmenzella-google manuelmenzella-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if my comment is addressed.

@@ -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) -->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@dpcollins-google dpcollins-google Feb 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dpcollins-google dpcollins-google added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 1, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 1, 2021
elharo
elharo previously requested changes Feb 1, 2021
@@ -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) -->
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@dpcollins-google dpcollins-google dismissed elharo’s stale review February 1, 2021 15:07

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

@dpcollins-google dpcollins-google merged commit ba16af8 into master Feb 1, 2021
@dpcollins-google dpcollins-google deleted the creds_to_settings branch February 1, 2021 15:07
@suztomo
Copy link
Member

suztomo commented Apr 19, 2021

@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?

Comment on lines +236 to +242
Committer wireCommitter =
CommitterSettings.newBuilder()
.setSubscriptionPath(subscriptionPath())
.setPartition(partition)
.setServiceClient(newCursorServiceClient())
.build()
.instantiate();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

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

@suztomo suztomo Apr 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines -97 to 109
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);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PublisherServiceSettings.Builder settingsBuilder = PublisherServiceSettings.newBuilder();
settingsBuilder =
addDefaultMetadata(
PubsubContext.of(FRAMEWORK),
Copy link
Member

@suztomo suztomo Apr 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for myself. This sets the context now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsublite Issues related to the googleapis/java-pubsublite API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make it easier to use non defaults CredentialsProvider
5 participants