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.

Sign up for GitHub

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

Jump to bottom

Fix L2 logging. Fix L2 empty read. #2339

Merged
merged 6 commits into from
Jul 28, 2023
Merged

Fix L2 logging. Fix L2 empty read. #2339

merged 6 commits into from
Jul 28, 2023

Conversation

pmaytak
Copy link
Contributor

@pmaytak pmaytak commented Jul 22, 2023 •
edited
Loading

Description

  • Updates MsalAbstractTokenCacheProvider.OnBeforeAccessAsync to call Msal.Deseralize even if the read operation returned nothing.
  • Fixes distributed cache logger to print correct cache entry size.

Fixes #2348, fixes #2349

@pmaytak pmaytak marked this pull request as ready for review July 27, 2023 02:21
@jennyf19
Copy link
Collaborator

@pmaytak Thanks for fixing this, but looks like tests are failing...were they failing locally as well?

@pmaytak
Copy link
Contributor Author

pmaytak commented Jul 27, 2023

@pmaytak Thanks for fixing this, but looks like tests are failing...were they failing locally as well?

Fixed some tests. The rest fail because of Lab.

@jennyf19
Copy link
Collaborator

@pmaytak Thanks for fixing this, but looks like tests are failing...were they failing locally as well?

Fixed some tests. The rest fail because of Lab.

Tests are still failing...

@jennyf19
Copy link
Collaborator

@pmaytak & @bgavrilMS - any update on this PR?

@pmaytak
Copy link
Contributor Author

pmaytak commented Jul 28, 2023

@pmaytak & @bgavrilMS - any update on this PR?

The tests were failing because of Lab issues, which were fixed, so re-running the build now.

jennyf19
jennyf19 approved these changes Jul 28, 2023
View reviewed changes
Copy link
Collaborator

@jennyf19 jennyf19 left a comment

Choose a reason for hiding this comment

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

:shipit:

@jennyf19
Copy link
Collaborator

jennyf19 commented Jul 28, 2023 •
edited
Loading

ETA on getting this wrapped up? Asking as we might have to get out another fix for something else and if we should wait or not.

@pmaytak
Copy link
Contributor Author

pmaytak commented Jul 28, 2023

ETA on getting this wrapped up? Asking as we might have to get out another fix for something else and if we should wait or not.

Implemented feedback. Builds are running. You can merge earlier, if you want.

Aside: We enabled auto-merge in .NET repo, it's pretty helpful.

@jennyf19
Copy link
Collaborator

ETA on getting this wrapped up? Asking as we might have to get out another fix for something else and if we should wait or not.

Implemented feedback. Builds are running. You can merge earlier, if you want.

Aside: We enabled auto-merge in .NET repo, it's pretty helpful.

thanks. Would rather not enable auto-merge, but appreciate the tip.

@pmaytak pmaytak merged commit ba21960 into master Jul 28, 2023
4 checks passed
@pmaytak pmaytak deleted the pmaytak/l2-cache-fix branch July 28, 2023 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bgavrilMS bgavrilMS bgavrilMS approved these changes

@jennyf19 jennyf19 jennyf19 approved these changes

@jmprieur jmprieur Awaiting requested review from jmprieur

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@pmaytak @jennyf19 @bgavrilMS