Skip to content

Enhancement: Adds Code to Enable Exception Handling Using CosmosClientOptions#5072

Closed
kundadebdatta wants to merge 1 commit intoananth/openai-exception-handling-asynccachefrom
users/kundadebdatta/openai_asynccache_exceptionhandling_client_options
Closed

Enhancement: Adds Code to Enable Exception Handling Using CosmosClientOptions#5072
kundadebdatta wants to merge 1 commit intoananth/openai-exception-handling-asynccachefrom
users/kundadebdatta/openai_asynccache_exceptionhandling_client_options

Conversation

@kundadebdatta
Copy link
Copy Markdown
Member

Pull Request Template

Description

This PR enables AsyncCache and AsyncCacheNonBlocking Exception Handling Using CosmosClientOptions.

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 #IssueNumber

@kundadebdatta kundadebdatta changed the base branch from master to ananth/openai-exception-handling-asynccache March 19, 2025 23:35
CosmosClientTelemetryOptions cosmosClientTelemetryOptions = null,
IChaosInterceptorFactory chaosInterceptorFactory = null)
IChaosInterceptorFactory chaosInterceptorFactory = null,
bool? enableStackTraceOptimization = false)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need both ? and a default value of false?

public async Task ValidateNegativeScenario(bool forceRefresh)
{
AsyncCacheNonBlocking<string, string> asyncCache = new AsyncCacheNonBlocking<string, string>();
AsyncCacheNonBlocking<string, string> asyncCache = new AsyncCacheNonBlocking<string, string>(enableStackTraceOptimization: false);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even if we don't set the enableStackTraceOptimization: false, the constructor has a default value? but I think being explicit is always good.

/// When enabled, critical SDK components optimize exception handling to minimize performance overhead.
/// The default value is 'false'.
/// </summary>
internal bool? EnableStackTraceOptimization { get; set; }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need ternary state for decision model?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two states with default being true is my preference

/// <summary>
/// To enable stack trace optimization to reduce stack trace proliferation in high-concurrency scenarios where exceptions are frequently thrown.
/// When enabled, critical SDK components optimize exception handling to minimize performance overhead.
/// </summary>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Builder change is not needed.

internal InternalCache(
bool enableStackTraceOptimization)
{
this.collectionInfoByName = new AsyncCache<string, ContainerProperties>(new CollectionRidComparer());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pass this through

/// When enabled, critical SDK components optimize exception handling to minimize performance overhead.
/// The default value is 'false'.
/// </summary>
internal bool? EnableStackTraceOptimization { get; set; }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
internal bool? EnableStackTraceOptimization { get; set; }
internal bool? EnableAsyncCacheExceptionSharing { get; set; }

@kundadebdatta
Copy link
Copy Markdown
Member Author

This PR is duplicate of #5073 . Hence closing this to track the mainline.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants