Page MenuHomePhabricator

[lib] Add handler component for retrying holder processing
ClosedPublic

Authored by bartek on Wed, Oct 9, 2:03 AM.
Tags
None
Referenced Files
F2988864: D13653.id45004.diff
Wed, Oct 16, 6:56 PM
Unknown Object (File)
Wed, Oct 16, 3:42 AM
Unknown Object (File)
Sun, Oct 13, 6:27 PM
Unknown Object (File)
Sun, Oct 13, 1:06 PM
Unknown Object (File)
Thu, Oct 10, 7:15 AM
Unknown Object (File)
Thu, Oct 10, 7:14 AM
Unknown Object (File)
Thu, Oct 10, 7:14 AM
Unknown Object (File)
Wed, Oct 9, 5:02 PM
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
No Lint Coverage
Unit
No Test Coverage

Event Timeline

bartek held this revision as a draft.
bartek published this revision for review.Wed, Oct 9, 2:58 AM
bartek added inline comments.
lib/handlers/holders-handler.react.js
4

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

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

We can merge these

17

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

This revision is now accepted and ready to land.Thu, Oct 10, 5:03 AM
lib/handlers/holders-handler.react.js
17

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