Skip to content

Conversation

@jimmychu0807
Copy link
Collaborator

@stiiifff
Actually I think about it, with the current design on OcwNotification, we don't need to use locks.

pub OcwNotifications get (fn ocw_notifications): map hasher(identity) T::BlockNumber => Vec<ShippingEventIndex>;

Because for every ocw run corresponding to each block, only the queue of that block is processed. There is no overlapping, or double-processing here. In fact this is quite an elegant design, better than having just a single task queue (the previous design).

@jimmychu0807 jimmychu0807 requested a review from stiiifff July 17, 2020 06:02
@jimmychu0807
Copy link
Collaborator Author

@stiiifff @riusricardo
I am looking at the CI failure. How come compiling substrate v2-rc4 would fail? Does this happen to any of you?

https://github.com/stiiifff/pallet-product-tracking/pull/23/checks?check_run_id=880662075

@jimmychu0807
Copy link
Collaborator Author

This is to be fixed once a Substrate has a new release of this PR.

@stiiifff
Copy link
Owner

@stiiifff
Actually I think about it, with the current design on OcwNotification, we don't need to use locks.

pub OcwNotifications get (fn ocw_notifications): map hasher(identity) T::BlockNumber => Vec<ShippingEventIndex>;

Because for every ocw run corresponding to each block, only the queue of that block is processed. There is no overlapping, or double-processing here. In fact this is quite an elegant design, better than having just a single task queue (the previous design).

I agree we can keep it simple for the sample, but I also find it nice to expose devs to some intricacies of working with off-chain workers (and proposing a reusable pattern). Also, I think it'd be best if the worker can deal with transient listener issues, see another take in the refactor-ocw branch.

@jimmychu0807
Copy link
Collaborator Author

Refer to: #20

@jimmychu0807 jimmychu0807 deleted the jimmy/dev/rework-ocw branch July 21, 2020 03:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants