Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Switch the telemetry to new futures #3100

Merged
merged 5 commits into from
Jul 11, 2019

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Jul 11, 2019

cc #3099

The majority of the switch to new futures is straight-forward, but for the telemetry it is pretty much not straight-forward.
Since libp2p is using futures v0.1, and the telemetry has a wrapper that guarantees that one telemetry write equals one WebSocket frame, we need to have a compatibility shim.

After this PR, the API of substrate-telemetry uses new futures.

@tomaka tomaka added the A0-please_review Pull request needs code review. label Jul 11, 2019
libp2p::core::transport::map::Map<
OptionalTransport<wasm_ext::ExtTransport>,
fn(wasm_ext::Connection, ConnectedPoint) -> StreamSink<wasm_ext::Connection>
libp2p::core::transport::map::Map<
Copy link
Member

Choose a reason for hiding this comment

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

😨

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem here is that I'm using libp2p in a way it wasn't really designed for, but theoretically I could add some utility that allows using a Box here instead.

Copy link
Member

Choose a reason for hiding this comment

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

It is okay, probably more rust faults of not having impl Something.

core/telemetry/src/worker/node.rs Outdated Show resolved Hide resolved
core/telemetry/src/worker/node.rs Outdated Show resolved Hide resolved
core/telemetry/src/worker/node.rs Outdated Show resolved Hide resolved
@gavofyork gavofyork merged commit e40864c into paritytech:master Jul 11, 2019
@tomaka tomaka deleted the telemetry-new-futures branch July 11, 2019 16:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants