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

make record compression into default features #86

Merged

Conversation

rukai
Copy link
Collaborator

@rukai rukai commented Sep 23, 2024

This PR moves support for each record compression type into its own cargo feature.
This will improve build times for projects that:

  • do not use the kafka-protocol crate's record types
  • specialized clients that only need to work with a subset of the compression types
  • clients that implement their own compression logic.

By default all of these new features are enabled since a regular client or broker would want to support all of them.

If a compression feature is not enabled it is up to the client or broker implementation to communicate that to the other side at the protocol level to avoid hitting the "support for {c:?} is not enabled as a cargo feature" error.

One downside is this has increased our CI runtime from 2 mins to 17 mins.
But I think this is tolerable and worth it.
In the future, if we find adding more features blows up the CI time to rediculous levels we can adjust cargo hack to avoid messages_enums in the main combinatorial run as that is the largest contributor to build time.

@rukai rukai force-pushed the make_record_compression_into_features branch from c55417c to 0aae295 Compare September 23, 2024 23:39
@rukai rukai marked this pull request as ready for review September 23, 2024 23:59
@rukai rukai mentioned this pull request Sep 24, 2024
Copy link
Owner

@tychedelia tychedelia left a comment

Choose a reason for hiding this comment

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

These 100% should be feature gated so the CI cost is fine, I think. Let's just keep an eye on it.

@tychedelia tychedelia merged commit 9d3cd22 into tychedelia:main Sep 24, 2024
3 checks passed
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