add pubsub support for per-operation options

[devbww]

Add an Options options = {} argument to the constructor of the client class(es). Merge these options with the default options for the service, and store them as a member of the client class.

Add an Options options = {} argument to each operation within each client. These options should then be merged with the client options from above, and installed as the prevailing options for the duration of the operation by instantiating an internal::OptionsSpan.

You could then use internal::CurrentOptions() to obtain (a const& to) the prevailing options from anywhere you might need them. Any cleanup for call paths where Options have been passed explicitly is discretionary.

Similar support for the generated client classes was added in #7683, so you might be able to use that as an example.

[coryan]

I need this to fix the x-goog-user-project anyway.

[coryan]

What is supposed to happen to the options used to initialize a Connection?

[devbww]

What is supposed to happen to the options used to initialize a Connection?

At least they should be returned by the Options() accessor. After that, I'd say it is up to the service.

In the generated ConnectionImpls they are only used to get policies when CurrentOptions() doesn't have them (but that is only an allowance for testing here:

google-cloud-cpp/generator/internal/connection_impl_generator.cc

Lines 112 to 116 in 2a40c30

	// `CurrentOptions()` may not have the service default options because we
	// could be running in a test that calls the ConnectionImpl layer directly,
	// and it does not create an `internal::OptionsSpan` like the Client layer.
	// So, we have to fallback to `options_`, which we know has the service
	// default options because we added them.

).

In the Spanner ConnectionImpl though, they are used to check if rpc-streams tracing is enabled, and to create the SessionPool.

[coryan]

I have not been following the ins and outs of this feature as I should. This bug is asking me to:

Save all the options, with their defaults, at the Client level, and then set these options (and maybe the per-call options) using an internal::OptionsSpan. That means the options set at the connection level never have an effect. I recall that we changed our minds about how options-per-call are implemented, did you update this bug to reflect the latest approach?

In Pub/Sub some of the options are only used while constructing the *Connection + *Stub stack, it is impossible to override them for each RPC. For example, consider this:

google-cloud-cpp/google/cloud/pubsub/publisher_connection.cc

Lines 141 to 148 in 207a602

	std::vector<std::shared_ptr<pubsub_internal::PublisherStub>> children(
	opts.get<GrpcNumChannelsOption>());
	int id = 0;
	std::generate(children.begin(), children.end(), [&id, &opts, &auth] {
	return pubsub_internal::CreateDefaultPublisherStub(
	auth->CreateChannel(opts.get<EndpointOption>(),
	pubsub_internal::MakeChannelArguments(opts, id++)));
	});

or this:

google-cloud-cpp/google/cloud/pubsub/publisher_connection.cc

Lines 89 to 100 in 207a602

	if (opts.get<pubsub::MessageOrderingOption>()) {
	auto factory = [topic, opts, sink, cq](std::string const& key) {
	return pubsub_internal::BatchingPublisherConnection::Create(
	topic, opts, key,
	pubsub_internal::SequentialBatchSink::Create(sink), cq);
	};
	return pubsub_internal::OrderingKeyPublisherConnection::Create(
	std::move(factory));
	}
	return pubsub_internal::RejectsWithOrderingKey::Create(
	pubsub_internal::BatchingPublisherConnection::Create(
	std::move(topic), opts, {}, std::move(sink), std::move(cq)));

I suppose we would need to document which options can have an effect on a per-call basis?

[devbww]

I have not been following the ins and outs of this feature as I should. This bug is asking me to:

Save all the options, with their defaults, at the Client level, and then set these options (and maybe the per-call options) using an internal::OptionsSpan. That means the options set at the connection level never have an effect. I recall that we changed our minds about how options-per-call are implemented, did you update this bug to reflect the latest approach?

Yes, we did modify the procedure, and no, I did not update this bug [ENOMEM].

At the Client level we should now merge the defaulted Connection options into the argument options. That is, something along the lines of:

ServiceClient::ServiceClient(std::shared_ptr<ServiceConnection> connection, Options opts = {})
    : connection_(std::move(connection)),
      options_(internal::MergeOptions(std::move(opts), 
                                      service_internal::DefaultOptions(connection_->options()))) {
  ...
}

In Pub/Sub some of the options are only used while constructing the *Connection + *Stub stack, it is impossible to override them for each RPC.

Yes. That's the same for all services I reckon. In the Spanner example from above, the SessionPool options would fall into that category.

I suppose we would need to document which options can have an effect on a per-call basis?

Yes, I suppose so. I hope that it would be clear where particular options are applied, and hence when overriding values has an effect, but that may be assuming too much implementation knowledge from a user.

[devbww]

Addendum: Please also ensure that the OptionsSpan instantiated during ServiceClient::Operation() is saved and then restored over any work done on behalf of the operation but after Operation() returns. Typically this means for async, paginated, or streaming RPCs. (These cases may already be handled via changes in the common library.)

[coryan]

I am still unsure what to do about pubsub::Publisher and specially about pubsub::Subscriber. I do not think I will be making progress for the next few weeks.

[coryan]

After thinking some more about, and looking at the code, I don't think it makes sense to implement this feature for pubsub::Publisher. The issue is that most interesting options affect the stack of decorators in the pubsub::PublisherConnnection, they cannot be changed "per call". And the batching properties / ordering properties affect previous messages.

I am closing this.