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

[FLINK-23926] one timestamp #36

Merged
merged 2 commits into from
Sep 3, 2021

Conversation

alpinegizmo
Copy link
Contributor

This PR is layered on top of #31.

The objective is to replace the startTime and endTime fields with a single eventTime field. This is more natural, and will make it more straightforward to convert a DataStream<TaxiRide> into a table.

This does affect the long rides exercise. The biggest change is that it is no longer possible to determine if a ride has been too long in the case where the START event is missing.

@alpinegizmo alpinegizmo force-pushed the FLINK-23926-one-timestamp branch 3 times, most recently from 709a68f to 5a3bfcf Compare August 31, 2021 02:01
@alpinegizmo alpinegizmo requested a review from NicoK August 31, 2021 03:58
Copy link
Contributor

@NicoK NicoK left a comment

Choose a reason for hiding this comment

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

Thanks for getting this change in - it maps better to the streaming approach.

I have a couple of smaller things around the code but once addressed, we can merge

@NicoK NicoK changed the title Flink 23926 one timestamp [FLINK-23926] one timestamp Sep 1, 2021
Copy link
Contributor

@NicoK NicoK left a comment

Choose a reason for hiding this comment

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

One tiny bit in a sentence seems wrong. I'll approve now assuming you'll address that one

long-ride-alerts/DISCUSSION.md Outdated Show resolved Hide resolved
@alpinegizmo alpinegizmo force-pushed the FLINK-23926-one-timestamp branch 2 times, most recently from 961be5d to a7deba1 Compare September 1, 2021 21:01
long-ride-alerts/DISCUSSION.md Outdated Show resolved Hide resolved
@alpinegizmo
Copy link
Contributor Author

@NicoK I've reworked the DISCUSSION for the long-rides exercise again. It should be correct now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants