Page MenuHomePhabricator

[lib] Add action type for processing holders
ClosedPublic

Authored by bartek on Sep 20 2024, 6:21 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 25, 9:58 AM
Unknown Object (File)
Mon, Nov 25, 8:38 AM
Unknown Object (File)
Mon, Nov 25, 6:24 AM
Unknown Object (File)
Sun, Nov 24, 7:16 PM
Unknown Object (File)
Wed, Nov 20, 10:35 AM
Unknown Object (File)
Wed, Nov 20, 10:35 AM
Unknown Object (File)
Wed, Nov 20, 10:35 AM
Unknown Object (File)
Wed, Nov 20, 10:35 AM
Subscribers

Details

Summary

Added action types and updated reducer for the "process holders" action.

  • Holders are marked as PENDING_ESTABLISHMENT or PENDING_REMOVAL, before making Blob calls
  • Afterwards they're either removed or marked as ESTABLISHED.
  • In case of failure, all of them are marked as NOT_ESTABLISHED / NOT_REMOVED. This will be then handled by a handler component (introduced later) for retrying.

Depends on D13403

Test Plan
  • Reducer unit tests
  • Tested together with actual Blob calls

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.Sep 20 2024, 6:43 AM
lib/actions/holder-actions.js
14–23 ↗(On Diff #44371)

Why aren't these types read-only?

kamil requested changes to this revision.Sep 20 2024, 8:16 AM
kamil added inline comments.
lib/actions/holder-actions.js
3–4 ↗(On Diff #44371)
7 ↗(On Diff #44371)

nit: I would remove StoreEstablishedHolderPayload and use BlobHashAndHolder directly

14–23 ↗(On Diff #44371)
lib/reducers/holder-reducer.js
63 ↗(On Diff #44371)

I think we should change this, update PROCESS_HOLDERS_FAILED to have failed hashes as payload (similar as here) and only those mark as failed.

Additionally wondering, if we really need pending states after this.

This revision now requires changes to proceed.Sep 20 2024, 8:16 AM

Updated types and failure action

This revision is now accepted and ready to land.Sep 23 2024, 5:09 AM
lib/reducers/holder-reducer.js
63 ↗(On Diff #44371)

makes sense, thanks!

Additionally wondering, if we really need pending states after this.

Yes, we need it to avoid unnecessarily start processing new holders if they're already being processed (pending status)