feat(pubsub): introduce PublisherOptionList (g::c::Options) #7273

dbolduc · 2021-09-03T22:07:48Z

Start of the work for #6306

Some choices:

used a private enum for FullPublisherAction. I considered having 3 bool options, which might conflict, but ultimately decided that although the enum is verbose, it is clearest. It is also consistent with this in spanner. (I could use a check on doxygen comments for enum values)
default values are just hardcoded in (instead of using constants which have hardcoded values). This is not what was done in bigtable, but it is what was done in spanner.
test for equality of default values instead of testing that default values are > 0. This is not what is done in publisher_options_test, but I personally prefer it.
forward declaration of PublisherOptions is ugly. Not sure how to avoid it though.

This change is

google-cloud-cpp-bot · 2021-09-03T22:09:55Z

Google Cloud Build Logs
For commit: 60d85e61d1216c6b9641e761f920962fe9e77bc9

Public Access Build Logs
Cloud Build Console (requires auth)
Kokoro Logs (requires auth)

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

codecov · 2021-09-03T22:18:11Z

Codecov Report

Merging #7273 (56efd26) into main (6750048) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #7273      +/-   ##
==========================================
+ Coverage   94.34%   94.36%   +0.02%     
==========================================
  Files        1317     1320       +3     
  Lines      114594   114688      +94     
==========================================
+ Hits       108108   108226     +118     
+ Misses       6486     6462      -24

Impacted Files	Coverage Δ
google/cloud/pubsub/internal/defaults.cc	`100.00% <100.00%> (ø)`
google/cloud/pubsub/internal/defaults_test.cc	`100.00% <100.00%> (ø)`
...ernal/flow_controlled_publisher_connection_test.cc	`100.00% <100.00%> (+1.51%)`	⬆️
google/cloud/pubsub/publisher.cc	`100.00% <100.00%> (ø)`
google/cloud/pubsub/publisher_options.cc	`100.00% <100.00%> (ø)`
google/cloud/pubsub/publisher_options.h	`100.00% <100.00%> (ø)`
google/cloud/pubsub/publisher_options_test.cc	`100.00% <100.00%> (+2.22%)`	⬆️
...cloud/pubsub/internal/subscription_session_test.cc	`98.27% <0.00%> (-0.25%)`	⬇️
google/cloud/completion_queue_test.cc	`96.95% <0.00%> (-0.20%)`	⬇️
... and 10 more

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 6750048...56efd26. Read the comment docs.

google-cloud-cpp-bot · 2021-09-04T04:52:27Z

Google Cloud Build Logs
For commit: 56efd264e18827558c840f9194f6254833af9d2c

Public Access Build Logs
Cloud Build Console (requires auth)
Kokoro Logs (requires auth)

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

coryan · 2021-09-04T13:37:51Z

google/cloud/pubsub/options.h

+ * @note Application developers should keep in mind that Cloud Pub/Sub
+ *    sets [limits][pubsub-quota-link] on the size of a batch (1,000 messages)
+ *    The library makes no attempt to validate the value provided in this
+ *    function.


s/function/option/ ?

coryan · 2021-09-04T13:38:03Z

google/cloud/pubsub/options.h

+ * @note Application developers should keep in mind that Cloud Pub/Sub
+ *    sets [limits][pubsub-quota-link] on the size of a batch (10MB). The
+ *    library makes no attempt to validate the value provided in this
+ *    function.


coryan · 2021-09-04T13:48:11Z

google/cloud/pubsub/options.h

+};
+
+/// Actions taken by a full publisher.
+enum class FullPublisherAction {


Hmmm... I was trying to not make these enums public, I cannot recall why. Maybe because I hate enum:

They are the devil to evolve

They are a code smell for "somebody is using switch when they should be using virtual functions"

But I cannot recall why I did not want them in this case.... Can we hide them in an opaque type?

class FullPublisherAction { public: // maybe == and operator!= static FullPublisherAction BlockIfFull() { return FullPublisherAction(kBlocks); } static FullPublisherAction RejectIfFull() { return FullPublisherAction(kRejects); } static FullPublisherAction None() { return FullPublisherAction(kIgnored); } private: enum Discriminant { kIgnored, kRejects, kBlocks }; explicit FullPublisherAction(Discriminant d) : discriminant_(d) {} friend ... internal::DoStuff(FullPublisherAction a); };

I am not convinced that a public enum is a bad idea, but I am not convinced it is a good idea. Can you provide an argument one way or the other?

So quick thoughts are that

user code doesn't change much either way.

opts.set<FullPublisherActionOption>(FullPublisherAction::kBlockIfFull); // or kBlocks // vs. opts.set<FullPublisherActionOption>(FullPublisherAction::BlockIfFull());

Spanner has a similar enum

google-cloud-cpp/google/cloud/spanner/options.h

Lines 117 to 125 in 6750048

/// Action to take when the session pool is exhausted.

enum class ActionOnExhaustion { kBlock, kFail };

/**

* Option for `google::cloud::Options` to set the action to take when

* attempting to allocate a session when the pool is exhausted.

*/

struct SessionPoolActionOnExhaustionOption {

using Type = spanner::ActionOnExhaustion;

};

I'm going to guess it's easier to deprecate a method from the class than it is to deprecate a value from the enum. Also it seems slightly better to grow a class by adding methods than by appending values to an enum.

Currently, the value is used as an enum. Would it ever grow? For example something that could take in a callback? Like do generic user provided action when Publisher is full, instead of one of the three actions we have listed for you.

the class is more complex for us to write. It is ever so slightly more complicated for a user to understand.

If there is an action to be taken, other than the ones we list, then the class makes sense. Here is a stupid one:

static FullPublisherAction RejectIfFullWithProbability(double p);

What might a real action be?

Let's go with the public enum.

coryan · 2021-09-04T13:48:55Z

google/cloud/pubsub/publisher_options.h

@@ -46,7 +60,19 @@ inline namespace GOOGLE_CLOUD_CPP_PUBSUB_NS {
 */
 class PublisherOptions {
 public:
-  PublisherOptions() = default;
+  PublisherOptions() : PublisherOptions(Options{}) {}
+  /**


nit: blank line before comment block

coryan · 2021-09-04T13:49:57Z

google/cloud/pubsub/publisher.cc

-                     PublisherOptions)
+Publisher::Publisher(
+    std::shared_ptr<PublisherConnection> connection,
+    PublisherOptions)  // NOLINT(performance-unnecessary-value-param)


Q: do you know why this is needed now?

A: Honestly, no... clang-tidy yelled at me, so I changed it. (It also yelled at me to make the change in flow_controlled_publisher_connection_test.cc)

dbolduc · 2021-09-05T03:15:32Z

I wanted to get this done but I do not have the patience to set up a development workstation on my windows laptop right now. So I am temporarily closing this, to be reopened in one week.

refactor(pubsub): implement PublisherOptions using g::c::Options

60d85e6

product-auto-label bot added the api: pubsub Issues related to the Pub/Sub API. label Sep 3, 2021

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 3, 2021

fix for clang 60, similar to ToProto() in message.h

56efd26

dbolduc changed the title ~~refactor(pubsub): implement PublisherOptions using g::c::Options~~ feat(pubsub): introduce PublisherOptionList (g::c::Options) Sep 4, 2021

dbolduc marked this pull request as ready for review September 4, 2021 04:58

dbolduc requested a review from a team as a code owner September 4, 2021 04:58

coryan suggested changes Sep 4, 2021

View reviewed changes

dbolduc closed this Sep 5, 2021

dbolduc mentioned this pull request Sep 13, 2021

feat(pubsub): introduce PublisherOptionList (g::c::Options) #7308

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(pubsub): introduce PublisherOptionList (g::c::Options) #7273

feat(pubsub): introduce PublisherOptionList (g::c::Options) #7273

dbolduc commented Sep 3, 2021 •

edited

Loading

google-cloud-cpp-bot commented Sep 3, 2021

codecov bot commented Sep 3, 2021 •

edited

Loading

google-cloud-cpp-bot commented Sep 4, 2021

coryan Sep 4, 2021

coryan Sep 4, 2021

coryan Sep 4, 2021

coryan Sep 4, 2021

dbolduc Sep 4, 2021

dbolduc Sep 4, 2021

coryan Sep 4, 2021

coryan Sep 4, 2021

coryan Sep 4, 2021

dbolduc Sep 13, 2021

dbolduc commented Sep 5, 2021

	/// Action to take when the session pool is exhausted.
	enum class ActionOnExhaustion { kBlock, kFail };
	/**
	* Option for `google::cloud::Options` to set the action to take when
	* attempting to allocate a session when the pool is exhausted.
	*/
	struct SessionPoolActionOnExhaustionOption {
	using Type = spanner::ActionOnExhaustion;
	};

feat(pubsub): introduce PublisherOptionList (g::c::Options) #7273

feat(pubsub): introduce PublisherOptionList (g::c::Options) #7273

Conversation

dbolduc commented Sep 3, 2021 • edited Loading

google-cloud-cpp-bot commented Sep 3, 2021

codecov bot commented Sep 3, 2021 • edited Loading

Codecov Report

google-cloud-cpp-bot commented Sep 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbolduc commented Sep 5, 2021

dbolduc commented Sep 3, 2021 •

edited

Loading

codecov bot commented Sep 3, 2021 •

edited

Loading