Skip to content
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

feat: added support for message timestamp #915

Merged
merged 16 commits into from
Jul 23, 2024

Conversation

antonio-pedro99
Copy link
Member

@antonio-pedro99 antonio-pedro99 commented Jul 2, 2024

Description:
This PR fixes #912. It introduces enhancements to the Kafka bridge to support more flexible message timestamp handling by allowing producers to specify timestamps explicitly in ProducerRecord objects. Currently, the Kafka bridge does not fully utilize the timestamp parameter in the ProducerRecord constructors, leading to using the CreateTime message timestamp type every time we produce a message. On the other hand, the ConsumerRecord has also been updated with the timestamp parameter.

Changes:

  • Implemented support for interpreting the timestamp parameter in ProducerRecord objects sent to Kafka topics via the bridge.
  • Allow users to read the ConsumerRecord timestamp on the request's response. E.g
[
    {
        "topic": "bridge-quickstart-topic",
        "key": "my-key",
        "value": "sales-lead-0001",
        "partition": 0,
        "offset": 0,
        "timestamp": 1719663008510(new)
    },
    {
        "topic": "bridge-quickstart-topic",
        "key": "my-key",
        "value": "sales-lead-0001",
        "partition": 0,
        "offset": 1,
        "timestamp": 1719663054984(new)
    }
]
  • Updated the Kafka bridge logic to correctly handle timestamps specified by producers, aligning with configured Kafka broker timestamp policies (CreateTime or LogAppendTime).

Testing:

  • Unit tests were added to verify the correct interpretation and propagation of timestamp in ProducerRecord.
  • I am still adjusting to the BasicKafkaClient to change ConsumerIT with the required tests.

@antonio-pedro99 antonio-pedro99 marked this pull request as ready for review July 4, 2024 18:25
@ppatierno
Copy link
Member

@antonio-pedro99 I haven't reviewed the PR yes but I see this doc build failure here you should address ...

ERROR: Uncommitted changes in documentation:
M	documentation/book/api/definitions.adoc
Run the following to add up-to-date resources:
  make docu_api \
    && git add documentation/book/ \
    && git commit -s -m 'Update generated documentation'
make: *** [Makefile:67: docu_check] Error 1

after that I will start my review. Thanks!

@ppatierno ppatierno added this to the 0.30.0 milestone Jul 10, 2024
@antonio-pedro99
Copy link
Member Author

antonio-pedro99 commented Jul 14, 2024

@ppatierno sorry for being late.

Any clue about this error?

[ERROR] Failed to execute goal io.github.swagger2markup:swagger2markup-maven-plugin:1.3.7:convertSwagger2markup (generate-apidoc) on project kafka-bridge: Failed to execute goal 'convertSwagger2markup': Error creating extended parser class: Could not determine whether class 'org.pegdown.Parser$$parboiled' has already been loaded: Unable to make protected final java.lang.Class java.lang.ClassLoader.findLoadedClass(java.lang.String) accessible: module java.base does not "opens java.lang" to unnamed module @358ab600 -> [Help 1]

I have been trying to fix, but could not figure it out.

@ppatierno
Copy link
Member

Any clue about this error?

No idea :-(

@scholzj
Copy link
Member

scholzj commented Jul 14, 2024

Could it be a wrong Java version or something?

@ppatierno
Copy link
Member

@antonio-pedro99 we are going to plan a release. Are you still working to fix this on your side? or should we leave it out of 0.30.0?

@antonio-pedro99
Copy link
Member Author

@antonio-pedro99 we are going to plan a release. Are you still working to fix this on your side? or should we leave it out of 0.30.0?

Yeah, I am going to finish it soon.

@antonio-pedro99
Copy link
Member Author

Could it be a wrong Java version or something?

I guess so

@antonio-pedro99
Copy link
Member Author

antonio-pedro99 commented Jul 20, 2024

@ppatierno, we had a similar issue here #858 (comment)
Maybe we should try to push the files again?

I guess there is something wrong with my setup, I will double-check the contribution documentation.

Copy link
Member

@ppatierno ppatierno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have pushed the doc changes as done last time and left some minor comments.

@ppatierno ppatierno requested a review from a team July 22, 2024 08:49
@ppatierno
Copy link
Member

@antonio-pedro99 we also need the feature to be listed in the CHANGELOG.

Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like something definitely worth having it in the CHANGELOG. LGTM otherwise. Thanks for the PR

@antonio-pedro99
Copy link
Member Author

I have pushed the doc changes as done last time and left some minor comments.

Thanks a lot for this.
I will DM regarding what might be wrong with my setup.

antonio-pedro99 and others added 4 commits July 23, 2024 03:28
Signed-off-by: Antonio Pedro <tonio.pedro99@gmail.com>
Signed-off-by: Antonio Pedro <tonio.pedro99@gmail.com>
Signed-off-by: Antonio Pedro <tonio.pedro99@gmail.com>
Signed-off-by: Paolo Patierno <ppatierno@live.com>
Signed-off-by: Antonio Pedro <tonio.pedro99@gmail.com>
Signed-off-by: Antonio Pedro <tonio.pedro99@gmail.com>
Signed-off-by: Antonio Pedro <tonio.pedro99@gmail.com>
Signed-off-by: Antonio Pedro <tonio.pedro99@gmail.com>
ppatierno and others added 3 commits July 23, 2024 03:30
Signed-off-by: Paolo Patierno <ppatierno@live.com>
Signed-off-by: Antonio Pedro <tonio.pedro99@gmail.com>
Signed-off-by: Antonio Pedro <tonio.pedro99@gmail.com>
antonio-pedro99 and others added 6 commits July 23, 2024 03:48
Signed-off-by: Antonio Pedro <tonio.pedro99@gmail.com>
Signed-off-by: Antonio Pedro <tonio.pedro99@gmail.com>
Signed-off-by: Antonio Pedro <tonio.pedro99@gmail.com>
Signed-off-by: Paolo Patierno <ppatierno@live.com>
Signed-off-by: Paolo Patierno <ppatierno@live.com>
Copy link
Member

@ppatierno ppatierno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Antonio. I reworded the CHANGELOG addition and resolved a conflict. Now ready to be merged! LGTM ;-)

@ppatierno ppatierno merged commit 58b3fd7 into strimzi:main Jul 23, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

get message publish timestamp
3 participants