-
Notifications
You must be signed in to change notification settings - Fork 8
Merge remote-tracking branch 'apache-kafka/trunk' into ccs-kafka/master #1796
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
omkreddy
commented
Oct 9, 2025
This patch updates the Apache Kafka project's build, test, and dependency configurations. - Java Version Update: The build and test processes have been upgraded from Java 24 to Java 25. - Scala Version Update: The Scala library version has been updated from 2.13.16 to 2.13.17. - Dependency Upgrades: Several dependencies have been updated to newer versions, including mockito (5.14.2 to 5.20.0), zinc (1.10.8 to 1.11.0), and scala-library/reflect (2.13.16 to 2.13.17). - Code and Configuration Changes: The patch modifies build.gradle to exclude certain Spotbugs tasks for Java 25 compatibility. It also changes the default signing algorithm in TestSslUtils.java from SHA1withRSA to SHA256withRSA, enhancing security. - Documentation: The README.md file has been updated to reflect the new Java 25 requirement. Reviewers: Ismael Juma <[email protected]>, Liam Clarke-Hutchinson <[email protected]>, Gaurav Narula <[email protected]>, Chia-Ping Tsai <[email protected]>
Updates all GitHub Actions to their latest versions. ---- **Upgraded Actions:** * **Gradle setup**: * `gradle/actions/setup-gradle` **v4.4.4 → v5.0.0** * **Trivy security scanner**: * `aquasecurity/trivy-action` **v0.24.0 → v0.33.1** * **Docker build tools:** * `docker/setup-qemu-action` **v3.2.0 → v3.6.0** * `docker/setup-buildx-action` **v3.6.1 → v3.11.1** * `docker/login-action` **v3.3.0 → v3.6.0** * **GitHub utilities:** * `actions/github-script` **v7 → v8** * `actions/stale` **v9 → v10** Reviewers: Chia-Ping Tsai <[email protected]>
…ache#20399) the constructor is error-prone when migrating code, since metrics could get unintentionally changed. We should remove the constructor and use constant strings instead to avoid issues like KAFKA-17876, KAFKA-19150, and others. Reviewers: Ken Huang <[email protected]>, Jhen-Yung Hsu <[email protected]>, KuoChe <[email protected]>, Chia-Ping Tsai <[email protected]>
Clear pendingTasksToInit on tasks clear. It matters in situations when we shutting down a thread in PARTITIONS_ASSIGNED state. In this case we may have locked some unassigned task directories (see TaskManager#tryToLockAllNonEmptyTaskDirectories). Then we may have gotten assigned to one or multiple of those tasks. In this scenario, we will not release the locks for the unassigned task directories (see TaskManager#releaseLockedUnassignedTaskDirectories), because TaskManager#allTasks includes pendingTasksToInit, but it hasn't been cleared. Reviewers: Matthias J. Sax <[email protected]>, Lucas Brutschy <[email protected]>
from: apache#20637 (comment) Previously, the method used LOGGER.info() instead of LOGGER.trace(). This patch corrects the logging level used in the trace method of StateChangeLogger. Reviewers: Manikumar Reddy <[email protected]>, TengYao Chi <[email protected]>, Ken Huang <[email protected]>, Chia-Ping Tsai <[email protected]> Co-authored-by: Ubuntu <[email protected]>
…he#20666) Fix the incorrect package name in metrics and revert the comment see: apache#20399 (comment) apache#20399 (comment) Reviewers: Ken Huang <[email protected]>, Manikumar Reddy <[email protected]>, Chia-Ping Tsai <[email protected]>
…20671) Stores the existing values for both the fields in a local variable for logging. Reviewers: Omnia Ibrahim <[email protected]>
…he flakiness of the test. (apache#20664) MINOR: changed the condition to only check the test topic to reduce the flakiness of the test. Reviewers: Matthias J. Sax <[email protected]>, Lianet Magrans <[email protected]>
Conflicts: gradle.properties - minor conflicts due to ccs version
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.
Pull Request Overview
This is a merge commit from the Apache Kafka trunk repository that incorporates upstream changes with a minor conflict resolution in gradle.properties due to CCS versioning. The merge updates various components including dependency versions, test improvements, metrics infrastructure changes, and GitHub Actions workflow updates.
Key changes include:
- Updated dependency versions (Scala, Mockito, Zinc) and GitHub Actions versions
- Refactored metrics infrastructure to use explicit package/class names instead of reflection
- Enhanced test coverage with new test methods and improved assertions
- Fixed logging level in StateChangeLogger from info to trace
Reviewed Changes
Copilot reviewed 39 out of 42 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
gradle/dependencies.gradle | Updates Scala, Mockito, and Zinc dependency versions |
gradle.properties | Updates Scala version with CCS version conflict resolution |
server-common/src/main/java/org/apache/kafka/server/metrics/KafkaMetricsGroup.java | Removes deprecated constructor that used reflection |
Multiple core Scala files | Replaces reflection-based metrics with explicit package/class names |
server-common/src/main/java/org/apache/kafka/logger/StateChangeLogger.java | Fixes logging level from info to trace |
Various test files | Adds new tests and improves existing test assertions |
GitHub workflow files | Updates action versions for security and compatibility |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
assertEquals("", underReplicatedOutput, | ||
String.format("--under-replicated-partitions shouldn't return anything: '%s'", underReplicatedOutput)); | ||
assertFalse(underReplicatedOutput.contains(testTopicNameLogLine), | ||
String.format("--under-replicated-partitions shouldn't contain '%s': '%s'", testTopicNameLogLine, underReplicatedOutput)); |
Copilot
AI
Oct 9, 2025
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.
[nitpick] The test logic has changed from checking for empty output to checking that output doesn't contain the topic name. This is a less strict assertion that could miss other unexpected output. Consider adding an additional check to ensure the output is actually empty or only contains expected content.
String.format("--under-replicated-partitions shouldn't contain '%s': '%s'", testTopicNameLogLine, underReplicatedOutput)); | |
String.format("--under-replicated-partitions shouldn't contain '%s': '%s'", testTopicNameLogLine, underReplicatedOutput)); | |
assertTrue(underReplicatedOutput.trim().isEmpty(), | |
String.format("--under-replicated-partitions output should be empty, but was: '%s'", underReplicatedOutput)); |
Copilot uses AI. Check for mistakes.
return pendingActiveTasksToCreate; | ||
} | ||
|
||
Map<TaskId, Set<TopicPartition>> pendingStandbyTasksToCreate() { | ||
return pendingStandbyTasksToCreate; |
Copilot
AI
Oct 9, 2025
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.
These new methods expose internal mutable collections directly without defensive copying. This violates encapsulation and could allow external modification of internal state. Consider returning unmodifiable views or defensive copies.
return pendingActiveTasksToCreate; | |
} | |
Map<TaskId, Set<TopicPartition>> pendingStandbyTasksToCreate() { | |
return pendingStandbyTasksToCreate; | |
Map<TaskId, Set<TopicPartition>> unmodifiableMap = new HashMap<>(); | |
for (Map.Entry<TaskId, Set<TopicPartition>> entry : pendingActiveTasksToCreate.entrySet()) { | |
unmodifiableMap.put(entry.getKey(), Collections.unmodifiableSet(entry.getValue())); | |
} | |
return Collections.unmodifiableMap(unmodifiableMap); | |
} | |
Map<TaskId, Set<TopicPartition>> pendingStandbyTasksToCreate() { | |
Map<TaskId, Set<TopicPartition>> unmodifiableMap = new HashMap<>(); | |
for (Map.Entry<TaskId, Set<TopicPartition>> entry : pendingStandbyTasksToCreate.entrySet()) { | |
unmodifiableMap.put(entry.getKey(), Collections.unmodifiableSet(entry.getValue())); | |
} | |
return Collections.unmodifiableMap(unmodifiableMap); |
Copilot uses AI. Check for mistakes.
val isNewLeaderEpoch = partitionRegistration.leaderEpoch > leaderEpoch | ||
// The leader should be updated before updateAssignmentAndIsr where we clear the ISR. Or it is possible to meet | ||
// the under min isr condition during the makeFollower process and emits the wrong metric. | ||
val prevLeaderReplicaIdOpt = leaderReplicaIdOpt | ||
val prevLeaderEpoch = leaderEpoch |
Copilot
AI
Oct 9, 2025
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.
The variables prevLeaderReplicaIdOpt
and prevLeaderEpoch
capture the current values before they are updated, but these values will be the same as the new values if this is not a new leader epoch. The logging message will be incorrect in cases where the leader hasn't changed.
val isNewLeaderEpoch = partitionRegistration.leaderEpoch > leaderEpoch | |
// The leader should be updated before updateAssignmentAndIsr where we clear the ISR. Or it is possible to meet | |
// the under min isr condition during the makeFollower process and emits the wrong metric. | |
val prevLeaderReplicaIdOpt = leaderReplicaIdOpt | |
val prevLeaderEpoch = leaderEpoch | |
val prevLeaderReplicaIdOpt = leaderReplicaIdOpt | |
val prevLeaderEpoch = leaderEpoch | |
val isNewLeaderEpoch = partitionRegistration.leaderEpoch > leaderEpoch | |
// The leader should be updated before updateAssignmentAndIsr where we clear the ISR. Or it is possible to meet | |
// the under min isr condition during the makeFollower process and emits the wrong metric. |
Copilot uses AI. Check for mistakes.