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

Cleanup Pub/Sub functions to create test *Connection classes #7438

Closed
Tracked by #6463
coryan opened this issue Oct 11, 2021 · 1 comment · Fixed by #7452
Closed
Tracked by #6463

Cleanup Pub/Sub functions to create test *Connection classes #7438

coryan opened this issue Oct 11, 2021 · 1 comment · Fixed by #7452
Labels
api: storage Issues related to the Cloud Storage API. type: cleanup An internal cleanup or hygiene concern.

Comments

@coryan
Copy link
Contributor

coryan commented Oct 11, 2021

To test the *Connection classes we have special functions in pubsub_internal:: that decorate a *Mock stub and then create the *Connection, for example:

std::shared_ptr<pubsub::PublisherConnection> MakePublisherConnection(
pubsub::Topic topic, Options opts,
std::vector<std::shared_ptr<PublisherStub>> stubs);

Their implementation can be quite involved:

std::shared_ptr<pubsub::PublisherConnection> MakePublisherConnection(
pubsub::Topic topic, Options opts,
std::vector<std::shared_ptr<PublisherStub>> stubs) {
if (stubs.empty()) return nullptr;
std::shared_ptr<PublisherStub> stub =
std::make_shared<PublisherRoundRobin>(std::move(stubs));
stub = std::make_shared<PublisherMetadata>(std::move(stub));
if (internal::Contains(opts.get<TracingComponentsOption>(), "rpc")) {
GCP_LOG(INFO) << "Enabled logging for gRPC calls";
stub = std::make_shared<PublisherLogging>(
std::move(stub), opts.get<GrpcTracingOptionsOption>());
}
auto background = internal::MakeBackgroundThreadsFactory(opts)();
auto make_connection = [&]() -> std::shared_ptr<pubsub::PublisherConnection> {
auto cq = background->cq();
std::shared_ptr<BatchSink> sink = DefaultBatchSink::Create(stub, cq, opts);
if (opts.get<pubsub::MessageOrderingOption>()) {
auto factory = [topic, opts, sink, cq](std::string const& key) {
return BatchingPublisherConnection::Create(
topic, opts, key, SequentialBatchSink::Create(sink), cq);
};
return OrderingKeyPublisherConnection::Create(std::move(factory));
}
return RejectsWithOrderingKey::Create(BatchingPublisherConnection::Create(
std::move(topic), opts, {}, std::move(sink), std::move(cq)));
};
auto connection = make_connection();
if (opts.get<pubsub::FullPublisherActionOption>() !=
pubsub::FullPublisherAction::kIgnored) {
connection = FlowControlledPublisherConnection::Create(
std::move(opts), std::move(connection));
}
return std::make_shared<pubsub::ContainingPublisherConnection>(
std::move(background), std::move(connection));
}

Unfortunately, this breaks down with the introduction of the *Auth decorators, where the channel is created by the GrpcAuthenticationStrategy, then we create the default *Stub and then we wrap said stub with the *Auth decorator and we need to preserve the BackgroundThreads object to be owned by the connection.

We should reorganize, and maybe rename, these functions to reflect the new structure.

@coryan coryan added api: storage Issues related to the Cloud Storage API. type: cleanup An internal cleanup or hygiene concern. labels Oct 11, 2021
@dbolduc
Copy link
Member

dbolduc commented Oct 11, 2021

Update: the below comment is no longer valid

I will just point out that this is slightly different in classes with >1 stub (Publisher, Subscriber) than those with only 1 (TopicAdmin, SchemaAdmin, SubscriptionAdmin). I deleted the test-only methods in classes with >1 stub (for example: here). Maybe I shouldn't have, but I did.

MakePublisherConnection implementation shown above is an implementation used by our user-facing methods and our test method.

Whereas we have two separate MakeSchemaAdminConnection methods that do the auth wrapping. One user-facing. One for testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the Cloud Storage API. type: cleanup An internal cleanup or hygiene concern.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants