[GitHub] [druid] kfaraz commented on pull request #12615: Modify process for deserializing DynamicCoordinatorConfig

[GitBox <gi...@apache.org>]

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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org