-
Notifications
You must be signed in to change notification settings - Fork 1
feat(llc): Add caching for own capabilities #47
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
Conversation
gpunto
left a comment
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.
It's more or less similar to what I'm implementing, but I have a couple of questions (that I also ask myself)
| Future<List<FeedOwnCapability>?> getCapabilities(String feed) async { | ||
| return _capabilities[feed] ?? | ||
| (await _fetchBatchedFeedCapabilities(feed)).getOrNull(); | ||
| } |
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.
Do I read it correctly that on failures we just give up? I'm thinking if it makes sense to have some retry
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.
That's a good idea, I was not sure yet what to do. Should we just add the list of failed feeds to the next batch, or shall we retry only this specific batch once, so the next batch is clean and can also be retried?
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 was thinking of adding them to the next batch, but no strong opinion. We could also check what they do in JS with @isekovanic
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.
For now I've added a retry after half a second, but we can improve on this later
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.
Currently I have not added a retry mechanism on JS, but the queued up IDs will simply not be purged unless the request is successful. So they'll be there the next time something arrives.
But, a retry is much better in my opinion as well, since otherwise you might need to wait. I'll add a retry as well on my side.
| final ownCapabilities = | ||
| await capabilitiesRepository.getCapabilities(fid.rawValue); |
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.
What happens if we get another WsEvent while we're waiting here? Is the new event going to be processed concurrently or will it wait until the capabilities call is done (including the debounce timeout)?
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 feed handler can indeed handle multiple concurrent events. This await will only return the capabilities of this feed, but the repository will batch them together.
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.
Ok, so just to confirm, if we're waiting here for the backend to respond, we are still going to process other random WS events, like activity deleted, comment added, etc and update the state, right?
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.
yes correct
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (1.57%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #47 +/- ##
========================================
- Coverage 3.29% 3.24% -0.06%
========================================
Files 117 120 +3
Lines 3157 3271 +114
========================================
+ Hits 104 106 +2
- Misses 3053 3165 +112 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| if (result.shouldRetry() && !isRetry) { | ||
| await Future<void>.delayed(const Duration(milliseconds: 500)); | ||
| return _fetchWithRetry(feeds: feeds, isRetry: true); | ||
| } |
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.
Is it going to loop forever in case of no internet? Maybe better to limit it to n times. We can use the backoff from rate_limiter.
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.
Because of the isRetry: true we only retry once.
| final activity = event.activity.toModel(); | ||
| state.onActivityUpdated(activity); | ||
|
|
||
| final updatedActivity = await withUpdatedFeedCapabilities(activity); | ||
| if (updatedActivity != null) { | ||
| state.onActivityUpdated(updatedActivity); | ||
| } | ||
| return; |
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.
Can't we have this logic in the state layer?
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.
discussed offline to keep it here for now.
Submit a pull request
Closes FLU-294
Also fixes FLU-291
Closes #
CLA
Description of the pull request
The ActivityAdded events don't contain capabilities on the
currentFeed, so we have to add them while we get the event and cache the capabilities so we don't have to fetch them often.Screenshots / Videos