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

Cache resiliency #63

Merged
merged 11 commits into from
Oct 1, 2020
Merged

Cache resiliency #63

merged 11 commits into from
Oct 1, 2020

Conversation

abhidnya13
Copy link
Contributor

@abhidnya13 abhidnya13 commented Jul 28, 2020 •
edited by rayluo
Loading

This should have close #61

@abhidnya13 abhidnya13 requested a review from rayluo July 28, 2020 17:20
rayluo
rayluo reviewed Jul 28, 2020
View reviewed changes
Copy link
Contributor

@rayluo rayluo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@abhidnya13 You clearly have done your experiments, one week ago and yesterday. Is there any learnings from those experiments to suggest that we go one way or another, that you can perhaps put inside this PR's description or as a comment below?

PS: Might as well ping @SomkaPe so that he could also comment on how this 15 lines compares to what he did in MSAL EX Java.

UPDATE at 8/4/2020: Per our offline discussion, let's switch back to your first commits approach.

@rayluo rayluo mentioned this pull request Aug 21, 2020
Open
@abhidnya13 abhidnya13 marked this pull request as ready for review September 14, 2020 22:23
@abhidnya13 abhidnya13 requested a review from rayluo September 15, 2020 17:13
@abhidnya13 abhidnya13 requested a review from rayluo September 18, 2020 20:38
rayluo
rayluo reviewed Sep 21, 2020
View reviewed changes
Copy link
Contributor

@rayluo rayluo left a comment

Choose a reason for hiding this comment

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

Overall structure looks good. Now let's dive into the details.

msal_extensions/cache_lock.py Outdated Show resolved Hide resolved
msal_extensions/cache_lock.py Outdated Show resolved Hide resolved
msal_extensions/cache_lock.py Outdated Show resolved Hide resolved
@abhidnya13 abhidnya13 requested a review from rayluo October 1, 2020 00:52
rayluo
rayluo approved these changes Oct 1, 2020
View reviewed changes
Copy link
Contributor

@rayluo rayluo left a comment

Choose a reason for hiding this comment

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

Overall looks good. Your latest commit reminds me that I missed out an issue in earlier commits. :-) Please refer to the comment below. I'm still leaving an approval here in advance.

msal_extensions/cache_lock.py Outdated Show resolved Hide resolved
rayluo
rayluo reviewed Oct 1, 2020
View reviewed changes
msal_extensions/cache_lock.py Outdated Show resolved Hide resolved
msal_extensions/cache_lock.py Show resolved Hide resolved
rayluo
rayluo approved these changes Oct 1, 2020
View reviewed changes
Copy link
Contributor

@rayluo rayluo left a comment

Choose a reason for hiding this comment

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

Good job! Ship it! :-)

@abhidnya13 abhidnya13 merged commit bd930e8 into dev Oct 1, 2020
@abhidnya13 abhidnya13 deleted the cache-resiliency branch October 1, 2020 22:13
@rayluo rayluo mentioned this pull request Oct 7, 2020
Open
@rayluo rayluo mentioned this pull request Dec 1, 2021
Closed
This was referenced Dec 9, 2021
Closed
Merged
@rayluo rayluo mentioned this pull request Apr 28, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rayluo rayluo rayluo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cache resiliency improvements
2 participants
@abhidnya13 @rayluo