Allow max-optimization-thread config to be set to null on update#5520
Conversation
coszio
left a comment
coszio
left a comment
There was a problem hiding this comment.
Nice job, @gulshan-rs! However, there are interface changes still to be made for this to work as expected.
- GRPC interface needs to be updated as well with a new field (old field can be renamed, but can't change its numbering).
Surely it will need deprecation, but to allow zero-downtime upgrades, for now we will need to be able to read both fields (new and old). We can take care of safely removing the old field after a releasing a later version. - Right now the generated openapi model for
MaxOptimizationThreadsacceptsnullor a number, but we want"auto"or a number. - Please run
./tools/generate_openapi_models.sh(for REST) and./tools/generate_grpc_docs.sh(for grpc) after making the respective interface updates.
| .map_or(MaxOptimizationThreads::Auto, |n| { | ||
| MaxOptimizationThreads::Threads(n as usize) | ||
| }), |
There was a problem hiding this comment.
This means that even if max_optimization_threads is not specified in a grpc client, it will set it to Auto, which is not necessarily the intention.
There was a problem hiding this comment.
Thanks for the review and detailed comments. I will take another pass at it.
My initial intention was to not make any change in the GRPC interface so as to avoid any need of deprecation (tags) and worry about backward compatibility. Hence I decided to only change the inners i.e the OptimizersConfig and handle remaps via conversions -- but it seems broken. I will rerun integration tests locally to see what is broken or follow what you have suggested.
There was a problem hiding this comment.
Unfortunately we don't have a test which would break with this change yet, nor an easy framework for testing grpc interface. Most grpc changes are just validated with the compiler.
For REST, though, a test could be made to validate that setting a number or "auto" works ok. These tests usually take place at tests/openapi
|
I'm afraid we'll not be able to make this change, because as far as I understand is this will introduce a breaking change on the API level. Also note that we cannot use Sadly I don't have a better suggestion. We've been struggling with this limitation for quite a while. |
Ok. I am more concerned with this one, yes this will be breaking changes for existing customers. I am going to pause work and discard this PR. thankyou both for all your quick feedback. |
|
Thank you so much for giving it a shot though! I'll let you know if I think of another way to potentially approach this. |
ghost
closed this
2 participants
All Submissions:
Fixes #5243
New Feature Submissions:
cargo +nightly fmt --allcommand prior to submission?cargo clippy --all --all-featurescommand?Changes to Core Features: