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.

Sign up for GitHub

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

Jump to bottom

refactor(op-store)!: pass ops as bytes to op store #71

Merged
merged 1 commit into from
Jan 14, 2025
Merged

refactor(op-store)!: pass ops as bytes to op store #71

merged 1 commit into from
Jan 14, 2025

Conversation

jost-s
Copy link
Contributor

@jost-s jost-s commented Jan 14, 2025

Refactor the op store trait to take opaque bytes instead of MetaOps when processing incoming ops. I think that's already it. Retrieving ops from the store should still be MetaOps.

@jost-s jost-s force-pushed the refactor/pass-op-bytes-to-op-store branch from 3c4aca5 to 2882924 Compare January 14, 2025 03:05
@jost-s jost-s requested a review from a team January 14, 2025 03:08
c12i
c12i previously approved these changes Jan 14, 2025
View reviewed changes
Copy link

@c12i c12i left a comment

Choose a reason for hiding this comment

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

LGTM

ThetaSinner
ThetaSinner previously approved these changes Jan 14, 2025
View reviewed changes
Copy link
Member

@ThetaSinner ThetaSinner 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 otherwise!

Are all the From/TryFrom conversions in mem_op_store.rs still used? There's a conversion between mem ops and meta ops that also used serialization that might not be useful any more?

crates/api/src/op_store.rs Outdated Show resolved Hide resolved
@jost-s jost-s dismissed stale reviews from ThetaSinner and c12i via 0e74e52 January 14, 2025 13:12
@jost-s jost-s force-pushed the refactor/pass-op-bytes-to-op-store branch from 2882924 to 0e74e52 Compare January 14, 2025 13:12
@jost-s
Copy link
Contributor Author

jost-s commented Jan 14, 2025

Are all the From/TryFrom conversions in mem_op_store.rs still used? There's a conversion between mem ops and meta ops that also used serialization that might not be useful any more?

They weren't, deleted the Kitsune2MemoryOp conversions.

@jost-s jost-s requested a review from ThetaSinner January 14, 2025 13:14
ThetaSinner
ThetaSinner previously approved these changes Jan 14, 2025
View reviewed changes
Copy link
Member

@ThetaSinner ThetaSinner left a comment

Choose a reason for hiding this comment

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

Nice, looking good!

@jost-s jost-s force-pushed the refactor/pass-op-bytes-to-op-store branch from 0e74e52 to ade70b7 Compare January 14, 2025 13:17
@jost-s
Copy link
Contributor Author

jost-s commented Jan 14, 2025

Apologies, I addressed another windows test flake. Windows getting slower by the day obviously.

@jost-s jost-s enabled auto-merge (rebase) January 14, 2025 13:18
@jost-s jost-s merged commit 4b2f5f8 into main Jan 14, 2025
5 checks passed
@jost-s jost-s deleted the refactor/pass-op-bytes-to-op-store branch January 14, 2025 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ThetaSinner ThetaSinner ThetaSinner approved these changes

@c12i c12i c12i left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jost-s @ThetaSinner @c12i