Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow runtimes to have a custom dispatcher #2749

Merged
merged 2 commits into from
Mar 5, 2020
Merged

Conversation

jberci
Copy link
Contributor

@jberci jberci commented Mar 2, 2020

Closes #2281.

@codecov
Copy link

codecov bot commented Mar 2, 2020

Codecov Report

Merging #2749 into master will decrease coverage by 0.44%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2749      +/-   ##
==========================================
- Coverage   62.74%   62.29%   -0.45%     
==========================================
  Files         384      384              
  Lines       36258    36258              
==========================================
- Hits        22749    22588     -161     
- Misses      10616    10815     +199     
+ Partials     2893     2855      -38
Impacted Files Coverage Δ
...s-node/cmd/debug/txsource/workload/registration.go 0% <0%> (-74.44%) ⬇️
...n/crypto/signature/signers/memory/memory_signer.go 71.42% <0%> (-4.77%) ⬇️
go/consensus/api/grpc.go 59.3% <0%> (-4.66%) ⬇️
go/worker/common/p2p/p2p.go 68.46% <0%> (-2.71%) ⬇️
go/consensus/tendermint/tendermint.go 66.39% <0%> (-0.55%) ⬇️
go/worker/registration/worker.go 66.34% <0%> (-0.49%) ⬇️
go/storage/database/database.go 76.31% <0%> (ø) ⬆️
go/storage/metrics.go 75.21% <0%> (ø) ⬆️
go/storage/client/client.go 76.89% <0%> (+0.42%) ⬆️
go/worker/compute/merge/committee/node.go 76.11% <0%> (+0.74%) ⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8079357...cdb2e53. Read the comment docs.

@jberci jberci force-pushed the jberci/feature/dispatcher branch from 3cc6176 to 5f03879 Compare March 2, 2020 14:50
Copy link
Contributor

@pro-wh pro-wh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems straightforward

{
txn
} else {
Box::new(TxnMethDispatcher::new())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if we made the initializer do this internally

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure we can... whoever constructs a concrete dispatcher will need its specific interface, so it would be problematic if the initializer itself returned the general boxed trait already.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok what about yet another implementation that's specifically a no-op dispatcher

@jberci jberci force-pushed the jberci/feature/dispatcher branch from 5f03879 to d658954 Compare March 3, 2020 15:07
@jberci jberci force-pushed the jberci/feature/dispatcher branch from d658954 to 9d347a7 Compare March 4, 2020 15:01
@jberci jberci force-pushed the jberci/feature/dispatcher branch from 9d347a7 to cdb2e53 Compare March 4, 2020 15:09
@jberci jberci merged commit a45718f into master Mar 5, 2020
@kostko kostko deleted the jberci/feature/dispatcher branch April 22, 2020 07:59
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.

Allow runtimes to have a custom dispatcher
3 participants