Modify process for deserializing DynamicCoordinatorConfig

[capistrant]

Fixes #11161.

Description

Modified the deserialization of DynamicCoordinatorConfig

The coordinator now uses the DynamicCoordinatorConfig.Builder.class for deserialization of the dynamic coordinator config. This allows us to automatically use the Builder specified default for a configuration key that does not exist in the metastore payload for the dynamic config. Doing so ensures that an "incomplete" config in the metastore will be supplemented with the proper Druid specified defaults every time it is deserialized.

Prior to this change, Druid used DynamicCoordinatorConfig.class directly for deserialization. The drawback on this is that DynamicCoordinatorConfig is less equipped for cleanly handling missing configuration keys during deserialization. If a developer used a Java primitive when adding a new config, on upgrade, the deserialization would use the Java system default for a missing value. This is often times very bad! For instance, I might add newIntConfig to Druid. Since this isn't in the previous version of Druid, upgrading the coordinator would cause deserialization of the legacy config to populate newIntConfig with the value 0. But as a developer, I needed newIntConfig to default to 100. To work-a-round this, I would flip the type to Integer which would deserialize a missing value as null. I could then check for this null value in the constructor and replace it with the desired default from the Builder class constants. You can see a more realistic example of this work-a-round here Another reference to this conditional null check comes up in this thread.

My change gets us away from this pattern. We are catching an undesirable state and deferring to the default in the Builder class already... so why not just use the Builder for deserialization is my thought. This way we can leverage the built in patterns for handling nulls and replacing them with constant defaults there instead of the DynamicCoordinatorConfig

My one worry is that I am missing an angle here that required us to forgo the Builder for this deserialization. At first instance, I figured that serialization used the same ConfigManager watcher and that using the builder for serde would cause issues in writing the config. However, this does not seem to be the case after running automated tests, manual review of the config update code path, and testing this change in my local druid cluster. Still, perhaps I am overlooking something that has prevented us from using the builder. That is what I'm hoping the review process can smoke out.

What I did not touch in this PR

I did not alter any of the code in the CoordinatorDynamicConfig constructor that is performing the deserialization work-a-round that my update is intended to prevent. I could certainly make that change here as well, but I wanted to be conservative with the scope of this change and guarantee no change in behavior of existing code. Removing the conditional checks on null and changing the variables to primitive types would change behavior IF there were any use of the CoordinatorDynamicConfig constructor directly. To my knowledge, unit tests are the only place this still happens.

---

Key changed/added classes in this PR

• CoordinatorDynamicConfig

---

This PR has:

• [X] been self-reviewed.
• [X] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
• [X] been tested in a test Druid cluster.

[kfaraz]

This is an interesting fix, @capistrant !

Some observations: A) For consistency with the read from config manager, 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().

There is a similar discrepancy between the POST and GET of the config. 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.

But 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 metadata store. 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?

[kfaraz]

@capistrant , any thoughts on this?

https://github.com/apache/druid/pull/12615#issuecomment-1250083968

[github-actions[bot]]

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the [email protected] list. Thank you for your contributions.

[github-actions[bot]]

This pull request/issue has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.