mjsax commented on code in PR #20744:
URL: https://github.com/apache/kafka/pull/20744#discussion_r2449964932
##########
streams/src/main/java/org/apache/kafka/streams/StreamsConfig.java:
##########
@@ -1015,6 +1020,11 @@ public class StreamsConfig extends AbstractConfig {
// LOW
+ .define(ALLOW_OS_GROUP_WRITE_ACCESS_CONFIG,
+ Type.BOOLEAN,
+ false,
+ Importance.LOW,
Review Comment:
Here you set low -- We need to keep the code and cods consistent.
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/StateDirectoryTest.java:
##########
@@ -101,17 +102,21 @@ public class StateDirectoryTest {
private File appDir;
private void initializeStateDirectory(final boolean createStateDirectory,
final boolean hasNamedTopology) throws IOException {
+ initializeStateDirectory(createStateDirectory, hasNamedTopology,
false);
+ }
+
+ private void initializeStateDirectory(final boolean createStateDirectory,
final boolean hasNamedTopology,
+ final boolean allowOsGroupWriteAccess) throws IOException {
Review Comment:
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:
##########
@@ -140,7 +145,18 @@ public void shouldHaveSecurePermissions() {
assertPermissions(appDir);
}
+ @Test
+ public void shouldHaveSecurePermissionsIfGroupWriteAccessAllowed() throws
IOException {
+ initializeStateDirectory(true, false, true);
Review Comment:
Should we cleanup/delete the state directory we create in
```
@BeforeEach
public void before() throws IOException {
initializeStateDirectory(true, false);
}
```
before we init a new one?
##########
docs/streams/developer-guide/config-streams.html:
##########
@@ -261,11 +261,16 @@ <h4><a class="toc-backref"
href="#id45">num.standby.replicas</a><a class="header
</tr>
</thead>
<tbody valign="top">
- <tr class="row-even"><td>acceptable.recovery.lag</td>
+ <tr class="row-odd"><td>acceptable.recovery.lag</td>
<td>Medium</td>
<td colspan="2">The maximum acceptable lag (number of offsets to
catch up) for an instance to be considered caught-up and ready for the active
task.</td>
<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>
Review Comment:
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.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
