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-12092] [docs]Clarify when onTimer(...) is called #8106

Merged
merged 1 commit into from Oct 29, 2019
Merged

[FLINK-12092] [docs]Clarify when onTimer(...) is called #8106

merged 1 commit into from Oct 29, 2019

Conversation

an0
Copy link
Contributor

@an0 an0 commented Apr 3, 2019

No description provided.

@flinkbot
Copy link
Collaborator

flinkbot commented Apr 3, 2019

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit 81f7ac0 (Wed Dec 04 14:51:28 UTC 2019)

Warnings:

  • Documentation files were touched, but no .zh.md files: Update Chinese documentation or file Jira ticket.
  • This pull request references an unassigned Jira ticket. According to the code contribution guide, tickets need to be assigned before starting with the implementation work.

Mention the bot in a comment to re-run the automated checks.

Review Progress

  • ❓ 1. The [description] looks good.
  • ❓ 2. There is [consensus] that the contribution should go into to Flink.
  • ❗ 3. Needs [attention] from.
  • ❓ 4. The change fits into the overall [architecture].
  • ❓ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.


The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commands
The @flinkbot bot supports the following commands:

  • @flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
  • @flinkbot approve all to approve all aspects
  • @flinkbot approve-until architecture to approve everything until architecture
  • @flinkbot attention @username1 [@username2 ..] to require somebody's attention
  • @flinkbot disapprove architecture to remove an approval you gave earlier

event-/processing-time instants. When a timer's particular time is reached, the `onTimer(...)` method is
called. During that call, all states are again scoped to the key with which the timer was created, allowing
event-/processing-time instants. The `onTimer(...)` method is
called when such an event-time is first caught up by a watermark or such a processing-time is reached. During that call, all states are again scoped to the key with which the timer was created, allowing
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree we should explain what "when time is reached" means for both event and processing time, but I'm not sure of the actual changes in the PR.

Maybe something along the lines of:

When a timer's particular time is reached, the `onTimer(...)` method is called; for processing time, this would mean __________, while for event time, this would mean ______________.

Copy link
Contributor

Choose a reason for hiding this comment

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

@alpinegizmo what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest:

With event-time timers, the onTimer(...) method is called when the current watermark is advanced up to or beyond the timestamp of the timer, while with processing-time timers, onTimer(...) is called when wall clock time reaches the specified time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have a strong opinion about wording as long as it explains things clearly.

Copy link
Contributor

Choose a reason for hiding this comment

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

@an0 can you update the PR to what David suggested?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tzulitai Is it mergeable?

@tzulitai
Copy link
Contributor

tzulitai commented Apr 9, 2019

@flinkbot attention @alpinegizmo

@an0 an0 changed the title [FLINK-12092] [docs]Clarity when onTimer(...) is called [FLINK-12092] [docs]Claritf when onTimer(...) is called Apr 10, 2019
@an0 an0 changed the title [FLINK-12092] [docs]Claritf when onTimer(...) is called [FLINK-12092] [docs]Clarify when onTimer(...) is called Apr 10, 2019
@dawidwys
Copy link
Contributor

Hi @an0 thank you for your contribution and sorry for a late reply. I will merge it now.

@dawidwys dawidwys merged commit f79dda8 into apache:master Oct 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants