Skip to content

Conversation

sanjay24
Copy link

This feature allows Brooklin to make use of Kafka message's source timestamp when sending the
event to destination cluster. If the mirrored topic is configured with message.timestamp.type
set to CreateTime, the intent is to have the timestamp supplied by the application. This feature
will help achieving this intention.

This feature allows Brooklin to make use of Kafka message's source timestamp when sending the
event to destination cluster. If the mirrored topic is configured with message.timestamp.type
set to CreateTime, the intent is to have the timestamp supplied by the application. This feature
will help achieving this intention.
@sanjay24
Copy link
Author

@celiakung @ahmedahamid could you please review this PR?

@ahmedahamid
Copy link
Contributor

@sanjay24 Will do.

@ahmedahamid ahmedahamid requested a review from somandal February 28, 2020 01:50
@sanjay24
Copy link
Author

sanjay24 commented Mar 3, 2020

thanks @ahmedahamid @somandal

@mateuszmrozewski
Copy link

What is the expected timeline for to review this change @ahmedahamid, @somandal? It would be a very useful feature.

@ahmedahamid ahmedahamid self-requested a review March 12, 2020 22:33
@ahmedahamid
Copy link
Contributor

Will look into it by end of next week. @mateuszmrozewski

celiakung
celiakung previously approved these changes Mar 30, 2020
Added the following test-cases:
- When preserveEventSourceTimestamp is True
- When preserveEventSourceTimestamp is False
- When preserveEventSourceTimestamp is not configured
somandal
somandal previously approved these changes Apr 2, 2020
Copy link
Contributor

@somandal somandal left a comment

Choose a reason for hiding this comment

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

lgtm, thank you!

@sanjay24
Copy link
Author

sanjay24 commented Apr 3, 2020

@celiakung could you please check & approve the changes? Thanks

@sanjay24
Copy link
Author

sanjay24 commented Apr 8, 2020

There is still some more work required on this. I'll get back with my discovery and changes

…enabled

Current implementation passes event's source timestamp only when its timestamp type is
LogAppendTime, else it passes event's read time. This breaks the features.
This commit changes the following:
- Use event's source timestamp when the feature is enabled
- Resort to default behavior when feature is disabled
- Added associated test cases
@sanjay24
Copy link
Author

@ahmedahamid @celiakung @somandal KafkaMirrorMakerConnectorTask was translating the records to pass the source event's timestamp only when its timestamp type was "LogAppendTime". Do you know why was it done?
I've updated the PR to pass the correct timestamp when the feature is enabled.

@sanjay24
Copy link
Author

@ahmedahamid could we get moving on the review? Also, when is the next Brooklin release planned?

@sanjay24
Copy link
Author

@ahmedahamid Pinging you to get this reviewed and move ahead on this!

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.

5 participants