ThroughputControl-Global #19183

xinlian12 · 2021-02-11T17:05:22Z

This is the second PR of the throughput control.
Compared to local control mode, the throughput control group is shared among different clients.

TODO:

CTL run

Benchmark tests results:

# Throughput control part 1 -- local # refactor # refactor # refactor class name Throughput control part 1 -- Local control

q1 :q!

azure-pipelines · 2021-02-26T15:52:51Z

Azure Pipelines successfully started running 1 pipeline(s).

xinlian12 · 2021-02-26T22:14:54Z

/azp run java - cosmos - tests

azure-pipelines · 2021-02-26T22:15:09Z

Azure Pipelines successfully started running 1 pipeline(s).

...src/main/java/com/azure/cosmos/implementation/throughputControl/LinkedCancellationToken.java

sdk/cosmos/azure-cosmos-spark_3-0_2-12/scalastyle-output.xml

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/GlobalThroughputControlConfig.java

...cosmos/azure-cosmos/src/main/java/com/azure/cosmos/GlobalThroughputControlConfigBuilder.java

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/GlobalThroughputControlConfig.java

...a/com/azure/cosmos/implementation/throughputControl/config/ThroughputGlobalControlGroup.java

...va/com/azure/cosmos/implementation/throughputControl/config/ThroughputLocalControlGroup.java

sdk/cosmos/azure-cosmos/src/samples/java/com/azure/cosmos/ThroughputControlCodeSnippet.java

.../src/main/java/com/azure/cosmos/implementation/throughputControl/ThroughputControlStore.java

moderakh · 2021-02-26T22:49:50Z

.../src/main/java/com/azure/cosmos/implementation/throughputControl/ThroughputControlStore.java

    private final RxPartitionKeyRangeCache partitionKeyRangeCache;

+    private final LinkedCancellationTokenSource cancellationTokenSource;
+    private final ConcurrentHashMap<String, LinkedCancellationToken> cancellationTokenMap;


ditto, on the thread-safely.

Will discuss offline for this one.

.../src/main/java/com/azure/cosmos/implementation/throughputControl/ThroughputControlStore.java

.../main/java/com/azure/cosmos/implementation/throughputControl/ThroughputRequestThrottler.java

.../src/test/java/com/azure/cosmos/implementation/throughputControl/ThroughputControlTests.java

moderakh · 2021-02-26T23:16:55Z

.../main/java/com/azure/cosmos/implementation/throughputControl/ThroughputRequestThrottler.java

-            if (request.requestContext != null) {
-                BridgeInternal.setResourceAddress(requestRateTooLargeException, request.requestContext.resourcePhysicalAddress);
+        try {
+            this.throughputReadLock.lock();


here mixing the readLock pattern with reactor Mono is interesting, and I am not sure if it does what you intended to do. thinking ...

Think about this,

let's say you acquire the read lock this.throughputReadLock.lock() and then we check if (this.availableThroughput.get() > 0)

then we are returning aMono from the processRequest(.) method. which means as we go out of the scope of this method the finally block and this.throughputReadLock.unlock(); will be invoked. so now the lock is released.

now trackRequestCharge and trackRequestCharge won't be invoked till we go out of the scope of processRequest and someone subscribes to the returned Mono.

I checked the trackRequestCharge and trackRequestCharge implementation I noticed that you are re-accquiring the read lock.

Now please consider this scenario: does this cause any problem?

processRequest acquires the lock this.throughputReadLock.lock()and read the value ofavailableThroughput`

processRequest returns a Mono and release the this.throughputReadLock.lock() lock in the finally block

now let's say some other thread acquires the write lock and updates the throughputs

someone subsctibes to the Mono returned in step 2, and now trackRequestCharge and trackRequestCharge will be invoked, they will attempt to accquire readLock but because of the step 3. in between the throughput values they see is not the same value that the if branch of processRequest observed.

Does this cause any problem?

@moderakh @FabianMeiswinkel @kushagraThapar

I think this is one of the consequences of we only track the RU usage based on the response compared to pre-calculation.

The consequences of it :
a. could be some requests actually happened in the previous cycle their response will be tracked in the next cycle and eat some throughput from the next cycle.
b. The throughput usage for previous cycle is lower than the real usage.

It can be improved if we do some operation RU predication later on.
Or we can do some stuff like: for each operation, it can be attached to a certain cycle version. and we only track the response if it matches to the current cycle version. Maybe this is a better way?

The reason I introduced the read write lock is mainly to keep ScheduledThroughput and availableThroughput to be in sync, since if we are going to return 429, then we depends on those two values to calculate the backoff time.

Any throughputs for this one?

.../src/test/java/com/azure/cosmos/implementation/throughputControl/ThroughputControlTests.java

xinlian12 · 2021-02-27T19:34:16Z

/azp run java - cosmos - tests

azure-pipelines · 2021-02-27T19:34:29Z

Azure Pipelines successfully started running 1 pipeline(s).

xinlian12 · 2021-02-27T19:39:22Z

/azp run java - cosmos - tests

azure-pipelines · 2021-02-27T19:39:33Z

Azure Pipelines successfully started running 1 pipeline(s).

moderakh

great work @xinlian12
Looks great. This is done very well.

I added a few comments around thread-safety, your usage of locks, and also I think you need to more frequently update the document associated with each client (as discussed at least every ttl -1 / 2 to ensure your document doesn't get ttl due to slight shift in task scheduling, etc)
other than these looks great.

moderakh · 2021-03-01T03:28:16Z

...src/main/java/com/azure/cosmos/implementation/throughputControl/LinkedCancellationToken.java

+
+    public void cancel() {
+        if (this.cancellationRequested.compareAndSet(false, true)) {
+            for (LinkedCancellationTokenSource childTokenSource : this.childTokenSourceList) {


non thread-safe access to the this.childTokenSourceList

...com/azure/cosmos/implementation/throughputControl/config/ThroughputControlGroupInternal.java

moderakh · 2021-03-01T03:32:11Z

.../cosmos/implementation/throughputControl/controller/group/ThroughputGroupControllerBase.java

+                }
+            })
+            .doOnSuccess(requestController -> {
+                if (requestController != null) {


per reactor-stream spec, null values are never allowed. hence requestController always will be non null. you don't need to check.

per reactive-streams/reactive-streams-jvm#209
it is guaranteed you will never receive a null value here.

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/models/CosmosQueryRequestOptions.java

xinlian12 · 2021-03-02T17:12:20Z

/azp run java - cosmos - tests

azure-pipelines · 2021-03-02T17:12:32Z

Azure Pipelines successfully started running 1 pipeline(s).

Annie Liang and others added 30 commits November 30, 2020 17:20

fix requestTimeline

0b5ef5e

clean

86d7521

resolve comments

e4f60d4

something works

77cfb0b

Merge branch 'master' into ThroughputControlDraft

3ad3e00

Squashed commit of the following:

f6b3447

# Throughput control part 1 -- local # refactor # refactor # refactor class name Throughput control part 1 -- Local control

revert back some unrelated change

c3e5bc5

clean

e7dd70c

resolve compilation failure

833981b

resolve some comments

fdb144b

Merge branch 'master' into ThroughputControl-Local

37c4f21

merge master and resolve conflicts

eb8a709

track throughput usage per region

2b365e1

q1 :q!

clean some code

d72f169

resolve comments

8675381

enable throughput control for CosmosClient

cbdc812

merge and resolve conflicts

15ecac2

clean code

2e8856b

some cleaning

e6fa22b

merge master and resolve

48be906

clean and resolve comemnts

c0c6b71

resolve compilation issue

b7a183f

resolve comments

10cdaba

resolve comments

89031cc

fix tests

11d1f27

fix tests

1216900

resolve comments

3234d04

add more unit tests

7b1e3bf

resolve comments

1d6a436

resolve comments and refactor

5bd6270

fixTest

8a6e547