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

[BEAM-53] Pub/sub client with gRPC implementation #120

Closed
wants to merge 6 commits into from
Closed

[BEAM-53] Pub/sub client with gRPC implementation #120

wants to merge 6 commits into from

Conversation

mshields822
Copy link
Contributor

Support pub/sub via gRPC.
Will later use for pure Java pub/sub source/sink.

@mshields822
Copy link
Contributor Author

R: @kennknowles

import java.util.Collection;

/**
* A helper interface for talking to pub/sub via an underlying transport.
Copy link
Member

Choose a reason for hiding this comment

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

I think the official name is "Pubsub", no slash, capital P, lowercase 's'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

/**
* Construct a new Pubsub grpc client. It should be closed via {@link #close} in order
* to ensure tidy cleanup of underlying netty resources. (Or use the try-with-resources
* construct since this class is {@link AutoCloseable}. If non-{@literal null}, use
Copy link
Member

Choose a reason for hiding this comment

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

close paren

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@kennknowles
Copy link
Member

Looks like checkstyle dislikes your copyright headers.

@mshields822
Copy link
Contributor Author

Could do with a squash but this addresses your comments and supports some quirks needed later by the dataflow runner.

@mshields822
Copy link
Contributor Author

Looking at the misc support code in PubsubIO thinks would be much clearer if we also had a PubsubApiaryClient and all the regex and other misc handling in PubsubIO was moved into PubsubClient.

BUT I'd like to make steady progress, so how about we take that as a TODO?

@kennknowles
Copy link
Member

LGTM. Merging.

@asfgit asfgit closed this in 46c82ac Apr 12, 2016
/**
* A helper interface for talking to Pubsub via an underlying transport.
*/
public interface PubsubClient extends AutoCloseable {
Copy link
Member

Choose a reason for hiding this comment

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

It occurs to me that this PR would really benefit from a follow-up with an in-memory testing fake, with a test suite that can be applied to both it and the gRPC implementation.

@davorbonaci
Copy link
Member

I think this PR might not have received the attention it deserves:

  • We are adding new dependencies. Have we considered the impact on shading and leakages? Any impact on the dependency tree? How about transitive dependencies?
  • We are adding new public APIs in the io package. Perhaps it would be more appropriate for the util package, or package-visible API.
  • We seem to be missing tests for these new APIs.
  • We seem to be importing repackaged Preconditions. Is there a reason to?
  • Etc.

Since this is IO-related, @dhalperi would be a really good reviewer. @mshields822, can I ask you to get these things fixed up?


package com.google.cloud.dataflow.sdk.io;

import com.google.api.client.repackaged.com.google.common.base.Preconditions;
Copy link
Member

Choose a reason for hiding this comment

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

Whoops

@kennknowles
Copy link
Member

Package-private would be better than util/, and it does seem possible for now at least.

@mshields822
Copy link
Contributor Author

No worries:

  1. Flink can't handle gRPC without special magic (openSSL dep problems), so that's the last straw for me with gRCP. I'm going to add a PubsubApiaryClient and see what I can do to bring the gRPC deps back under control.
  2. Only reason any of this is public is so the Nexmark driver can piggy back on it for all its pubsub create/list/delete stuff. Davor suggested util/ is a happy place for this sort of code, will follow that.
  3. Repackaged slipped through an auto-complete. Sorry about that.
  4. Hard to unit test this and I didn't want to go to town on integration or service tests, especially since it is much easier to test at the pipeline level than the raw client level.
    Stand by.

@mshields822
Copy link
Contributor Author

Also, the extra deps (beyond grpc-pubsub-v1) are all needed by the gRPC client. They are also included by the grpc-pubsub-v1 dep, but our transitive dependency check requires them to be included directly. You can find the same deps inside the bigtable-client-core pom. So the only new dep here is on grpc-pubsub-v1 itself.

@mshields822
Copy link
Contributor Author

This work continues in
#213

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants