Clone kafka 18894 test #24
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add the verify_license.py script to our build to detect missing licenses. Reviewers: Chia-Ping Tsai <[email protected]>, Ken Huang <[email protected]>, David Arthur <[email protected]>
Add explicit not-null checks in Snapshot so we get a better error message in the event that a Snapshot object is accessed after erase has been called. Reviewers: David Arthur <[email protected]>
Adds `describeStreamsGroup` to Admin API. This exposes the result of the `DESCRIBE_STREAMS_GROUP` RPC in the Admin API. Reviewers: Bill Bejeck <[email protected]>
This patch allows for the immediatePeriodNs to be passed in when creating a periodic task Reviewers: Colin P. McCabe <[email protected]>
…pache#19151) If specified an invalid option then an exception trace appears with `kafka-client-metrics.sh` and `kafka-groups.sh` utilities. Then once has to explicitly remove the invalid argument and append `--help` to fetch correct options. The PR fixes below error message to one with `cause` and `usage`. This behaviour is similar to `kafka-console-consumer.sh` and `kafka-console-share-consumer.sh` Reviewers: Andrew Schofield <[email protected]>
The electionWasClean should also consider if the election is done through ELR. Otherwise, the metric uncleanLeaderElection will wrongly count the ELR election https://issues.apache.org/jira/browse/KAFKA-18940 Reviewers: Jun Rao <[email protected]>
…pache#19125) - Adding a space, article and punctuation to the Producer config doc strings for consistency and readability. Reviewers: TengYao Chi <[email protected]>, Ken Huang <[email protected]>, Justine Olshan <[email protected]>
…sponse (apache#19127) The kafka controllers need to set kraft.version in their ApiVersionsResponse messages according to the current kraft.version reported by the Raft layer. Instead, currently they always set it to 0. Also remove FeatureControlManager.latestFinalizedFeatures. It is not needed and it does a lot of copying. Reviewers: Jun Rao <[email protected]>, Chia-Ping Tsai <[email protected]>
…19160) Running `bin/kafka-features.sh upgrade --release-version 4.0` results in the following error. This PR fixes the issue by adding the required argument. `kafka-features: error: one of the arguments --bootstrap-server --bootstrap-controller is required.` Reviewers: Colin P. McCabe <[email protected]>
…pache#19146) Reviewers: Chia-Ping Tsai <[email protected]>
…, and LogAppendInfo to record classes (apache#19062) Migrate the following data carrier class to records to eliminate constructors, `equals`, `hashCode`, and `toString`. * `Entry` in `LogHistory` * `SnapshotPath` Additionally, migrate the following classes as discussed: * OffsetAndEpoch * LogFetchInfo * LogAppendInfo In Java, accessing a field in record class requires parentheses. In Scala, parentheses are not needed because Scala allows omitting them when calling parameterless methods; hence, there is no need to change the Scala code. Reviewers: Ken Huang <[email protected]>, Chia-Ping Tsai <[email protected]>
* Add `DynamicThreadPool.java` to the server module. * Remove the old DynamicThreadPool object in the `DynamicBrokerConfig.scala`. Reviewers: TengYao Chi <[email protected]>, Chia-Ping Tsai <[email protected]>
Remove unused `saslServerProperties`, `saslClientProperties`, `adminClientProperties`, `producerProperties`, and `consumerProperties` in ClusterConfig. First, I quickly fixed the unused adminClientProperties, and then I will move on to apache#19094 to fix the related issues. Pass AdminClientRebootstrapTest <img width="1398" alt="Screenshot 2025-03-09 at 12 54 57 PM" src="https://github.com/user-attachments/assets/73c50376-6602-493d-8abd-0eb2bb304114" /> Pass ClusterConfigTest <img width="1117" alt="Screenshot 2025-03-09 at 12 55 28 PM" src="https://github.com/user-attachments/assets/b4da59da-dfdf-4698-9077-5086854360ab" /> Reviewers: Chia-Ping Tsai <[email protected]>
Move AclPublisher to org.apache.kafka.metadata.publisher package. Reviewers: Christo Lolov <[email protected]>, Chia-Ping Tsai <[email protected]>
…o tool module (apache#17767) Reviewers: TengYao Chi <[email protected]>, Chia-Ping Tsai <[email protected]>
Reviewers: TengYao Chi <[email protected]>, Chia-Ping Tsai <[email protected]>
…'user1' (apache#19140) Reviewers: TengYao Chi <[email protected]>, Ken Huang <[email protected]>, Chia-Ping Tsai <[email protected]>
This PR addresses minor grammar and clarity issues in upgrade.html doc. Reviewers: mingdaoy <[email protected]>, Colin P. McCabe <[email protected]>, TengYao Chi <[email protected]>, Ken Huang <[email protected]>, Jhen-Yung Hsu <[email protected]>, Chia-Ping Tsai <[email protected]>
…apache#19099) Reviewers: Christo Lolov <[email protected]>, TengYao Chi <[email protected]>, Ken Huang <[email protected]>
Recently, we found a regression that could have been detected by static analysis, since a local variable wasn't being passed to a method during a refactoring, and was left unused. It was fixed in [7a749b5](apache@7a749b5), but almost slipped into 4.0. Unused variables are typically detected by IDEs, but this is insufficient to prevent these kinds of bugs. This change enables unused local variable detection in checkstyle for Kafka. A few notes on the usage: - There are two situations in which people actually want to have a local variable but not use it. First, there are `for (Type ignored: collection)` loops which have to loop `collection.length` number of times, but that do not use `ignored` in the loop body. These are typically still easier to read than a classical `for` loop. Second, some IDEs detect it if a return value of a function such as `File.delete` is not being used. In this case, people sometimes store the result in an unused local variable to make ignoring the return value explicit and to avoid the squiggly lines. - In Java 22, unsued local variables can be omitted by using a single underscore `_`. This is supported by checkstyle. In pre-22 versions, IntelliJ allows such variables to be named `ignored` to suppress the unused local variable warning. This pattern is often (but not consistently) used in the Kafka codebase. This is, however, not supported by checkstyle. Since we cannot switch to Java 22, yet, and we want to use automated detection using checkstyle, we have to resort to prefixing the unused local variables with `@SuppressWarnings("UnusedLocalVariable")`. We have to apply this in 11 cases across the Kafka codebase. While not being pretty, I'd argue it's worth it to prevent bugs like the one fixed in [7a749b5](apache@7a749b5). Reviewers: Andrew Schofield <[email protected]>, David Arthur <[email protected]>, Matthias J. Sax <[email protected]>, Bruno Cadonna <[email protected]>, Kirk True <[email protected]>
Reviewers: Ismael Juma <[email protected]>, Chia-Ping Tsai <[email protected]>
Infra has manually setup rulesets for trunk and our release branches (INFRA-26603). We need to disable the legacy branch protections to avoid interfering with the new rulesets. Reviewers: Chia-Ping Tsai <[email protected]>
…dinator is loaded (apache#19173) This PR adds `scheduleShareGroupSessionTimeout` for all the persisted members of a share group when the group coordinator is loaded. Reviewers: Andrew Schofield <[email protected]>
…nment and AbstractCoordinatorTest (apache#18945) Reviewers: Lianet Magrans <[email protected]>
This commit adds the conditions to decide when a Streams group heartbeat should be sent. A heartbeat should be sent when: - the group coordinator is available - the member is part of the Streams group or wants to join it - the heartbeat interval expired or the member is leaving the group or acknowledging the assginment This commit does not implement: - not sending fields that did not change - handling errors Reviewers: Zheguang Zhao <[email protected]>, Lucas Brutschy <[email protected]>
Add a step summary for the PR linter which shows all the errors in a more readable format. Reviewers: Chia-Ping Tsai <[email protected]>
The upgrade test in question is not supported for AK 3.3.2 due to a [known issue](https://issues.apache.org/jira/browse/KAFKA-18442). Previous attempt at solving this left the `metadata.log.dir` empty which leads to the following crash log: ``` ERROR Exiting Kafka due to fatal exception (kafka.Kafka$) org.apache.kafka.common.KafkaException: No `meta.properties` found in (have you run `kafka-storage.sh` to format the directory?) at kafka.server.BrokerMetadataCheckpoint$.$anonfun$getBrokerMetadataAndOfflineDirs$2(BrokerMetadataCheckpoint.scala:172) at scala.collection.Iterator.foreach(Iterator.scala:943) at scala.collection.Iterator.foreach$(Iterator.scala:943) at scala.collection.AbstractIterator.foreach(Iterator.scala:1431) at scala.collection.IterableLike.foreach(IterableLike.scala:74) at scala.collection.IterableLike.foreach$(IterableLike.scala:73) at scala.collection.AbstractIterable.foreach(Iterable.scala:56) at kafka.server.BrokerMetadataCheckpoint$.getBrokerMetadataAndOfflineDirs(BrokerMetadataCheckpoint.scala:161) at kafka.server.KafkaRaftServer$.initializeLogDirs(KafkaRaftServer.scala:184) at kafka.server.KafkaRaftServer.<init>(KafkaRaftServer.scala:61) at kafka.Kafka$.buildServer(Kafka.scala:79) at kafka.Kafka$.main(Kafka.scala:87) at kafka.Kafka.main(Kafka.scala) ```
In "Upgrading to 4.0.0 from any version 0.8.x through 3.9.x" section, we directly give instructions about [Upgrading to KRaft-based clusters](https://kafka.apache.org/documentation/#upgrade_390_kraft), but there might still be some users under ZK cluster before upgrading to v4.0.0. We need to make it clear that they need to upgrade to KRaft mode first before upgrading to v4.0.0 in "Upgrading to 4.0.0 from any version 0.8.x through 3.9.x" section. Reviewers: TengYao Chi <[email protected]>, Ken Huang <[email protected]>, Chia-Ping Tsai <[email protected]>
The current homogeneous SimpleAssignor for share groups is not very good at revoking partitions which have previously been assigned when the number of members increases. This PR improves the situation. It also fixes the sorting of assignments in `kafka-consumer-groups.sh` and `kafka-share-groups.sh` so that it sorts partition indices numerically instead of alphabetically. It also adds the missing number of partitions column for share groups.
…[1/N] (apache#19432) * We wish to track the time of creation of the `ShareSnapshot` records so that automated jobs could force their creation if a share partition has gone cold (no updates for a specified time interval). * To accomplish this, we have added 2 new fields `CreateTimestamp` and `WriteTimestamp` in the `ShareSnapshot` record. * The former tracks snapshot creation due to regular RPC calls while the latter will track snapshots created by periodic jobs. * In this PR we have made the requisite changes. * This is a first of a series of PRs to create the automated jobs and associated scaffolding. Reviewers: Andrew Schofield <[email protected]>
…19425) KIP-1071 creates internal topics broker-side, so this test checks whether, when KIP-1071 is enabled, basically the same topics are created. It also adds a little helper method in `EmbeddedKafkaCluster`, so that fewer code changes are required to enable KIP-1071. We use that helper in the already enabled SmokeTestDriverIntegrationTest and revert some of the changes there (making the cluster `final` again). Reviewers: Bill Bejeck <[email protected]>, PoAn Yang <[email protected]>
…pache#19421) In the first version of the integration of the stream thread with the new Streams rebalance protocol, the consumer used a dedicated event queue for Streams/specific background events to request the stream thread to call the rebalance callbacks. That led to an issue where the consumer times out when unsubscribing. This commit gets rid of the dedicated queue and incorporates the Streams-specific background events into event queue used by the consumer. Reviewers: Lucas Brutschy <[email protected]>
…apache#19363) * Currently, the delete share group code flow uses `group.subscribedTopicNames()` to fetch information about all the share partitions to which a share group is subscribed to. * However, this is incorrect since once the group is EMPTY, a precondition for delete, the aforementioned method will return an empty list. * In this PR we have leveraged the `ShareGroupStatePartitionMetadata` record to grab the `initialized` and `initializing` partitions to build the delete candidates, which remedies the situation. Reviewers: Andrew Schofield <[email protected]>
apache#19354) **Summary** Extend ApplicationRecoverableException related exceptions Reviewers: Artem Livshits <[email protected]>, Justine Olshan <[email protected]>
…umulator (apache#19399) Remove unused `ApiVersions` variable from Sender and RecordAccumulator. Reviewers: PoAn Yang <[email protected]>, Ken Huang <[email protected]>, Parker Chang <[email protected]>, Chia-Ping Tsai <[email protected]>
…ed parameter (apache#19410) It seems `timeMs` this parameter never used in Kafka project, the method init commit is apache@b5f90da Reviewers: Jhen-Yung Hsu <[email protected]>, PoAn Yang <[email protected]>, TengYao Chi <[email protected]>, Chia-Ping Tsai <[email protected]>
…tion-tests module (apache#19289) Use Java to rewrite `TransactionsWithMaxInFlightOneTest` by new test infra and move it to client-integration-tests module. Reviewers: Chia-Ping Tsai <[email protected]>
…#19435) Create a single formatter for use with `kafka-console-consumer.sh` that formats all record types for share groups on the `__consumer_offsets` topic.
…pache#19423) Add support for streams groups in kafka-groups.sh. The change adds command-line options `--streams` to list only streams groups, and value `--group-type streams`. Those two options are mutually exclusive with other group type and protocol filters specified on the command line. Includes a small integration test that spins up a kafka streams application and lists the group. Reviewers: Bill Bejeck <[email protected]>, Alieh Saeedii <[email protected]>
…pache#19438) Enable KIP-1071 parameter in `StandbyTaskCreationIntegrationTest`. Required a fix: In `ChangelogTopic.setup`, we actually need to return both the source-topic (optimized) and the non-source-topic changelog topics, since otherwise we will not find the partition number later on. Extended `EmbeddedKafkaCluster` to set the number of standby replicas dynamically for the group. We need to initialize it to one for the integration test to go through. Reviewers: Bill Bejeck <[email protected]>
Remove `OffsetConfig` which was used by old coordinator. Reviewers: PoAn Yang <[email protected]>, Ken Huang <[email protected]>, David Jacot <[email protected]>, TengYao Chi <[email protected]>, Chia-Ping Tsai <[email protected]>
…up configs (apache#19436) This PR introduces integration tests for verifying - altering configs - `describe` output for a streams group. The configs have been defined in `KIP-1071`. Reviewers: Lucas Brutschy <[email protected]>
This PR proposes a switch to enable share groups for 4.1 (preview) and 4.2 (GA). * `share.version=1` to indicate that share groups are enabled. This is used as the switch for turning share groups on and off. In 4.1, the default will be `share.version=0`. Then a user wanting to evaluate the preview of KIP-932 would use `bin/kafka-features.sh --bootstrap.server xxxx upgrade --feature share.version=1`. In 4.2, the default will be `share.version=1`. Reviewers: Jun Rao <[email protected]>
…apache#19447) There was a bug in `ConsoleShareConsumer` where the delivery count received in the `ShareFetchResponse` was not sent to the formatter to be shown to the user. PR fixes the bug by passing in the delivery count in a different constructor for `ConsumerRecord`. Reviewers: Andrew Schofield <[email protected]>
…che#19362) It is currently impossible to set both number of retries and idempotency in the DucktapeVerifiable producer. This change allows that to occur. Reviewers: PoAn Yang <[email protected]>, Manikumar Reddy <[email protected]>
Move LogCleaner and related classes to storage module and rewrite in Java. Reviewers: Mickael Maison <[email protected]>, Jun Rao <[email protected]>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. ✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai full review |
|
/gemini review |
✅ Actions performedFull review triggered. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Warning Gemini encountered an error creating the summary. You can try again by commenting |
|
/gemini summary |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a massive refactoring of the testing infrastructure, moving away from a dedicated quarantinedTest task to a more flexible tag-based system. It also includes significant updates to the consumer group protocol implementation, adds new admin client APIs for Streams and Share groups, and refactors various internal components. Many new test files and utility scripts have been added to support these changes. The overall direction seems to be towards a more modern and maintainable codebase. My review focuses on a couple of minor issues in the Python scripts.
Comment on lines
+213
to
+215
| test_tags.append("+github") | ||
| tags = [] | ||
| for tag in test_tags: |
There was a problem hiding this comment.
The test_tags list is modified in-place. This is a side effect that can be surprising for callers of this function. It's generally better to avoid modifying input arguments directly. Consider creating a new list that includes the additional tag.
Suggested change
| test_tags.append("+github") | |
| tags = [] | |
| for tag in test_tags: | |
| all_tags = test_tags + ["+github"] | |
| tags = [] | |
| for tag in all_tags: |
Comment on lines
+488
to
+489
| print(build_ids) | ||
| print(list(build_ids)) |
There was a problem hiding this comment.
These print statements appear to be for debugging purposes and should be removed before merging.
Summary of ChangesThis pull request significantly modernizes and expands the Kafka project's capabilities. It includes a major overhaul of the testing framework to provide more detailed and actionable insights into test stability and performance. Concurrently, it introduces a suite of new features for managing Kafka Streams applications and enhances the behavior of share consumers, offering more control over their operational aspects. These changes are supported by extensive updates to the build system and underlying dependencies, ensuring a more robust and maintainable codebase. Highlights
Changelog
Activity
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
61 participants
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.
Delete this text and replace it with a detailed description of your change. The
PR title and body will become the squashed commit message.
If you would like to tag individuals, add some commentary, upload images, or
include other supplemental information that should not be part of the eventual
commit message, please use a separate comment.
If applicable, please include a summary of the testing strategy (including
rationale) for the proposed change. Unit and/or integration tests are expected
for any behavior change and system tests should be considered for larger
changes.