Skip to content
This repository has been archived by the owner on Sep 26, 2023. It is now read-only.

BatchingSettings default value for elementCountThreshold #543

Closed
zhengzhu88 opened this issue May 31, 2018 · 3 comments
Closed

BatchingSettings default value for elementCountThreshold #543

zhengzhu88 opened this issue May 31, 2018 · 3 comments
Assignees
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. 🚨 This issue needs some love. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@zhengzhu88
Copy link

When creating a BatchingSettings object using the builder, if a user doesn't explicitly call builder.setElementCountThreshold(), the builder uses the default elementCountThreshold of 1. That means that the default behavior of the BatchingSettings object is to not batch at all. I spent a good 30 minutes trying to figure out my application wasn't batching messages even when I was using a BatchingSettings object. Can the default value be something greater than 1, just so that when the user tries to batch something, it doesn't look like the library is broken?

@JustinBeckwith JustinBeckwith added triage me I really want to be triaged. 🚨 This issue needs some love. labels Jun 8, 2018
@JustinBeckwith JustinBeckwith added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p2 Moderately-important priority. Fix may not be included in next release. and removed 🚨 This issue needs some love. triage me I really want to be triaged. labels Jun 20, 2018
@JustinBeckwith JustinBeckwith added the 🚨 This issue needs some love. label Nov 27, 2018
@landrito
Copy link

landrito commented Jan 2, 2019

@vam-google any updates on this?

@vam-google vam-google added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Jan 4, 2019
@vam-google
Copy link
Contributor

vam-google commented Jan 4, 2019

@zhengzhu88 This behavior is by design.

I believe that the default values for the batching properties have to be null, requiring setting all the threasholds before calling .build() on the builder (otherwise it would fail). It looks like it used to be the case.

Then the default values were chaned to "no batching by default", see #2542. Interestingly enough the older issue also mentions amount of minutes needed to figure out what was happening (it says 20 minutes). So it seems, the older defaults were better ones (it took 20 minutes, compared to 30 minutes now =).

At the same time we cannot provide default batching properties to anything else except "no batching" because gax-java is a very core library, which is used by many clients and will be used by even more in the future. Different clients have different "ideal default" values. The only default values which make sence in all possible scenarios are either null (i.e. require users to set everything explicitly or fail, this is what it used to be before #2542), or no-batching (this is what it is now).

Also max element count is not the only threshold BatchingSettings has, it has more:

  1. ElementCountThreshold (default 1 element);
  2. RequestByteThreshold (default 1 byte);
  3. DelayThreshold (default 1 milisecond).

All these three must coexist somehow, if we increase ElementCountThreshold to something greater than 1 (lets say 10), then probably the others should be increased as well. If so, then what should be the value, which is ok for all possible usages? No-batching seems the safest approach here.

But what is really missing here is the documentation. If we choose a default value, we must specify it explicitly in the documentation, so it does not confuse users. I believe the proper documentation should solve most of the problems here.

@vam-google
Copy link
Contributor

This was fixed by providing proper documentation for BatchingSettings in #639. Closing

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. 🚨 This issue needs some love. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

No branches or pull requests

4 participants