You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2022/06/07 19:24:31 UTC
[GitHub] [druid] capistrant opened a new pull request, #12615: Modify process for deserializing DynamicCoordinatorConfig
capistrant opened a new pull request, #12615:
URL: https://github.com/apache/druid/pull/12615
<!-- Thanks for trying to help us make Apache Druid be the best it can be! Please fill out as much of the following information as is possible (where relevant, and remove it when irrelevant) to help make the intention and scope of this PR clear in order to ease review. -->
<!-- Please read the doc for contribution (https://github.com/apache/druid/blob/master/CONTRIBUTING.md) before making this PR. Also, once you open a PR, please _avoid using force pushes and rebasing_ since these make it difficult for reviewers to see what you've changed in response to their reviews. See [the 'If your pull request shows conflicts with master' section](https://github.com/apache/druid/blob/master/CONTRIBUTING.md#if-your-pull-request-shows-conflicts-with-master) for more details. -->
Fixes #11161.
<!-- Replace XXXX with the id of the issue fixed in this PR. Remove this section if there is no corresponding issue. Don't reference the issue in the title of this pull-request. -->
<!-- If you are a committer, follow the PR action item checklist for committers:
https://github.com/apache/druid/blob/master/dev/committer-instructions.md#pr-and-issue-action-item-checklist-for-committers. -->
### Description
<!-- Describe the goal of this PR, what problem are you fixing. If there is a corresponding issue (referenced above), it's not necessary to repeat the description here, however, you may choose to keep one summary sentence. -->
<!-- Describe your patch: what did you change in code? How did you fix the problem? -->
<!-- If there are several relatively logically separate changes in this PR, create a mini-section for each of them. For example: -->
#### 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](https://github.com/apache/druid/commit/bb3c810b368f2f49d33d24df38b078640d7d38d4#diff-3119db307661e92f88879f6465bfa3229f9e5c8cce0fa11c9487b2d7ded87fd7R140) Another reference to this conditional null check comes up in [this](https://github.com/apache/druid/pull/12504#discussion_r887380372) 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.
<!--
In each section, please describe design decisions made, including:
- Choice of algorithms
- Behavioral aspects. What configuration values are acceptable? How are corner cases and error conditions handled, such as when there are insufficient resources?
- Class organization and design (how the logic is split between classes, inheritance, composition, design patterns)
- Method organization and design (how the logic is split between methods, parameters and return types)
- Naming (class, method, API, configuration, HTTP endpoint, names of emitted metrics)
-->
<!-- It's good to describe an alternative design (or mention an alternative name) for every design (or naming) decision point and compare the alternatives with the designs that you've implemented (or the names you've chosen) to highlight the advantages of the chosen designs and names. -->
<!-- If there was a discussion of the design of the feature implemented in this PR elsewhere (e. g. a "Proposal" issue, any other issue, or a thread in the development mailing list), link to that discussion from this PR description and explain what have changed in your final design compared to your original proposal or the consensus version in the end of the discussion. If something hasn't changed since the original discussion, you can omit a detailed discussion of those aspects of the design here, perhaps apart from brief mentioning for the sake of readability of this PR description. -->
<!-- Some of the aspects mentioned above may be omitted for simple and small changes. -->
<hr>
##### Key changed/added classes in this PR
* `CoordinatorDynamicConfig`
<hr>
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
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.
--
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
[GitHub] [druid] kfaraz commented on a diff in pull request #12615: Modify process for deserializing DynamicCoordinatorConfig
Posted by GitBox <gi...@apache.org>.
kfaraz commented on code in PR #12615:
URL: https://github.com/apache/druid/pull/12615#discussion_r973585921
##########
server/src/main/java/org/apache/druid/server/coordinator/CoordinatorDynamicConfig.java:
##########
@@ -216,19 +216,29 @@ private static Set<String> parseJsonStringOrArray(Object jsonStringOrArray)
}
}
- public static AtomicReference<CoordinatorDynamicConfig> watch(final JacksonConfigManager configManager)
+ /**
+ * Setup a watch on the {@link CoordinatorDynamicConfig} in order to ensure access to the latest stored version of the config.
+ *
+ * Note that the {@link CoordinatorDynamicConfig.Builder} class is used here for serde because that allows clean setting of
+ * defaults for missing configuation keys. Missing configuration keys are common in cases such as the addition of a
+ * new config key in a new Druid version.
+ *
+ * @param configManager {@link JacksonConfigManager } responsible for managing persisted druid configuration content
+ * @return {@link AtomicReference} of {@link CoordinatorDynamicConfig.Builder}
+ */
+ public static AtomicReference<CoordinatorDynamicConfig.Builder> watch(final JacksonConfigManager configManager)
Review Comment:
The above javadoc could then be updated and moved to the `current()` method.
##########
server/src/main/java/org/apache/druid/server/coordinator/CoordinatorDynamicConfig.java:
##########
@@ -216,19 +216,29 @@ private static Set<String> parseJsonStringOrArray(Object jsonStringOrArray)
}
}
- public static AtomicReference<CoordinatorDynamicConfig> watch(final JacksonConfigManager configManager)
+ /**
+ * Setup a watch on the {@link CoordinatorDynamicConfig} in order to ensure access to the latest stored version of the config.
+ *
+ * Note that the {@link CoordinatorDynamicConfig.Builder} class is used here for serde because that allows clean setting of
+ * defaults for missing configuation keys. Missing configuration keys are common in cases such as the addition of a
+ * new config key in a new Druid version.
+ *
+ * @param configManager {@link JacksonConfigManager } responsible for managing persisted druid configuration content
+ * @return {@link AtomicReference} of {@link CoordinatorDynamicConfig.Builder}
+ */
+ public static AtomicReference<CoordinatorDynamicConfig.Builder> watch(final JacksonConfigManager configManager)
Review Comment:
I think we should also make this method private while we are at it as it is not being used anywhere except this class itself. We also already have the `current` method which is much more convenient. I can't think of a use case where `watch()` would be needed.
--
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
[GitHub] [druid] kfaraz commented on pull request #12615: Modify process for deserializing DynamicCoordinatorConfig
Posted by 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
Re: [PR] Modify process for deserializing DynamicCoordinatorConfig (druid)
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12615:
URL: https://github.com/apache/druid/pull/12615#issuecomment-1861898782
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 dev@druid.apache.org list.
Thank you for your contributions.
--
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
Re: [PR] Modify process for deserializing DynamicCoordinatorConfig (druid)
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12615:
URL: https://github.com/apache/druid/pull/12615#issuecomment-1894723235
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.
--
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
Re: [PR] Modify process for deserializing DynamicCoordinatorConfig (druid)
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #12615: Modify process for deserializing DynamicCoordinatorConfig
URL: https://github.com/apache/druid/pull/12615
--
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
[GitHub] [druid] kfaraz commented on pull request #12615: Modify process for deserializing DynamicCoordinatorConfig
Posted by GitBox <gi...@apache.org>.
kfaraz commented on PR #12615:
URL: https://github.com/apache/druid/pull/12615#issuecomment-1382666445
@capistrant , any thoughts on this?
https://github.com/apache/druid/pull/12615#issuecomment-1250083968
--
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