Page MenuHomePhabricator

[lib] Add handler component for retrying holder processing
ClosedPublic

Authored by bartek on Oct 9 2024, 2:03 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 12, 2:38 PM
Unknown Object (File)
Tue, Nov 12, 9:52 AM
Unknown Object (File)
Tue, Nov 12, 9:10 AM
Unknown Object (File)
Fri, Nov 1, 2:57 PM
Unknown Object (File)
Fri, Nov 1, 1:04 PM
Unknown Object (File)
Fri, Nov 1, 6:09 AM
Unknown Object (File)
Fri, Nov 1, 2:01 AM
Unknown Object (File)
Thu, Oct 31, 7:45 AM
Subscribers

Details

Summary

Address ENG-9290.
Added handler component that watches for NOT_ESTABLISHED and NOT_REMOVED holders and retries their processing.

Test Plan
  • Started a DM thread and set image avatar.
  • Changed client Blob Service URL to a fake one.
  • Reset the avatar back to emoji. This caused holder removal that failed.
  • Used console logs in the handler to see that it retries every 5s.
  • After restoring real Blob URL, it successfully removed that holder.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bartek held this revision as a draft.
bartek published this revision for review.Oct 9 2024, 2:58 AM
bartek added inline comments.
lib/handlers/holders-handler.react.js
4 ↗(On Diff #45003)

I was also considering _throttle here.

There's no difference if holder changes (and failures) are infrequent.
But when there's lots of holder "traffic":

  • debounce will wait until there's no traffic for 5000ms and then start processing
  • throttle will retry every 5000ms despite constant traffic

check this playground

44–46 ↗(On Diff #45003)

Just a note - I deliberately used useMemo instead of useCallback for debounce:

  • The latter shouldn't be used with higher order funcs:, source 1, source 2
  • ESLint complains: React Hook useCallback received a function whose dependencies are unknown. Pass an inline function instead react-hooks/exhaustive-deps
tomek added inline comments.
lib/handlers/holders-handler.react.js
7–11 ↗(On Diff #45003)

We can merge these

17 ↗(On Diff #45003)

This feels quite frequent. Do you think it makes sense?

This revision is now accepted and ready to land.Oct 10 2024, 5:03 AM
lib/handlers/holders-handler.react.js
17 ↗(On Diff #45003)

You might be right, maybe increasing to 30s would be better? Or even 60? But I'm worrying about the debounce vs throttle problem I described above - perhaps we should use throttle then

Set timeout to 30s, use throttle