-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[fix][io] Make record properties configurable for kinesis source #24495
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
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #24495 +/- ##
============================================
+ Coverage 73.57% 74.28% +0.71%
- Complexity 32624 32843 +219
============================================
Files 1877 1868 -9
Lines 139502 145902 +6400
Branches 15299 16728 +1429
============================================
+ Hits 102638 108390 +5752
- Misses 28908 28914 +6
- Partials 7956 8598 +642
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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
pulsar-io/kinesis/src/test/java/org/apache/pulsar/io/kinesis/KinesisSourceConfigTests.java
Show resolved
Hide resolved
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 PR makes Kinesis metadata properties configurable and fixes a key-collision bug by giving each property a unique key. It updates the source configuration, record processor, and record class to honor a user-defined list of properties, and adds unit tests for the new behavior.
- Introduce
kinesisRecordProperties
config to select which metadata properties to include - Replace empty-string constants in
KinesisRecord
with descriptive keys and conditionally set them - Update processor and config loading logic, and add tests for default, custom, and empty property lists
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
KinesisSourceConfig.java | Add kinesisRecordProperties field, parse it into a propertiesToInclude set |
KinesisRecordProcessor.java | Store and pass propertiesToInclude into each KinesisRecord ; update logging |
KinesisRecord.java | Replace empty constants with descriptive keys; conditionally populate properties |
KinesisSourceConfigTests.java | Add tests for default, custom, and empty property configurations |
KinesisRecordTest.java | Add tests covering all/some/none property inclusion scenarios |
Comments suppressed due to low confidence (1)
pulsar-io/kinesis/src/test/java/org/apache/pulsar/io/kinesis/KinesisSourceConfigTests.java:176
- The default properties test only verifies the total count and two keys; consider asserting all six default properties are present to ensure full coverage.
assertEquals(properties.size(), 6);
pulsar-io/kinesis/src/main/java/org/apache/pulsar/io/kinesis/KinesisRecordProcessor.java
Show resolved
Hide resolved
…ackward compatibility
) (cherry picked from commit e0efcbb)
Motivation
The Kinesis source connector's handling of metadata properties was rigid and contained a critical bug.
Properties were not configurable: Users could not select which Kinesis metadata properties to include in Pulsar messages. This forced all properties to be included, which could be inefficient in terms of message size.
Key Collision Bug: A bug was discovered where all property keys were defined as an empty string (""), causing a key collision that resulted in the loss of all metadata except for the last one set (sequenceNumber).
This PR fixes these issues by introducing a configuration to control which properties are included, which also resolves the underlying bug.
Modifications
Verifying this change
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
doc
doc-required
doc-not-needed
doc-complete
Matching PR in forked repository
PR in forked repository: