kfaraz commented on PR #12615:
URL: https://github.com/apache/druid/pull/12615#issuecomment-1250083968
This is an interesting fix, @capistrant !
Some observations:
A) I was confused at first as the PR changes the deserialization when
reading from the config manager but not the serialization when writing to it.
Then I realized that it works because we have (purposefully?) named the json
properties in the exact same way in both the builder and the config classes.
I would have advised that you update the method
`DruidDynamicConfigsResource.setDynamicConfigs()` to write the Builder itself
to the manager, but then we would miss out on the validations happening in
`Builder.build()`.
Also, there is an existing discrepancy similar to the above. While the POST
API payload is read as a Builder, the response of the GET API is still the
actual config object. This probably makes sense as the user should be allowed
to omit nullable fields in the POST payload but while reading the config, they
should get back the fully validated and null-handled config.
I think we should call this out clearly in the javadocs that we must always:
- serialize as the config
- deserialize as the builder
- maintain the same property names in config and builder
- use the builder to instantiate
In this vein, we could also make the constructor of
`CoordinatorDynamicConfig` private
and get rid of all of its Json annotations.
B) Another fishy thing is the `Builder.build(CoordinatorDynamicConfig
defaults)`. I don't see the point of this method and it's being used in a weird
way. Effectively, when a user tries to set a new config but has omitted some
fields, we don't update those fields at all but take the existing values from
the metastore.
I am not sure but I think it violates REST. A POST must _fully_ update the
target object. We can certainly use default values but not old ones.
The fix here would be to simply get rid of this method and this weird logic.
Both API and console users would be affected as their old assumptions (if any)
about carrying forward omitted values would break.
If we do decide to make this change, we should call it out in release notes.
Although, this would not be a hindrance to the end goal of this PR and need
not be handled here.
@gianm , @capistrant , what do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
