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-35289][core] Batch mode onTimer should support processing ts #24800

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jeyhunkarimov
Copy link
Contributor

What is the purpose of the change

Fix incorrect timestamp of stream elements collected from onTimer in batch mode.
In batch mode, onTimer calls will be processed at the end of the data. As a result, whichever timer registered by the users, they will all be triggered at the end. Therefore, it makes sense to trigger them with the processing time timestamp.

Brief change log

  • Adjust the timer processing logic of BatchExecutionInternalTimeService
  • Adjust tests

Verifying this change

Via tests in BatchExecutionInternalTimeServiceTest

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API, i.e., is any changed class annotated with @Public(Evolving): (no)
  • The serializers: (no)
  • The runtime per-record code paths (performance sensitive): (no)
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (no)
  • The S3 file system connector: (no)

Documentation

  • Does this pull request introduce a new feature? (no)

@flinkbot
Copy link
Collaborator

flinkbot commented May 16, 2024

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

@jeyhunkarimov
Copy link
Contributor Author

@flinkbot run azure

@reswqa
Copy link
Member

reswqa commented May 21, 2024

Thanks for the PR, but I'm not sure about this. AFAIK, processing time doesn't make much sense in batch mode. In particular, we return a processing time for event time timer.

@jeyhunkarimov
Copy link
Contributor Author

Hi @reswqa Thanks for your comment. The issue is that in batch mode all event-time triggers will be processed at the end. So, no matter what is the event-time set, they will be all triggered at the end of data. So, for example it doesnt make sense to process Long.MAX_VALUE event time - which will be triggered at the end of data with the timestamp Long.MAX_VALUE. WDYT?

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