Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

apply doesn't consider cluster defaults when settings are changed #46

Open
MrTrustworthy opened this issue Sep 7, 2019 · 1 comment
Labels
bug Something isn't working
Milestone

Comments

@MrTrustworthy
Copy link
Contributor

MrTrustworthy commented Sep 7, 2019

Example case:

Cluster default retention is 100ms
Create topic with retention of 50ms (ex. via esque apply)
Specify no retention for this topic in a config
Change the topic based on the config with esque apply

Expected: esque apply shows the change as "50ms -> 100ms" and applies this correctly when confirming

Actual: esque apply doesn't show the change. Not sure if it's applied correctly.

Notes:
This relates to the fact that we ignore None values for the new changes in AttributeDiffs. Instead, we should treat None values as ClusterDefault for this setting.
If we simply show the None, we'd get a diff for EVERYTHING that we haven't specified.
To solve, we need to do the following in the case that the new value is None: IF old_val IS NOT cluster_default THEN show change as old_val -> cluster_default and IF old_val IS cluster_default THEN show no change.

Open questions:
How do we get the cluster defaults?
What happens if we submit a None setting to the client when we change a topic? Will it revert to the cluster default? I'd assume yes, but this should also be tested (ideally as part of the test suite, I don't trust any of the clients to not have regressions on this at some point).

@MrTrustworthy MrTrustworthy added the bug Something isn't working label Sep 7, 2019
@MrTrustworthy MrTrustworthy added this to the 1.0.0 milestone Sep 7, 2019
@MrTrustworthy
Copy link
Contributor Author

Additional note: If it turns out that submitting a value of None for a configuration property causes the property to be automatically set to cluster default, we don't necessarily need to know the cluster defaults. Instead, we could store all changes in a topic as mentioned in #22 and use that info to decide whether we need to change something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant