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-3674 Add an interface for EventTime aware User Function (Ram) #2301

Closed
wants to merge 2 commits into from
Closed

FLINK-3674 Add an interface for EventTime aware User Function (Ram) #2301

wants to merge 2 commits into from

Conversation

ramkrish86
Copy link
Contributor

Thanks for contributing to Apache Flink. Before you open your pull request, please take the following check list into consideration.
If your changes take all of the items into account, feel free to open your pull request. For more information and/or questions please refer to the How To Contribute guide.
In addition to going through the list, please provide a meaningful description of your changes.

  • General
    • The pull request references the related JIRA issue ("[FLINK-XXX] Jira title text")
    • The pull request addresses only one issue
    • Each commit in the PR has a meaningful commit message (including the JIRA id)
  • Documentation
    • Documentation has been added for new functionality
    • Old documentation affected by the pull request has been updated
    • JavaDoc for public methods has been added
  • Tests & Build
    • Functionality added by the pull request is covered by tests
    • mvn clean verify has been executed successfully locally or a Travis build has passed


/** Flag to prevent duplicate function.close() calls in close() and dispose() */
private transient boolean functionsClosed = false;


public AbstractUdfStreamOperator(F userFunction) {
this.userFunction = requireNonNull(userFunction);
if(userFunction instanceof EventTimeFunction) {
Copy link
Contributor

Choose a reason for hiding this comment

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

missing space after if

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the super quick review. A quick question - In intellij IDE how should a formatting be applied? In Eclipse Ctrl+Shift+F applies the formatter that was configured. How about here? I can update the next commit based on that. Thank you.

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 got that. It is Ctrl+Alt+L here.

@ramkrish86
Copy link
Contributor Author

Created two interfaces EventTimeFunction and a WindowTimer (interface). The WindowTimer interface allows users to implement their own watermark timers and the way they could be fired. The EventTimeFunction could be used with any user defined functions and allows the creation of the WindowTimer. So the WindowOperator will see if the UDF is EventTimeFunction and uses the WindowTimer created by the EventimeFunction. If not it will use the DefaultTimer as in the current case. I ran mvn verify to ensure there are no checkstyle or compilation errors.
@aljoscha
Any feedback/ suggestions? This will now allow users to customize the timers.

@Override
public void apply(String key,
W window,
Iterable<Tuple2<String, Integer>> input,
Copy link
Contributor

Choose a reason for hiding this comment

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

this should fail checkstyle, as the indentation is partly done with spaces. we only use tabs.

@StephanEwen
Copy link
Contributor

Can we go back to the JIRA issue for a step, and first decide how we actually want his feature to look like?
For API additions, it is crucial to not do "something fast", but discuss and understand deeply what the feature should actually be like.

@StephanEwen
Copy link
Contributor

I would suggest to close this PR and start with a proper design discussion.
The approach taken here has a bunch of decisions/implications that I am not sure about, like

  • Mixing UDFs with "onWatermark" functionality with timers
  • Mixing windowed and not windowed code

In a clean design, this feature should not touch any window operator code.

Sorry for that, but that is why we encourage people to share designs before going full on implementing - at least for more complex additions like this one.

@ramkrish86
Copy link
Contributor Author

Closing this as discussed. I will wait for more feedback on what is expected here and then proceed.

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