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

Take with timestamp #242

Merged
merged 14 commits into from
Dec 17, 2021
Merged

Conversation

nicor88
Copy link
Contributor

@nicor88 nicor88 commented Dec 15, 2021

Add take_with_timestamp

Description

I'm currently using the take method of a stream in production. I have some cases where I would like to have not only the batch of values but a batch of values with the timestamp of when the message was inserted into kafka.

This PR adds a new method called take_with_timestamp, it works exactly like take, but it adds the Kafka timestamp to each returned value.

I tested in a local environment and seems working as expected, the real difference with take is this:

value[timestamp_field_name] = event.message.timestamp

the name of the timestamp must be passed as an argument, to force to pass the name, some people might like kafka_timestamp, some event_timestamp, I preferred to leave it generic.

@nicor88
Copy link
Contributor Author

nicor88 commented Dec 16, 2021

@patkivikram and @ekerstens I saw that you are pretty active, could you have a look? 🙏

@taybin
Copy link
Contributor

taybin commented Dec 16, 2021

The code looks clean to me, but I think for a user facing function, there really needs to be a unit test of some kind for it.

@nicor88
Copy link
Contributor Author

nicor88 commented Dec 16, 2021

@taybin could you have a look again? I added 3 tests, should cover the most essentials parts.

@codecov-commenter
Copy link

codecov-commenter commented Dec 17, 2021

Codecov Report

Merging #242 (22a1924) into master (8d8971a) will decrease coverage by 0.05%.
The diff coverage is 82.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #242      +/-   ##
==========================================
- Coverage   94.32%   94.26%   -0.06%     
==========================================
  Files         100      100              
  Lines       10762    10814      +52     
  Branches     1472     1477       +5     
==========================================
+ Hits        10151    10194      +43     
- Misses        542      549       +7     
- Partials       69       71       +2     
Impacted Files Coverage Δ
faust/streams.py 96.26% <82.69%> (-1.46%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d8971a...22a1924. Read the comment docs.

@nicor88
Copy link
Contributor Author

nicor88 commented Dec 17, 2021

@patkivikram if you have some time to look into that would be amazing, it's pretty trivial. Many thanks.

@patkivikram patkivikram merged commit a44fb6b into faust-streaming:master Dec 17, 2021
@nicor88 nicor88 deleted the take_with_timestamp branch December 17, 2021 21:18
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.

4 participants