Support min/max duration per ack extension for pubsub #1254
Conversation
…Extension` in PubSubConfiguration.
… in default subscriber factory
…Extension` in DefaultSubscriberFactory
…Extension` in GcpPubSubAutoConfiguration
There was a problem hiding this comment.
Really good test coverage! Added the first round of comments:) It might also be worth including these properties in PubSubAutoConfigurationIntegrationTests
...cp-pubsub/src/main/java/com/google/cloud/spring/pubsub/support/DefaultSubscriberFactory.java
Outdated
Show resolved
Hide resolved
...cp-pubsub/src/main/java/com/google/cloud/spring/pubsub/support/DefaultSubscriberFactory.java
Outdated
Show resolved
Hide resolved
...d-gcp-pubsub/src/test/java/com/google/cloud/spring/pubsub/core/PubSubConfigurationTests.java
Show resolved
Hide resolved
...bsub/src/test/java/com/google/cloud/spring/pubsub/support/DefaultSubscriberFactoryTests.java
Show resolved
Hide resolved
...bsub/src/test/java/com/google/cloud/spring/pubsub/support/DefaultSubscriberFactoryTests.java
Show resolved
Hide resolved
| @@ -552,13 +553,61 @@ void subscriberExecutorProvider_globalAndSelectiveConfigurationSet_selectiveTake | |||
| }); | |||
| } | |||
|
|
|||
| @Test | |||
| void pullConfig_defaultConfigurationSet() { | |||
There was a problem hiding this comment.
How about we also add assertions for other pull configs? I don't think we have a similar test for checking the default value for those anywhere.
There was a problem hiding this comment.
Done in later commits.
Great advice, in fact, I found out that the default value for parallelPullCount and pullEndpoint are both null, while in pubsub docs the default value should be 1 and pubsub.googleapis.com:443, respectively. Any reasons about this?
There was a problem hiding this comment.
We should avoid setting defaults for the user, even if they are the same as the client library. There was at least one instance in which the client library changed the default, but Spring Cloud GCP did not, and it led to a hard-to-troubleshoot issue.
Best practice is to allow null in Properties, so that we only modify client library builders if the user explicitly set some option.
There was a problem hiding this comment.
I looked at original code, and it looks like we avoid setting our own defaults in parallelPullCount and computeRetryableCodes, but not for computeMaxAckExtensionPeriod, which you naturally used as a reference point. There was probably a reason for it, but see if you can manage to pass nulls back up to PublisherFactory for the new properties -- that would be better.
There was a problem hiding this comment.
Do you mean SubscriberFactory because new properties are subscriber settings. To double check, you think we should set the default value of two new properties to null rather than 0?
There was a problem hiding this comment.
Sorry, yes, SubscriberFactory.
Leaving the value of wrapper types unset will automatically make them null. See if you can avoid providing a default value and allow the null to get returned through the layers.
There was a problem hiding this comment.
Done.
Now the default value for minDurationPerAckExtension and maxDurationPerAckExtension is null.
I left the default value in docs as is (i.e., 0). I'm not sure whether I should also change it to null, since most default values are not what they actually are.
.../test/java/com/google/cloud/spring/autoconfigure/pubsub/GcpPubSubAutoConfigurationTests.java
Outdated
Show resolved
Hide resolved
.../test/java/com/google/cloud/spring/autoconfigure/pubsub/GcpPubSubAutoConfigurationTests.java
Outdated
Show resolved
Hide resolved
...cp-pubsub/src/main/java/com/google/cloud/spring/pubsub/support/DefaultSubscriberFactory.java
Outdated
Show resolved
Hide resolved
…on` and `maxDurationPerAckExtension`.
Sorry I missed your comments last week. I modified the integration test to verify the properties. |
|
Kudos, SonarCloud Quality Gate passed! |
JoeWang1127
deleted the
feature/support-minDurationPerAckExtension-for-pubsub
branch
…tform#1254) * add support for `minDurationPerAckExtension` and `maxDurationPerAckExtension`. * add unit test for `minDurationPerAckExtension` and `maxDurationPerAckExtension` in PubSubConfiguration. * support `minDurationPerAckExtension` and `maxDurationPerAckExtension` in default subscriber factory * add unit test for `minDurationPerAckExtension` and `maxDurationPerAckExtension` in DefaultSubscriberFactory * add unit test for `minDurationPerAckExtension` and `maxDurationPerAckExtension` in GcpPubSubAutoConfiguration * change description * change docs for pubsub * revert code factorization, leave it to another PR * change method docs * refactor code after code review * remove setters for newly added parameters * change unit tests to set customer settings using pubsub config * add comments about setting default value on `minDurationPerAckExtension` and `maxDurationPerAckExtension`. * change new properties' default value to `null` * modify unit tests relevant to default value of new properties * test new properties in integration tests
Support
minDurationPerAckExtensionandmaxDurationPerAckExtensionfor pubsub.Note that the validation logic is NOT implemented in this PR, because the server will respond an exception if the two parameters are invalid, e.g., either of the parameters is negative or
minDurationPerAckExtensionis larger thanmaxDurationPerAckExtension. I add some unit tests to check the server's behavior so that these tests will fail if the sever removed or changed the validation logic in future version.