Copilot commented on code in PR #20544:
URL: https://github.com/apache/kafka/pull/20544#discussion_r2358023737
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java:
##########
@@ -296,7 +314,7 @@ public void
shouldLockCommitableTasksOnCorruptionWithProcessingThreads() {
@Test
public void shouldLockActiveOnHandleAssignmentWithProcessingThreads() {
final TasksRegistry tasks = mock(TasksRegistry.class);
- final TaskManager taskManager =
setUpTaskManager(ProcessingMode.AT_LEAST_ONCE, tasks, true, true);
+ final TaskManager taskManager =
setUpTaskManager(ProcessingMode.AT_LEAST_ONCE, tasks, true);
Review Comment:
Multiple test methods are calling the 3-parameter `setUpTaskManager` method
with `true` for the third parameter, but this creates ambiguity about which
overloaded method is being called. The new 3-parameter method expects
`processingThreadsEnabled` while the old 4-parameter method expects
`stateUpdaterEnabled` as the third parameter. This could lead to confusion and
potential bugs when the temporary method is removed.
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java:
##########
@@ -261,7 +279,7 @@ public void
shouldLockAllTasksOnCorruptionWithProcessingThreads() {
.inState(State.RUNNING)
.withInputPartitions(taskId00Partitions).build();
final TasksRegistry tasks = mock(TasksRegistry.class);
- final TaskManager taskManager =
setUpTaskManager(ProcessingMode.AT_LEAST_ONCE, tasks, true, true);
+ final TaskManager taskManager =
setUpTaskManager(ProcessingMode.AT_LEAST_ONCE, tasks, true);
Review Comment:
Multiple test methods are calling the 3-parameter `setUpTaskManager` method
with `true` for the third parameter, but this creates ambiguity about which
overloaded method is being called. The new 3-parameter method expects
`processingThreadsEnabled` while the old 4-parameter method expects
`stateUpdaterEnabled` as the third parameter. This could lead to confusion and
potential bugs when the temporary method is removed.
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java:
##########
@@ -216,7 +216,7 @@ public class TaskManagerTest {
@BeforeEach
public void setUp() {
- taskManager =
setUpTaskManager(StreamsConfigUtils.ProcessingMode.AT_LEAST_ONCE, null, false);
+ taskManager =
setUpTaskManager(StreamsConfigUtils.ProcessingMode.AT_LEAST_ONCE, null, false,
false);
Review Comment:
The method call now has two boolean parameters without clear meaning.
Consider using named parameters or method overloading to make the intent
clearer. The current call `setUpTaskManager(..., false, false)` is ambiguous
about what each boolean controls.
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java:
##########
@@ -283,7 +301,7 @@ public void
shouldLockCommitableTasksOnCorruptionWithProcessingThreads() {
.inState(State.RUNNING)
.withInputPartitions(taskId01Partitions).build();
final TasksRegistry tasks = mock(TasksRegistry.class);
- final TaskManager taskManager =
setUpTaskManager(ProcessingMode.AT_LEAST_ONCE, tasks, true, true);
+ final TaskManager taskManager =
setUpTaskManager(ProcessingMode.AT_LEAST_ONCE, tasks, true);
Review Comment:
Multiple test methods are calling the 3-parameter `setUpTaskManager` method
with `true` for the third parameter, but this creates ambiguity about which
overloaded method is being called. The new 3-parameter method expects
`processingThreadsEnabled` while the old 4-parameter method expects
`stateUpdaterEnabled` as the third parameter. This could lead to confusion and
potential bugs when the temporary method is removed.
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java:
##########
@@ -319,7 +337,7 @@ public void shouldLockAffectedTasksOnHandleRevocation() {
.inState(State.RUNNING)
.withInputPartitions(taskId01Partitions).build();
final TasksRegistry tasks = mock(TasksRegistry.class);
- final TaskManager taskManager =
setUpTaskManager(ProcessingMode.AT_LEAST_ONCE, tasks, true, true);
+ final TaskManager taskManager =
setUpTaskManager(ProcessingMode.AT_LEAST_ONCE, tasks, true);
Review Comment:
Multiple test methods are calling the 3-parameter `setUpTaskManager` method
with `true` for the third parameter, but this creates ambiguity about which
overloaded method is being called. The new 3-parameter method expects
`processingThreadsEnabled` while the old 4-parameter method expects
`stateUpdaterEnabled` as the third parameter. This could lead to confusion and
potential bugs when the temporary method is removed.
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/TaskManagerTest.java:
##########
@@ -339,7 +357,7 @@ public void shouldLockTasksOnClose() {
.inState(State.RUNNING)
.withInputPartitions(taskId01Partitions).build();
final TasksRegistry tasks = mock(TasksRegistry.class);
- final TaskManager taskManager =
setUpTaskManager(ProcessingMode.AT_LEAST_ONCE, tasks, true, true);
+ final TaskManager taskManager =
setUpTaskManager(ProcessingMode.AT_LEAST_ONCE, tasks, true);
Review Comment:
Multiple test methods are calling the 3-parameter `setUpTaskManager` method
with `true` for the third parameter, but this creates ambiguity about which
overloaded method is being called. The new 3-parameter method expects
`processingThreadsEnabled` while the old 4-parameter method expects
`stateUpdaterEnabled` as the third parameter. This could lead to confusion and
potential bugs when the temporary method is removed.
--
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]
