-
Notifications
You must be signed in to change notification settings - Fork 70
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
feat: fallback to NoopProvider
if OOM happens
#485
feat: fallback to NoopProvider
if OOM happens
#485
Conversation
3e8ee71
to
6ba2711
Compare
@marcaaron @tgolen I assigned you as you were reviewer of the closed PR and because you're familiar with the Onyx library |
It's not needed to review now, because it contains changes from part 2. Let's review & merge part 2 and after that I'll rebase this Pr and we can review it 👀 |
7725163
to
d400ef1
Compare
@danieldoglas @tgolen this PR is ready for review 😊 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM - I agree with @hayata-suenaga that we do not need to measure the size of the store (or at least can't think of any useful reason to do it).
@marcaaron so do you want to return a mocked value from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the code looks good to me
there is a pending question
yeah, it does not need to be accurate so I'd rather not add a dependency if we don't need one. |
@marcaaron okay, make sense - I'll remove it 🙌 |
56e7087
56e7087
to
c461b99
Compare
MemoryOnlyProvider
[3/3]MemoryOnlyProvider
let us know when the RR becomes ready for review again 🙇 |
I got word from QA this morning that they have begun testing the E/App PR. They will report any bugs they find on that PR and we can triage them from there. |
a2ea02e
to
ce04390
Compare
ce04390
to
8254d59
Compare
OK, I took one last look at the code and it looks good to me. I'm glad QA hasn't found any more issues with this and sounds like it's ready to be released. @marcaaron @danieldoglas @hayata-suenaga I wouldn't mind having at least one more approval before merging this. Would one of you like the honors? |
thank you for the review, Tim! Unfortunately, I don't have time to go over the code again, so I'd wait for Daniel or Marc 😄 |
Sure, taking a look! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a small note. I am not sure if it's worth blocking on and this PR has been open for quite a while so suggesting a follow up to address it instead.
* Degrade performance by removing the storage provider and only using cache | ||
*/ | ||
function degradePerformance(error: Error) { | ||
Logger.logAlert(`Error while using ${provider.name}. Falling back to only using cache and dropping storage.`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAB, it would be useful to log the error message as well. Without knowing anything about why we fell back to using the cache this alert is not really actionable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I always got tripped up on this too, but it is logged on the very next line 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, but maybe you mean to send the error as part of logAlert()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! That's what I meant. I do see the console.error()
which is local (good) but might be desirable to have server logs.
I backpedaled here because I'm not sure if we are going to be tracking the frequency of these errors or doing anything about them.
That said, I also wonder if logging alert will lead to issues that nobody can action... I am not sure if the JS logs create issues or not. Maybe they should and if so - this could probably be a hmmm()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tgolen @marcaaron should I do these changes in follow up PR? 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think it would probably be good to change this to a hmmm()
and also log the specific error. It would sure be nice to look at server logs to determine IF the fallback is happening and also WHY.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Details
A follow up for #439
This PR accumulates al changes from #483 and #475 and adds a usage of
MemoryOnlyProvider
in unit-tests.Originally #483 and #475 were reverted - in this PR I added fixes:
this
isundefined
(which prevented a synchronization between tabs).In this PR I added [1/3]:
Storage
layer (in future this layer will intercept errors and will substitute provider to InMemory);InstanceSync
class/object - such approach allows us to avoid code duplication if we addInMemory
provider on web;init
method);In this PR I added [2/3]:
name
property to all providers (used in logger if we ran into OOM);NoopProvider
that is used as a fallback one when OOM happens;tryOrDegradePerformance
function that attempt to perform operation and in case of specific error substitutes the storage to a fallback one;In this PR I added [3/3]:
MemoryOnlyProvider
implementation that stores all info in memory;MemoryOnlyProvider
there;Related Issues
Expensify/App#29403
Automated Tests
This PR changes the way we use the storage but the functionality of the library to the outer world is the same. Therefore no new tests were added.
Manual Tests
Verify that no flows and functionality were broken by the changes. Check for console errors regarding Onyx.
Author Checklist
### Related Issues
section aboveTests
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop