-
Notifications
You must be signed in to change notification settings - Fork 44
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
Quantity fields should be human-readable #862
Comments
Hi @aaronmondal I was thinking of taking on this issue if still available. |
Hi @matdexir feel free to take the issue ❤️ Personally, I'd be in favor of just removing the int support altogether to keep the implementation simple, (otherwise the field would have to support both strings and integers). I believe this can be done by adding something like a #[derive(Serialize, Deserialize, Debug, Clone)]
#[serde(deny_unknown_fields)]
pub struct SizePartitioningStore {
/// Size to partition the data on.
#[serde(deserialize_with = "convert_numeric_with_shellexpand")]
pub size: u64,
// ...
} into this: #[derive(Serialize, Deserialize, Debug, Clone)]
#[serde(deny_unknown_fields)]
pub struct SizePartitioningStore {
/// Size to partition the data on.
#[serde(deserialize_with = "convert_numeric_with_shellexpand")]
pub size: Quantity,
// ...
} cc @MarcusSorealheis @allada and @adam-singer @blakehatch wdyt? I believe the part that needs to be thought out here is how we can best propagate this information to monitoring services. Under the hood a |
To be honest, I'd prefer to always use primitives (non-complex structs) in our config. Some day in the future there's a reasonable chance we'll need our structs to be FFI compliant, and using raw primitives is going to make things easier. As for how to do this, I'd keep them as {
"size1": "5kB"
"size2": "5KiB",
"size3": "5GiB",
"duration1": "5s",
"duration2": "5m",
"duration3": "5h"
} Would result in a struct like (respectively): Data {
size1: 1000,
size2: 1024,
size3: 1073741824,
duration1: 5,
duration2: 300,
duration3: 18000
} (obviously we have some places that take milliseconds, but lets ignore those for now, we can figure out what to do with them later). It shouldn't be too difficult to make one that takes shell_expand and a suffix qualifier. I'd even argue we could probably convert what is already there and we wouldn't even need to create a new deserialization function for it. |
Hi, Here's a sample of how I think it would go: preview gist For this use case I would only implement
Then implement deserialization accordingly, although, I do not think that keeping the unit would be of much value 🤔. |
This is pretty close to what I was thinking. There are a few minor differences, the struct's must use only FFI compatible primitives (String, Enum, u64, usize, float64, exc...). I suggest using the following libraries that will do the conversion though. I'm pretty sure just wrapping these in a function should do much of the work: Thanks :-) |
I would like to contribute on this issue. @allada @MarcusSorealheis @aaronmondal |
@matdexir, @aleksdmladenovic and us had a small conversation on slack. I assume you wanted to take this one on? |
Yes, now I am working on the issue for reducing the verbosity of ping message. |
Hi @allada |
Thanks to both. @matdexir has already put work into it, so lets give some time for them to accomplish it. |
Instead of this:
We could have this which seems more readable and is standardized in the Kubernetes Quantity format:
The text was updated successfully, but these errors were encountered: