Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Deprecate the behaviour of implicitly disabling file/native realm #69320
Deprecate the behaviour of implicitly disabling file/native realm #69320
Changes from 5 commits
56e6602
35af827
f6deeb5
aa81b0b
cf73f6d
41a4320
542e05a
b30c21f
10b488c
5bf7cea
42e5b90
864e979
f1bb799
3a3c261
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we good?
What will this config do in 8.0?
In 7.x it will result in both a file and native realm enabled (I think)
I believe the proposal is that in 8.x it will result in just a file realm.
Should we issue a deprecation warning (because the behaviour will change) in this case too?
(A separate PR is fine though)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Yes this is indeed the proposal in the other PR #69096. I updated this PR to issue warning for this scenario as well. I'd like to mention that this change also means that the cluster will not automatically fallback to file/native realms when:
I don't know whether users rely on this "feature". I personally feel it is not something we should cater for because implicitly auto-enabling realms on the fly could be a security issue and also explicit behaviours are what we are after with these changes. All in all, if a realm is explicity disabled, I prefer it remain disabled at all times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"native" here means "file or native" right?
Do we need a better word?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I struggled with this and could not find a better word. I used
native
because that is what used in theRealms
class. I agree it is confusing. If possible, I would rename "native realm" to something like "index realm", but I think it would be a cognitive challenge for users. I also thought about the word "builtin" but all realms are kinda "builtin", i.e. part of default distribution. The word "internal" is also used for all realms. How about "basic"?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed all places to use the term "basic".