-
Couldn't load subscription status.
- Fork 14.7k
KAFKA-19803: Implemented support for allow.os.group.write.access config. #20744
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
Conversation
| <td><code class="docutils literal"><span class="pre">10000</span></code></td> | ||
| </tr> | ||
| <tr class="row-even"><td>allow.os.group.write.access</td> | ||
| <td>Medium</td> |
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.
Do we think Medium is right? Maybe Low would be better? -- Did not bring this up on the KIP. Let me follow up there about it.
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.
my bad, overlooked when copied the code. fixed
| } | ||
|
|
||
| private void initializeStateDirectory(final boolean createStateDirectory, final boolean hasNamedTopology, | ||
| final boolean allowOsGroupWriteAccess) throws IOException { |
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.
nit: formatting -- we usually either keep all parameter in a single line, or make one line per parameter. The current formatting is kinda hard to read.
streams/src/test/java/org/apache/kafka/streams/processor/internals/StateDirectoryTest.java
Show resolved
Hide resolved
|
We would expect that an existing state directory get updated when the config is changed, ie, either revoke group write or grant it? We should add a test for it. |
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.
LGTM.
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 think docs need some move updates though. We list all KIPs for each release in the upgrade section: https://github.com/apache/kafka/blob/trunk/docs/streams/upgrade-guide.html#L111 -- Seems we don't have a 4.2 section yet -- can you add one?
For config-streams.html, should we also add an entry to https://github.com/apache/kafka/blob/trunk/docs/streams/developer-guide/config-streams.html#L74, and add a corresponding paragraph after the table this PR already updates?
…ig. (apache#20744) Implements KIP-1230. Reviewers: Matthias J. Sax <[email protected]>
Implements KIP-1230.
Reviewers: Matthias J. Sax [email protected]