[Internal] Upgrade Resiliency: Adds Code to Enable Replica Validation Feature Through CosmosClientOptions
#3967
Conversation
kundadebdatta
force-pushed
the
users/kundadebdatta/3922_make_replica_validation_dynamic
branch
from
23490fe
to
7a0bbcf
Compare
kundadebdatta
requested review from
khdang,
sboshra,
adityasa,
neildsh,
kirankumarkolli,
ealsur,
FabianMeiswinkel and
kirillg
as code owners
| /// timeouts. The default value for this parameter is false. | ||
| /// </summary> | ||
| /// <returns>The <see cref="CosmosClientBuilder"/> object</returns> | ||
| internal CosmosClientBuilder WithAdvancedReplicaSelectionEnabledForTcp() |
There was a problem hiding this comment.
Compute doesn't use builder API's is not needed.
There was a problem hiding this comment.
Thanks! Removed the method in the latest commit.
| /// <remarks> | ||
| /// <para>This is optimal for latency-sensitive workloads. Does not apply if <see cref="ConnectionMode.Gateway"/> is used.</para> | ||
| /// </remarks> | ||
| internal bool EnableAdvancedReplicaSelectionForTcp { get; set; } |
There was a problem hiding this comment.
Is explicit differentiation about opted/not vs default behavior for compute?
If so one option is to model it as nullable. Thoughts?
There was a problem hiding this comment.
Similar to EnableContentResponseOnWrite
There was a problem hiding this comment.
Well, not setting this flag EnableAdvancedReplicaSelectionForTcp will automatically make the SDK to opt out from replica validation. In order to opt the feature, Compute doesn't need to specify any value for this field and the default value for this flag is always false.
I think this brings more simplicity, since we are anyhow not going to make this flag to public.
There was a problem hiding this comment.
You can make the SDK default to disabled if the bool? has not value by just capturing:
bool isEnabled = EnableAdvancedReplicaSelectionForTcp.HasValue && EnableAdvancedReplicaSelectionForTcp.Value;
I think Kiran's point is that semantically it's always better to make settings bool? like EnableContentResponseOnWrite because it allows us (for whatever reason) to tell apart the scenario where the property's setter was not touched (use default) and when the user did set some preference (either true or false). In this particular case there might not be a special case for not touched, but for consistency's sake, if we are using bool? for other settings, might as well use it here.
There was a problem hiding this comment.
Its an optional property and hence the conversation.
If compute if always set it explicitly (true/false) then for current requirements its makes sense.
| @@ -233,7 +232,6 @@ internal partial class DocumentClient : IDisposable, IAuthorizationTokenProvider | |||
|
|
|||
| this.Initialize(serviceEndpoint, connectionPolicy, desiredConsistencyLevel); | |||
| this.initTaskCache = new AsyncCacheNonBlocking<string, bool>(cancellationToken: this.cancellationTokenSource.Token); | |||
| this.isReplicaAddressValidationEnabled = ConfigurationManager.IsReplicaAddressValidationEnabled(); | |||
There was a problem hiding this comment.
Is CosmosCLientOptions needs to default to ConfigurationManager.IsReplicaAddressValidationEnabled()?
There was a problem hiding this comment.
Did we ship a version that supported setting the environment variable and is anyone using it? If yes, we might want to have backward compatibility?
There was a problem hiding this comment.
CosmosClientOptions does not need to default to ConfigurationManager.IsReplicaAddressValidationEnabled(). This was an earlier implementation, which was reading the environment variable to set the replica validation flag. Since we never shipped this feature with any version of the SDK, we can safely remove this and rely on the CosmosClientOptions.
There was a problem hiding this comment.
Since we never shipped this feature with any version of the SDK, we can safely remove this
Then yeah, no issues from my side.
There was a problem hiding this comment.
How do external CX enable/disable this feature?
There was a problem hiding this comment.
Are you suggesting to have both the environment variable and CosmosClientOptions way of enabling and disabling the feature ? My understanding was we will flight it with Compute first, and thereafter we will enable it by default for the external Cx ? That way, there is no need to have any env variables/ client options.
|
This PR is no longer required as an alternate PR has been merged to |
kundadebdatta
deleted the
users/kundadebdatta/3922_make_replica_validation_dynamic
branch
Pull Request Template
Description
In reference to a recent conversation with the compute gateway team, we agreed to add an
internalBoolean flag inCosmosClientOptionsto dynamically enable the feature. This PR adds the flag inCosmosClientOptionsto enable or disable the replica validation using the client options.Type of change
Please delete options that are not relevant.
Closing issues
To automatically close an issue: closes #3922