fix: update the publisher batch settings#908
fix: update the publisher batch settings#908hannahrogers-google wants to merge 1 commit intomainfrom
Conversation
Updating the Publish Batch settings to be consistent with the rest of the CPS client libs.
There was a problem hiding this comment.
Just want to make sure it's consistent across languages (1 MB)
| static final long DEFAULT_ELEMENT_COUNT_THRESHOLD = 100L; | ||
| static final long DEFAULT_REQUEST_BYTES_THRESHOLD = 1000L; // 1 kB | ||
| static final Duration DEFAULT_DELAY_THRESHOLD = Duration.ofMillis(1); | ||
| static final long DEFAULT_REQUEST_BYTES_THRESHOLD = 1L * 1024L * 1024L; // 1 MB |
There was a problem hiding this comment.
Should it be 1 * 1000 * 1000? 1 MB as shown in go/cps-libs-defaults
Python just updated the docstring to 1 MB here: googleapis/python-pubsub#572
| static final long DEFAULT_REQUEST_BYTES_THRESHOLD = 1000L; // 1 kB | ||
| static final Duration DEFAULT_DELAY_THRESHOLD = Duration.ofMillis(1); | ||
| static final long DEFAULT_REQUEST_BYTES_THRESHOLD = 1L * 1024L * 1024L; // 1 MB | ||
| static final Duration DEFAULT_DELAY_THRESHOLD = Duration.ofMillis(10); |
There was a problem hiding this comment.
Upon reflection, I think we have to hold off on changing this value until v2.0 of the library. For users who are not hitting the batch size limit, this will represent a ~10x increase in publish latency if they are using the default settings. It will be hard for users to understand where that is coming from in the absence of client-side metrics.
|
I am going to close this pull request as kamal mentioned above, we are going to hold off on these changes. |
Updating the Publish Batch settings to be consistent with the rest of the CPS client libs.