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
F3518022: D13653.id45169.diff
Sun, Dec 22, 7:01 PM
F3516644: D13653.diff
Sun, Dec 22, 3:02 PM
Unknown Object (File)
Wed, Dec 18, 8:38 AM
Unknown Object (File)
Wed, Dec 18, 8:38 AM
Unknown Object (File)
Wed, Dec 18, 8:37 AM
Unknown Object (File)
Tue, Dec 10, 11:18 AM
Unknown Object (File)
Mon, Dec 9, 10:35 PM
Unknown Object (File)
Mon, Dec 9, 5:40 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
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