Introduce and use Sink abstraction #9962
Conversation
georgeee
added
the
ci-build-me
| }) ; | ||
| Mina_net2.Validation_callback.set_message_type valid_cb `Block ; | ||
| Mina_metrics.(Counter.inc_one Network.Block.received) ; | ||
| notify_online ()) |
There was a problem hiding this comment.
Is it fine to run notify_online without don't_wait_for?
Can it produce a stale-lock?
There was a problem hiding this comment.
The pipe being written to under the hood of Broadcast_pipe is an Async.Pipe which is consumed with pushback. This means that the write call will block until a reader picks up the value and finishes processing it. I would just put a don't_wait_for here to be safe because it's possible this could produce a stale lock in some conditions.
georgeee
force-pushed
the
georgeee/sink-refactoring
branch
from
7eee16c
to
f175181
Compare
georgeee
requested review from
bkase,
imeckler,
mrmr1993,
psteckler and
a team
as code owners
georgeee
force-pushed
the
georgeee/sink-refactoring
branch
3 times, most recently
from
a4a61bf
to
0379e2f
Compare
georgeee
force-pushed
the
georgeee/sink-refactoring
branch
from
0379e2f
to
051daba
Compare
| }) ; | ||
| Mina_net2.Validation_callback.set_message_type valid_cb `Block ; | ||
| Mina_metrics.(Counter.inc_one Network.Block.received) ; | ||
| notify_online ()) |
There was a problem hiding this comment.
The pipe being written to under the hood of Broadcast_pipe is an Async.Pipe which is consumed with pushback. This means that the write call will block until a reader picks up the value and finishes processing it. I would just put a don't_wait_for here to be safe because it's possible this could produce a stale lock in some conditions.
src/lib/network_pool/pool_sink.ml
Outdated
| let cb' = Msg.convert_callback cb in | ||
| match%bind verify_impl ~logger ~trace_label pool rl env' cb' with | ||
| | None -> | ||
| (* TODO log unverified? *) |
There was a problem hiding this comment.
This TODO needs to be resolved.
georgeee
force-pushed
the
georgeee/sink-refactoring
branch
2 times, most recently
from
243a1f4
to
74c7103
Compare
There was a problem hiding this comment.
LGTM. My last piece of feedback is that I'm not sure that block_sink.ml lives in a logical location right now (it's currently in the network_pool library, which currently contains snark and txn pool code, but not block code). Perhaps mina_transition (where blocks are defined) or transition_handler (where incoming blocks are processed) makes more sense.
georgeee
force-pushed
the
georgeee/sink-refactoring
branch
from
f7fdd16
to
de25253
Compare
Problem: there are a few layers of pipes that are used between sites of pubsub message receival and pubsub message processing. These layers complicate reasoning about code and impose an unnecessary additional load onto Async scheduler. Solution: introduce a Sink abstraction and use it to handle all gossips.
georgeee
force-pushed
the
georgeee/sink-refactoring
branch
from
de25253
to
b8a521c
Compare
georgeee
force-pushed
the
georgeee/sink-refactoring
branch
from
aaf2dc5
to
b227493
Compare
Problem: during refactoring, tx pool and snark pool metrics were not updated properly. Solution: update metrics as needed.
georgeee
force-pushed
the
georgeee/sink-refactoring
branch
from
5a5bd5e
to
42843d7
Compare
Problem: there are a few layers of pipes that are used between sites of
pubsub message receival and pubsub message processing. These layers
complicate reasoning about code and impose an unnecessary additional load
onto Async scheduler.
Solution: introduce a Sink abstraction and use it to handle all gossips.
Explain your changes:
Explain how you tested your changes:
Checklist: