BatchSettings.builder().build() encourages failure by making required arguments optional

[kir-titievsky]

Current pubsub Publisher settings seem to require an instance of BatchingSettings with multiple values set to non-null values. However, the builder pattern encourages setting a single setting at a time. For example, I wanted to make sure that messages were batched into 100 messages batches. So I did this:

publisher = Publisher.defaultBuilder(topicName).setBatchingSettings( BatchingSettings.newBuilder().setElementCountThreshold(100L).build()).build();
which fails with a NullPointerException .
Turns out I must set all other values of BatchingSettings to set one since the call above produces an object like this:
BatchingSettings{elementCountThreshold=100, requestByteThreshold=null, delayThreshold=null, isEnabled=true, flowControlSettings=FlowControlSettings{maxOutstandingElementCount=null, maxOutstandingRequestBytes=null, limitExceededBehavior=Ignore}}

Which took me a good 20 minutes to figure out.

What I wish we did here was an API that required all required parameters, like so (for example):
BatchingSettings.newBuilder().setThresholds(elementCount, requestByte, delay)

[garrettjonesgoogle]

What about giving everything defaults, so that it's ok if you only set one value?

[pongad]

Proposal:
I think we should fix this by documentation.

1. Give error message so at least the user knows what we're complaining about.

2. Expose either DEFAULT_BATCHING_SETTINGS or defaultBatchingSettings(), and recommend users to call builder.setBatchingSettings(DEFAULT_BATCHING_SETTINGS.toBuilder().setElementCountThreshold(100L).build()). This call will work and keep all other parameters default.

WDYT?

[garrettjonesgoogle]

I think we should just set defaults in newBuilder().

[pongad]

Note that there are two defaults at play here. One is BatchingSettings.DEFAULT and another is Subscriber.DEFAULT_BATCHING_SETTINGS.

I think Subscriber.DEFAULT_BATCHING_SETTINGS can stay as is.

BatchingSettings.DEFAULT defaulted to null for "never trigger". Eg, if you set element count to null, batch will only trigger on number of bytes and duration. What should default be here, Long.MAX?

[garrettjonesgoogle]

I think the default for BatchingSettings should be the values for no batching (send one at a time).

[kir-titievsky]

I vote against addressing this in documentation only. It's like hanging an "open" sign on a locked door with a little note at the bottom that says "JK. We are closed for the day."

On Fri, Oct 27, 2017 at 12:45 AM Garrett Jones ***@***.***> wrote: I think the default for BatchingSettings should be the values for no batching (send one at a time). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2542 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ARrMFuwzmeXdjz9k-SWXbQNMX_As9VR8ks5swV_UgaJpZM4QA7ZM> .

-- Kir Titievsky | Product Manager | Google Cloud Pub/Sub <https://cloud.google.com/pubsub/overview>