CreateAndInitializeAsync: Adds Code to Optimize Rntbd Open Connection Logic to Open Connections in Parallel.

[kundadebdatta]

Pull Request Template

Description

This PR optimizes the Rntbd open connection logic in the GatewayAddressCache by doing the following:

• Refactors the open connection logic to get partition addresses and open Rntbd connections in parallel, instead of invoking sequentially. This will help the CosmosClient.CreateAndInitializeAsync() flow to initialize and warm up the caches much faster for a large number of partitions, as compared with the previous counterpart.

• Utilizes the provided cancellationToken to cancel the OpenConnection() flow, so that the control of the time taken to initialize the cosmos client, remains to the caller.

Type of change

Please delete options that are not relevant.

• New feature (non-breaking change which adds functionality)

Closing issues

To automatically close an issue: closes #3518, #3923

[ghost]

Hello @kundadebdatta!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

[kirankumarkolli]

How is 45M determined/tuned future?
Incoming cancellatoinToken is an option right?

[ealsur]

Incoming cancellation is honored through:

using CancellationTokenSource linkedTokenSource = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);

[kirankumarkolli]

True, just incoming cancellationToken might suffice right? Why is extra 45M cap?

[kundadebdatta]

The 45 Min cap is there to honor the connection opening time, when there is no cancellation token provided. In order to understand the wiring better, please see the code snippet below:

        using CancellationTokenSource linkedTokenSource = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);
        Task timeoutTask = Task.Delay(45 mins, linkedTokenSource.Token);
        Task resultTask = await Task.WhenAny(Task.WhenAll(tasks), timeoutTask);

        if (resultTask == timeoutTask)
        {
            // Operation has been cancelled.
            DefaultTrace.TraceWarning("The open connection task was cancelled because the cancellation token was expired. '{0}'",
                System.Diagnostics.Trace.CorrelationManager.ActivityId);
        }
        else
        {
            linkedTokenSource.Cancel();
        }

In the above code, the resultTask will be completed, either when the timeoutTask is completed, i.e. the cancellationToken is expired OR the Task.WhenAll(tasks) , which is opening the rntbd connections is completed. Today, in .net standard, the Task.WhenAll() doesn't allow a cancellation token to cancel the tasks. There is a Task.WaitAsync on .NET >= 6, which allows cancellation token, however it is not supported in the current .net framework. This is the reason we are taking this alternate route to honor the cancellation token.

[ealsur]

True, just incoming cancellationToken might suffice right? Why is extra 45M cap?

Incoming cancellation is optional, what happens if the user does not provide one? How do we timebox this in case something hangs?

[kirankumarkolli]

Personal opinion, 45M too long for any startup task (might be as bad as deadlock for them).
Its a new behavior right, is it an ASK from customers?

[ealsur]

nit: What is more performant? Any() or Count > 0?

[kirankumarkolli]

Is the check necessary?

[kundadebdatta]

Yea, I think we can remove this condition. The await Task.WhenAll(openConnectionTasks); should be sufficient to check for empty tasks as well.

[kirankumarkolli]

Wire CancellationTokens?

[kundadebdatta]

In my understanding, the cancellationToken should honor the total open connection time to all replicas, and not individual connection time per partition key ranges. The entire thing is covered in Task resultTask = await Task.WhenAny(Task.WhenAll(tasks), timeoutTask);

[kirankumarkolli]

Yield based on cancelaltion after wiring through?

[kundadebdatta]

Could you please help me understanding this better ? Currently there is no need to have the cancellationToken inside WarmupCachesAndOpenConnectionsAsync method.

[ealsur]

stop the foreach if the cancellation is signaling (if ct.IsCancellationRequested break)

[kirankumarkolli]

OnCancellation behavior: With new changes its trying to gracefully complete right?
Current existing pattern across is to throw exception

cancellationToken.ThrowIfCancellationRequested()