Repository navigation
BatchSettings.builder().build() encourages failure by making required arguments optional #2542
Description
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)
Activity
garrettjonesgoogle commentedon Oct 20, 2017
What about giving everything defaults, so that it's ok if you only set one value?
pongad commentedon Oct 24, 2017
Proposal:
I think we should fix this by documentation.
Give error message so at least the user knows what we're complaining about.
Expose either
DEFAULT_BATCHING_SETTINGSordefaultBatchingSettings(), and recommend users to callbuilder.setBatchingSettings(DEFAULT_BATCHING_SETTINGS.toBuilder().setElementCountThreshold(100L).build()). This call will work and keep all other parameters default.WDYT?
garrettjonesgoogle commentedon Oct 26, 2017
I think we should just set defaults in
newBuilder().pongad commentedon Oct 27, 2017
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_SETTINGScan stay as is.BatchingSettings.DEFAULTdefaulted 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 commentedon Oct 27, 2017
I think the default for BatchingSettings should be the values for no batching (send one at a time).
kir-titievsky commentedon Oct 27, 2017
batching: set default settings to no-batching (#397)
deps: update dependency com.google.truth:truth to v1.4.3 (#2542)
chore: recursive comparison in hermetic build IT (#2542)
chore(deps): update dependency com.google.cloud:google-cloud-storage …
deps: update actions/github-script action to v8 (#2542)
deps: update actions/github-script action to v8 (#2542)
chore(main): release 2.57.1 (googleapis#2542)