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 the ability to use automatic subscriber assignment to the subscriber settings #163

Merged
merged 4 commits into from
Aug 11, 2020

Conversation

dpcollins-google
Copy link
Collaborator

No description provided.

…subscriber settings.

Also disable clirr as it fires on this PR. We are not providing interface stability guarantees.
@dpcollins-google dpcollins-google added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jul 15, 2020
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 15, 2020
@anguillanneuf anguillanneuf changed the title [feat] Add the ability to use automatic subscriber assignment to the subscriber settings. feat: Add the ability to use automatic subscriber assignment to the subscriber settings Jul 15, 2020
@anguillanneuf
Copy link
Contributor

Please fix the tests first.

Copy link
Contributor

@anguillanneuf anguillanneuf left a comment

Choose a reason for hiding this comment

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

The changes in samples/ looks fine. Please also update .readme-partials.yaml and fix tests.

@@ -46,7 +46,7 @@
<dependency>
<groupId>com.google.cloud</groupId>
<artifactId>google-cloud-pubsublite</artifactId>
<version>0.1.7</version>
<version>0.1.9-SNAPSHOT</version>
Copy link
Contributor

@anguillanneuf anguillanneuf Jul 15, 2020

Choose a reason for hiding this comment

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

Please don't update this to SNAPSHOT as this will get mirrored in https://cloud.google.com/pubsub/lite/docs/reference/libraries
Generally, autosynth will take care of updating version numbers in POM files.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Autosynth doesn't seem to be updating this number? At least it didnt in release 1.8 which is confusing.

Copy link
Contributor

@anguillanneuf anguillanneuf Jul 15, 2020

Choose a reason for hiding this comment

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

It may have something to do with most snippets folders usually don't require version updates / they use libraries bom. @chingor13 is that right? See history: https://github.com/googleapis/java-pubsublite/commits/master/samples/snippets/pom.xml How do we overwrite the update rule?


We never successfully released any version after 0.1.7: https://mvnrepository.com/artifact/com.google.cloud/google-cloud-pubsublite

google-cloud-pubsublite/pom.xml Show resolved Hide resolved
@dpcollins-google
Copy link
Collaborator Author

Please fix the tests first.

The tests are broken due to "[08:14:28][ERROR] Timed out waiting for GitHub to return mergeable status for the pull (PR). GitHub is taking too long to compute whether this PR is mergeable. Please retry the build in a few minutes. You can do this by attaching a kokoro:force-run label on your PR." which sounds like a kokoro issue.

@anguillanneuf anguillanneuf added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 15, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 15, 2020
@dpcollins-google dpcollins-google force-pushed the sub-settings branch 2 times, most recently from afededa to f8039c7 Compare July 15, 2020 20:38
@dpcollins-google
Copy link
Collaborator Author

.readme-partials.yaml updated.

@anguillanneuf
Copy link
Contributor

Samples update should come later, after the library has a new release that picks up the changes. If you want, you can split this into two. Or I can update the samples later.

@codecov
Copy link

codecov bot commented Jul 15, 2020

Codecov Report

Merging #163 into master will decrease coverage by 1.09%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #163      +/-   ##
============================================
- Coverage     61.16%   60.07%   -1.10%     
  Complexity      417      417              
============================================
  Files            92       93       +1     
  Lines          2145     2184      +39     
  Branches        181      182       +1     
============================================
  Hits           1312     1312              
- Misses          736      775      +39     
  Partials         97       97              
Impacted Files Coverage Δ Complexity Δ
...oud/pubsublite/cloudpubsub/SubscriberSettings.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...loud/pubsublite/internal/wire/AssignerBuilder.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)

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 3c471ab...214af54. Read the comment docs.

Copy link
Contributor

@anguillanneuf anguillanneuf left a comment

Choose a reason for hiding this comment

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

I would like @hannahrogers-google or someone else comment on if more tests need to be added before you merge.

@anguillanneuf anguillanneuf self-requested a review July 15, 2020 22:53
@hannahrogers-google
Copy link
Contributor

I would like @hannahrogers-google or someone else comment on if more tests need to be added before you merge.

I am ok with this change - I don't believe we have explicit unit tests for any of the Builders in the lite client library.

Copy link
Contributor

@anguillanneuf anguillanneuf left a comment

Choose a reason for hiding this comment

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

Cool. I can update samples in a different PR a bit later. Thanks!

@anguillanneuf anguillanneuf self-requested a review July 16, 2020 20:37
@anguillanneuf anguillanneuf dismissed their stale review July 16, 2020 20:38

Changes applied.

Copy link

@lesv lesv left a comment

Choose a reason for hiding this comment

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

This doesn't appear to have a sample - so I'm just approving w/o review.

@anguillanneuf
Copy link
Contributor

@dpcollins-google Does this Do Not Merge label still apply?

@dpcollins-google
Copy link
Collaborator Author

@dpcollins-google Does this Do Not Merge label still apply?

Yes it does. We don't want to merge this until the backend is tested internally and we've updated (/ are updating) the docs.

@dpcollins-google dpcollins-google removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 11, 2020
@dpcollins-google dpcollins-google merged commit a396f24 into master Aug 11, 2020
@dpcollins-google dpcollins-google deleted the sub-settings branch August 11, 2020 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants