Navigation Menu

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

Remove IntoBuf/FromBuf #288

Merged
merged 5 commits into from Aug 27, 2019
Merged

Conversation

DoumanAsh
Copy link
Contributor

@DoumanAsh DoumanAsh commented Aug 26, 2019

As consequence we remove Buf::collect and instead place it only into Chain.

This makes user to collect underlying bytes harder as can be illustrated with VecDeque

Closes #196

As consequence we remove Buf::collect and instead place it only into Chain.

This makes user to collect underlying bytes harder as can be illustrated with VecDeque
@taiki-e
Copy link
Member

taiki-e commented Aug 27, 2019

errors in cross are due to git apply fail:

error: patch failed: src/docker.rs:160
error: src/docker.rs: patch does not apply

https://dev.azure.com/tokio-rs/bytes/_build/results?buildId=2172&view=logs

We need to fix the patch file or add git checkout 540dc4f5a1ca8d6d95bbf4da66ce763a2352d36f before git apply.

@DoumanAsh
Copy link
Contributor Author

DoumanAsh commented Aug 27, 2019

Oh we had custom build of cross... Damn I wish they would just merge PR fixing it already.

UPD: this is retarded, but cross made new hub for images, but didn't upload any of these images...

Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

LGTM 👍 one question inline.

src/buf/chain.rs Outdated

#[inline]
/// Consumes self and returns joined value, containing underlying bufs.
pub fn collect<B: FromIterator<u8>>(self) -> B where T: Buf, U: Buf {
Copy link
Member

Choose a reason for hiding this comment

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

It's interesting you put this here. What is the reason preventing this from being on the trait? How could all T: Buf types get a collect fn?

Copy link
Member

Choose a reason for hiding this comment

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

Based on convo in Gitter, I think the way forward for this PR is to just remove collect everywhere. We can always bring it back later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@carllerche carllerche mentioned this pull request Aug 27, 2019
Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

Looking good.

I just realized the into_bytes was in this PR. When I received email notifications, the link took me directly to the commit and somehow i missed the into_bytes stuff.

I did a detailed review and I believe that the comments I left represent all my thoughts 👍 Almost there, thanks for sticking with it.

src/buf/buf.rs Outdated Show resolved Hide resolved
src/buf/buf.rs Outdated Show resolved Hide resolved
src/buf/buf.rs Outdated Show resolved Hide resolved
src/buf/chain.rs Outdated Show resolved Hide resolved
Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

Thanks 👍 great work.

@carllerche carllerche merged commit b6cb346 into tokio-rs:master Aug 27, 2019
@DoumanAsh DoumanAsh deleted the remove_into_buf branch August 27, 2019 20:09
gakonst added a commit to interledger/interledger-rs that referenced this pull request Jan 15, 2020
ILP-Packet is built using Bytes 0.4. The Futures 0.3 ecosystem's HTTP crates use Bytes 0.5.
Porting this crate to use Bytes 0.5 is non-trivial due to significant breaking changes in the
Bytes API:

tokio-rs/bytes#350
tokio-rs/bytes#288
gakonst added a commit to interledger/interledger-rs that referenced this pull request Jan 23, 2020
ILP-Packet is built using Bytes 0.4. The Futures 0.3 ecosystem's HTTP crates use Bytes 0.5.
Porting this crate to use Bytes 0.5 is non-trivial due to significant breaking changes in the
Bytes API:

tokio-rs/bytes#350
tokio-rs/bytes#288
gakonst added a commit to interledger/interledger-rs that referenced this pull request Jan 29, 2020
* feat(packet): implement From<Fulfill/Reject> for bytes05::BytesMut

ILP-Packet is built using Bytes 0.4. The Futures 0.3 ecosystem's HTTP crates use Bytes 0.5.
Porting this crate to use Bytes 0.5 is non-trivial due to significant breaking changes in the
Bytes API:

tokio-rs/bytes#350
tokio-rs/bytes#288

# Interledger Service: Futures 0.3 Transition (#596)

* feat(service): upgrade to futures 0.3 and async/await

* feat(service): Box wrapper methods to avoid exponential type blowup

Relevant rust-lang issue: rust-lang/rust#68508

* docs(service): add explanation on IlpResult

* chore(service): remove unused associated type

# Interledger Router: Futures 0.3 Transition (#595)

* feat(router): upgrade to futures 0.3 and async/await

# Interledger ILDCP: Futures 0.3 Transition (#597)

* feat(client): convert client to async/await

* docs(ildcp): enhance docs

* feat(server): make the service async

* test(server): add tests

# Interledger CCP: Futures 0.3 Transition (#598)

* feat(ccp): convert store traits to async/await

* feat(ccp-server): make the ccp server async

* test(ccp-server): make tests async

* chore(routing-table): limit api visibility of table methods

# Interledger BTP: Futures 0.3 Transition  (#599)

* feat(btp): update traits to be async

* refactor(btp/wrapped-ws): refactor WsWrap to a separate file

Ideally, we would want to get rid of it by doing a `StreamExt::map_ok` and `SinkExt::with` to map both WebSocket return types to the same value. We also use `filter_map` to get rid of any errors from the WebSocket. The WsError type has been removed as a result of that.

* feat(btp/client): port to async/await

* feat(btp/server): move to async/await

* feat(btp/service): move service to async/await

* We refactored the service to be more readable. Basically, we split the websocket in a Sink (write) and a Stream (read). We also create a `tx`/`rx` pair per account. The rx receiver gets attached to the sink, meaning any data sent over by the `tx` sender will get forwarded to the sink, which will forward it to the other end of the websocket. Unfortunately, due to being unable to combine the read and write sockets, we have to spawn them separately. This means that we have to remove the hook which cancels the streams.

# Interledger HTTP: Futures 0.3 Transition  (#600)

* feat(http): Update HttpStore trait to futures 0.3 and deserialize_json method

* feat(http): Update HTTP Errors and client

* feat(http): Update HTTP Server

* docs(http): extend http docs

# Interledger Stream: Futures 0.3 Transition  (#601)

* feat(stream): Update Stream server

* feat(stream): Update Stream client

* docs(stream): extend stream docs

* fix(stream): add extra limits to ensure all the pending request futures are thread safe

# Interledger Settlement: Futures 0.3 Transition  (#602)

* feat(settlement/core): Upgrade types and idempotency

* feat(settlement/core): Upgrade engines API Warp interface

* feat(settlement/core): Upgrade Redis backend implementation

* feat(settlement/api): Upgrade the message service

* feat(settlement/api): Upgrade the settlement client

* feat(settlement/api): Upgrade the Settlement API exposed by the node

* chore(settlement): remove need to pass future wrapped in closure

* docs(settlement): extend settlement docs

# Interledger SPSP: Futures 0.3 Transition (#603)

* feat(spsp): move to futures 0.3 and async/await

* docs(spsp): extend spsp docs

* fix(spsp): tighten trait bounds to account for stream changes

# Interledger Service Util: Futures 0.3 Transition  (#604)

* feat(service-util): update validator service

* feat(service-util): update rate limit service

* feat(service-util): update max packet amount service

* feat(service-util): update expiry shortener service

* feat(service-util): update exchange rate service and providers

* feat(service-util): update echo service

* feat(service-util): update balance service

# Interledger API: Futures 0.3 Transition  (#605)

* feat(api): update trait definitions and dependencies

* feat(api): update http retry client

* test(api): migrate test helpers

* feat(api): update node-settings route

* test(api): update node-settings route tests

* feat(api): update accounts route

* test(api): update accounts route tests

* chore(api): add missing doc

# Interledger Store: Futures 0.3 Transition (#606)

* feat(store): Update redis reconnect

* feat(store): Update base redis struct

* feat(store): Update AccountStore trait

* feat(store): Update StreamNotificationsStore trait

* feat(store): Update BalanceStore trait

* feat(store): Update BtpStore trait

* feat(store): Update HttpStore trait

* feat(store): Update NodeStore trait

* feat(store): Update AddressStore trait

* feat(store): Update RouteManagerStore trait

* feat(store): Update RateLimitStore trait

* feat(store): Update IdempotentStore trait

* feat(store): Update SettlementStore trait

* feat(store): Update LeftoversStore trait

* feat(store): Update update_routes

* test(store): convert all tests to tokio::test with async/await

* feat(store): update secrecy/bytes/zeroize

* docs(store): add more docs

# ILP CLI: Futures 0.3 Transition (#607)

* feat(ilp-cli): update CLI to async/await

# ILP Node: Futures 0.3 Transition (#608) (#609)

* test(ilp-node): migrate tests to futures 0.3

* feat(ilp-node): move metrics related files to feature-gated module

* feat(ilp-node): remove deprecated insert account function

* feat(ilp-node): make the node run on async/await

* ci(ilp-node): disable some advisories and update README

* fix(ilp-node): spawn prometheus filter
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.

Consider adding IntoBuf::len
3 participants