Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

L2 Cache retry initial commit #1046

Merged
merged 4 commits into from
Mar 7, 2021
Merged

L2 Cache retry initial commit #1046

merged 4 commits into from
Mar 7, 2021

Conversation

jennyf19
Copy link
Collaborator

@jennyf19 jennyf19 commented Mar 4, 2021

Fixes #1042 1042
-TODO - needs testing and further work, just to start discussion

@jennyf19 jennyf19 requested a review from jmprieur March 4, 2021 17:57
@henrik-me
Copy link
Contributor

I think this direction is good

@@ -82,6 +82,12 @@ protected override async Task RemoveKeyAsync(string cacheKey)
catch (Exception ex)
{
_logger.LogError($"[IdWebCache] Connection issue encountered with Distributed cache. Currently using In Memory cache only. Error message: {ex.Message} ");
Copy link
Contributor

@henrik-me henrik-me Mar 4, 2021

Choose a reason for hiding this comment

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

Currently using In Memory cache only [](start = 101, length = 36)

Forgot to mention this last time...
Replace: Remove cache entry for (key) failed. Don't have to mention anything about the memory cache here.
(same comment a few other places)
#Resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done & i agree...was more for testing too to see where we were.


In reply to: 587807465 [](ancestors = 587807465)

if (_distributedCacheOptions.OnL2CacheFailure != null && _distributedCacheOptions.OnL2CacheFailure(ex))
{
_logger.LogDebug($"[IdWebCache] DistributedCache: Retry to remove cacheKey {cacheKey}. ");
await _distributedCache.RemoveAsync(cacheKey).ConfigureAwait(false);
Copy link
Contributor

@henrik-me henrik-me Mar 4, 2021

Choose a reason for hiding this comment

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

await _distributedCache.RemoveAsync(cacheKey).ConfigureAwait(false); [](start = 20, length = 68)

have to catch this one as well #Resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes. fixed.


In reply to: 587807756 [](ancestors = 587807756)

@@ -82,6 +82,12 @@ protected override async Task RemoveKeyAsync(string cacheKey)
catch (Exception ex)
{
_logger.LogError($"[IdWebCache] Connection issue encountered with Distributed cache. Currently using In Memory cache only. Error message: {ex.Message} ");

if (_distributedCacheOptions.OnL2CacheFailure != null && _distributedCacheOptions.OnL2CacheFailure(ex))
Copy link
Contributor

@henrik-me henrik-me Mar 4, 2021

Choose a reason for hiding this comment

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

Good pattern to apply everywhere. Try/catch can be refactored into a method/generic which takes an action #Resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed.


In reply to: 587808630 [](ancestors = 587808630)

@henrik-me
Copy link
Contributor

henrik-me commented Mar 4, 2021

        var startTicks = Utility.Watch.Elapsed.Ticks;

nit: Usage of timer/ticks/time spend can be wrapped up in a helper. Allowing you to have time since method start as well as time for each operation of needed. Also you have now one more place where you want to put timing data into the log. Can also consider adding the total call time for the method. #Resolved


Refers to: src/Microsoft.Identity.Web/TokenCacheProviders/Distributed/MsalDistributedTokenCacheAdapter.cs:73 in 92fdc0e. [](commit_id = 92fdc0e, deletion_comment = False)

@jennyf19
Copy link
Collaborator Author

jennyf19 commented Mar 6, 2021

        var startTicks = Utility.Watch.Elapsed.Ticks;

i've used the Measure() static method for this instead.


In reply to: 790928037 [](ancestors = 790928037)


Refers to: src/Microsoft.Identity.Web/TokenCacheProviders/Distributed/MsalDistributedTokenCacheAdapter.cs:73 in 92fdc0e. [](commit_id = 92fdc0e, deletion_comment = False)

@jennyf19 jennyf19 changed the title [WIP] L2 Cache retry initial commit L2 Cache retry initial commit Mar 6, 2021
@jennyf19 jennyf19 marked this pull request as ready for review March 6, 2021 21:27
Copy link
Collaborator

@jmprieur jmprieur left a comment

Choose a reason for hiding this comment

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

LGTM.
Thanks @jennyf19. Awesome feature.
I tried it out with the Redis cache. It works great.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 7, 2021

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.

[Feature Request] Enable apps to decide what to do when the L2 cache fails (Retry)
3 participants