Skip to content

Conversation

@frankmcsherry
Copy link
Member

This PR introduces a trait Bytesable with methods from and into Bytes. It removes from communication any reliance on bincode, not because it is bad just because it is an opinion. It almost removes all dependence on serde, except that it creates several logging types that want to derive Serialize and Deserialize, but it is not otherwise reliant on them.

The trait that was formerly communication::Data is now instead communication::allocator::Exchangeable, and it looks like so:

/// A type that can be sent along an allocated channel.
pub trait Exchangeable : Send+Any+Bytesable+'static { }
impl<T: Send+Any+Bytesable+'static> Exchangeable for T { }

In exchange, timely gets a relatively small opinion, currently in the encoding module, that one can implement Bytesable with some reliance on serde and bincode. The opinions are expressed in a trait

    /// A composite trait for types that may be used with channels.
    pub trait Data : Send+Any+Serialize+for<'a>Deserialize<'a>+'static { }
    impl<T: Send+Any+Serialize+for<'a>Deserialize<'a>+'static> Data for T { }

which informs the timely::ExchangeData trait.

@frankmcsherry frankmcsherry force-pushed the bytesable_trait branch 3 times, most recently from 31f2783 to 1850d8e Compare November 10, 2024 21:39
@frankmcsherry
Copy link
Member Author

One thing I'd should tidy up are the unit tests in the doccomments. I copy/pasted in something that I knew would work, but it is a departure from what was there before. I don't know that they are bad, just .. different unintentionally.

@frankmcsherry
Copy link
Member Author

9d2ac9e moves the opinions all the way to the pacts, which are where we construct the channels. The default pacts use the bincode serialization strategy, but .. in the worst case the pacts are pluggable and can be implemented for other strategies. But probably the next thing to think through is how to have an opinion on the pact without having an opinion on the serialization.

Copy link
Member

@antiguru antiguru left a comment

Choose a reason for hiding this comment

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

Looks good, I think this is a worthwhile simplification. Some minor suggestions around leaving out 'static when it's implied by Any. No strong opinion though, it mostly removes some visual clutter.

@frankmcsherry
Copy link
Member Author

I'll take the communication documentation simplification as a future action item, to keep things moving along.

@frankmcsherry frankmcsherry merged commit 54974b9 into TimelyDataflow:master Nov 11, 2024
7 checks passed
@github-actions github-actions bot mentioned this pull request Nov 11, 2024
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.

2 participants