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

fix: use usize instead of u64 for batch-size arg #323

Merged
merged 1 commit into from
May 14, 2024

Conversation

KevFan
Copy link
Contributor

@KevFan KevFan commented May 14, 2024

Limitador Operator using the latest limitador image with redis has a failing test when starting up the limitador pod with the following error since #318 was introduced:

~ kubectl logs limitador-limitador-lcrp7-dffc56ff-qzccn -n test-namespace-8sgz7
thread 'main' panicked at /usr/local/cargo/registry/src/index.crates.io-6f17d22bba15001f/clap_builder-4.5.2/src/parser/error.rs:32:9:
Mismatch between definition and access of `batch`. Could not downcast to TypeId { t: (8519994227001858441, 10522819541147869382) }, need to downcast to TypeId { t: (11446210613632762899, 3222440509213045925) }

note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Seems to be related to mismatch of types defined at:

pub batch_size: usize,

vs
.value_parser(clap::value_parser!(u64))

Caveat: Not sure is this the correct change since I don't know Rust but changing the type and using a custom image with the change passed the failing test in limitador operator at least 🤷

@KevFan KevFan requested review from alexsnaps, a team and eguzki May 14, 2024 11:17
@alexsnaps
Copy link
Member

Thanks for catching this @KevFan
This whole i vs. u and 64 vs size are something I'd really like us to clean up entirely
i64 is abused afaict because that's the type you get from redis...

@KevFan KevFan merged commit b7984f5 into Kuadrant:main May 14, 2024
10 checks passed
Copy link
Member

@didierofrivia didierofrivia left a comment

Choose a reason for hiding this comment

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

🔎

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.

4 participants