Skip to content

fix: make wal-protocol compile with no features #3137

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
Merged
merged 1 commit into from
Apr 16, 2025
Merged

fix: make wal-protocol compile with no features #3137

merged 1 commit into from
Apr 16, 2025

Conversation

lsytj0413
Copy link
Contributor

Fix: #3036

before fix:

cargo clippy --manifest-path crates/wal-protocol/Cargo.toml --no-default-features

error[E0277]: the trait bound `Envelope: serde::ser::Serialize` is not satisfied
  --> crates/wal-protocol/src/lib.rs:61:1
   |
61 | flexbuffers_storage_encode_decode!(Envelope);

@lsytj0413
Copy link
Contributor Author

lsytj0413 commented Apr 11, 2025 •
edited
Loading

I think after this PR, all crates will compile with each features and no-default-features. We should find an automated way to detect these compilation failure issues.

@lsytj0413
Copy link
Contributor Author

@tillrohrmann ptal

@tillrohrmann tillrohrmann self-requested a review April 15, 2025 13:43
tillrohrmann
tillrohrmann reviewed Apr 15, 2025
View reviewed changes
Copy link
Contributor

@tillrohrmann tillrohrmann left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning things up @lsytj0413. The change looks good to me. We might be able to make things a bit more cleaner by moving the append_envelope_to_bifrost to the bifrost crate. Then we can remove the circular dependency from restate-wal-protocol and avoid the cfg feature.

crates/wal-protocol/src/lib.rs Outdated
@@ -246,6 +250,7 @@ pub enum Error {
/// todo: This method should be removed in favor of using Appender/BackgroundAppender API in
/// Bifrost. Additionally, the check for partition_table is probably unnecessary in the vast
/// majority of call-sites.
#[cfg(feature = "serde")]
pub async fn append_envelope_to_bifrost(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this method to the bifrost crate? Then we can get rid of the circular dependency and the cfg feature here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tillrohrmann fixed, ptal. (i didn't have permissions to add reviewer

crates/wal-protocol/src/lib.rs Outdated
@@ -225,6 +228,7 @@ impl MatchKeyQuery for Envelope {
}
}

#[cfg(feature = "serde")]
#[derive(Debug, thiserror::Error)]
pub enum Error {
Copy link
Contributor

Choose a reason for hiding this comment

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

This error type would move to the bifrost crate as well then.

@lsytj0413 lsytj0413 force-pushed the fix-wal-protocol-compile-with-no-features branch 6 times, most recently from cfa5f24 to 3176ce6 Compare April 16, 2025 11:22
@lsytj0413 lsytj0413 requested a review from tillrohrmann April 16, 2025 11:26
tillrohrmann
tillrohrmann approved these changes Apr 16, 2025
View reviewed changes
Copy link
Contributor

@tillrohrmann tillrohrmann left a comment

Choose a reason for hiding this comment

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

Thanks a lot for updating the PR and removing the bifrost dependency from the wal-protocol crate. This is a really good clean up. I'll address my last comments while merging this PR.

crates/bifrost/src/error.rs Outdated
Comment on lines 46 to 47
#[error("partition not found: {0}")]
PartitionNotFound(#[from] PartitionTableError),
Copy link
Contributor

Choose a reason for hiding this comment

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

I would create a new AppendError that has this and Error as a variant to not infect Error with the partition lookup error.

crates/wal-protocol/src/lib.rs Outdated
Comment on lines 48 to 55
#[cfg(feature = "serde")]
pub fn to_bytes(&self) -> Result<Bytes, StorageEncodeError> {
let mut buf = BytesMut::default();
StorageCodec::encode(self, &mut buf)?;
Ok(buf.freeze())
}

#[cfg(feature = "serde")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can have a dedicated impl block that is feature gated together with the imports. That way the bits of the serde feature are kept closer together.

@tillrohrmann tillrohrmann force-pushed the fix-wal-protocol-compile-with-no-features branch from 3176ce6 to d0b2ca4 Compare April 16, 2025 17:19
@tillrohrmann tillrohrmann merged commit da4f296 into restatedev:main Apr 16, 2025
27 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 16, 2025
@lsytj0413 lsytj0413 deleted the fix-wal-protocol-compile-with-no-features branch April 16, 2025 23:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tillrohrmann tillrohrmann tillrohrmann approved these changes

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

Successfully merging this pull request may close these issues.

A lot of compiled failed with each-features
2 participants
@lsytj0413 @tillrohrmann