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: All changes required to change library to use ApiException and gapic internals #295

Merged
merged 14 commits into from
Oct 26, 2020

Conversation

dpcollins-google
Copy link
Collaborator

@dpcollins-google dpcollins-google commented Oct 16, 2020

This converts the internals of the client library to use GAPIC generated clients, as well as the api surface to raise ApiException as opposed to grpc StatusException.

fixes #180

fixes #148

This does not throw RESOURCE_EXHAUSTED at high partition counts with the following code when removing RESOURCE_EXHAUSTED from the list of retryable errors:

public class PublisherTest {
  public static void main(String[] args) throws Exception {
    Publisher publisher = Publisher.create(PublisherSettings.newBuilder()
        .setTopicPath(TopicPath.parse(<400 partition topic>)).build());
    publisher.startAsync().awaitRunning();
    publisher.publish(PubsubMessage.newBuilder().setData(ByteString.copyFromUtf8("abc")).build()).get();
    publisher.stopAsync().awaitTerminated();
  }
}

@dpcollins-google dpcollins-google added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Oct 16, 2020
@dpcollins-google dpcollins-google requested a review from a team as a code owner October 16, 2020 03:01
@dpcollins-google dpcollins-google requested a review from a team October 16, 2020 03:01
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 16, 2020
@product-auto-label product-auto-label bot added the api: pubsublite Issues related to the googleapis/java-pubsublite API. label Oct 16, 2020
@dpcollins-google dpcollins-google added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 19, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 19, 2020
@dpcollins-google dpcollins-google added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 19, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 19, 2020
@dpcollins-google dpcollins-google added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 19, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 19, 2020
@dpcollins-google dpcollins-google removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Oct 20, 2020
@anguillanneuf anguillanneuf requested a review from a team October 21, 2020 23:09
@dpcollins-google dpcollins-google changed the title fix: All changes required to change library to use ApiException and gapic internals feat: All changes required to change library to use ApiException and gapic internals Oct 22, 2020
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.

Thanks for making this PR. I didn't review any of the tests. Here are some issues I found:

  1. This comment about setting stub on the publisher needs an update:
    * <p>Custom batching settings and a custom GRPC stub can also be set.
  2. I'm not sure if you intended google-cloud-pubsublite/src/main/java/com/google/cloud/pubsublite/internal/testing/UnitTestExamples.java to be there.
  3. Sporadic class descriptions in google-cloud-pubsublite/src/main/java/com/google/cloud/pubsublite/internal/wire/.
  4. Changes in pubsublite-beam-io/ and pubsublite-kafka-shim/ should be their own PRs.
  5. Changes to grpc-google-cloud-pubsublite-v1/ should come from synthtool pulling from the source.
  6. Update samples in a later PR.
  7. Also please resolve the conflicts with main.

I haven't tried the code to see how they impact samples myself. I can do that next week.

@dpcollins-google
Copy link
Collaborator Author

Thanks for making this PR. I didn't review any of the tests. Here are some issues I found:

Replies to each point below.

  1. Done.
  2. Yes, since this needs to be accessed from other modules.
  3. This is not a public API, thus it can be put off until later as the only users of these classes would be maintainers.
  4. No, since they would not compile otherwise.
  5. I've reverted these. They were unintentional.
  6. No, we've discussed this previously. For the integration tests to pass as currently set up, samples changes need to be made in the same PR, since the blocking presubmit is the snapshot one.
  7. Done.

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.

  1. It's in the public documentation, either we add more documentation there, or we choose to hide it. Can be in a later PR.
  2. Ok. Make sure to make a new release soon after this PR is merged then.

@dpcollins-google dpcollins-google merged commit 313bfc6 into master Oct 26, 2020
@dpcollins-google dpcollins-google deleted the change-to-gapic branch October 26, 2020 18:25
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.

Consider converting internals to use GAPIC layer Better exception for AdminClient
4 participants