ChangeFeedProcessorFix - IllegalArgumentException #34618

xinlian12 · 2023-04-24T19:02:55Z

Issue:
when customer configure ChangeFeedProcessor to use maxScaleCount, and when split happens, there is a possibility each changeFeedInstance will own more than maxScaleCount. An IllegalArgumentException can happen during load balancing stage when maxScaleCount is being configured. And then due to the exception, the configured AcquiredInterval will be ignored, which can cause request spike on the lease container.

Fix:

EqualPartitionsBalancingStrategyIf - if it ever happens that the change feed processor instance owns more than maxScaleCount leases, no illegalArgumentException will be thrown
PartitionLoadBalancerImpl - the acquireInterval will be used even for error cases
PartitionLoadBalancerImpl - skip child leases assignment if maxScaleCount > 0

azure-sdk · 2023-04-24T19:12:52Z

API change check

API changes are not detected in this pull request.

kushagraThapar

LGTM, thanks @xinlian12

...java/com/azure/cosmos/implementation/changefeed/common/EqualPartitionsBalancingStrategy.java

...in/java/com/azure/cosmos/implementation/changefeed/epkversion/PartitionLoadBalancerImpl.java

...ain/java/com/azure/cosmos/implementation/changefeed/pkversion/PartitionLoadBalancerImpl.java

kushagraThapar

Wait, missing changelog entry :)

FabianMeiswinkel

LGTM - thanks!

milismsft · 2023-04-24T20:50:51Z

@xinlian12 per our discussion, I do not see the splitting case addressed here when we detect "maxScaleCount" > 0.

xinlian12 · 2023-04-24T20:55:12Z

@xinlian12 per our discussion, I do not see the splitting case addressed here when we detect "maxScaleCount" > 0.
no, it does not. That change will come in a second PR - to always honor the maxScaleCount

...c/main/java/com/azure/cosmos/implementation/changefeed/epkversion/LeaseStoreManagerImpl.java

...azure-cosmos/src/main/java/com/azure/cosmos/implementation/changefeed/LeaseStoreManager.java

milismsft

We should expand our E2E tests to include this maxScaleCount case where the negative partitionNeededForMe was an issue.
You can start with a feed container of at least 3 partitions and one instance of CFP (max scale count will be 3). Then you stop and start another instance with exact same name but smaller maxScaleCount. At the same time have another instance that will be started as well. At the end you want to check that leases are equally distributed and whatever documents are fed into the feed container are accounted for (see https://github.com/Azure/azure-sdk-for-java/blob/main/sdk/cosmos/azure-cosmos-tests/src/test/java/com/azure/cosmos/rx/changefeed/pkversion/IncrementalChangeFeedProcessorTest.java#L939).

xinlian12 · 2023-04-27T22:02:20Z

We should expand our E2E tests to include this maxScaleCount case where the negative partitionNeededForMe was an issue. You can start with a feed container of at least 3 partitions and one instance of CFP (max scale count will be 3). Then you stop and start another instance with exact same name but smaller maxScaleCount. At the same time have another instance that will be started as well. At the end you want to check that leases are equally distributed and whatever documents are fed into the feed container are accounted for (see https://github.com/Azure/azure-sdk-for-java/blob/main/sdk/cosmos/azure-cosmos-tests/src/test/java/com/azure/cosmos/rx/changefeed/pkversion/IncrementalChangeFeedProcessorTest.java#L939).

Added tests.
What has included in the test:

Create a changeFeedProcessor with maxScaleCount = partitionCountBeforeSplit
write items, and validate the change feed processor reads all documents
trigger a split
write items
validate only partitionCountBeforeSplit leases have owner
Start another change feed processor
Validate all documents are processed

xinlian12 · 2023-04-27T22:03:04Z