Skip to content

[Feature][Java] Add java event listeners config#641

Open
twosom wants to merge 9 commits intoapache:mainfrom
twosom:add_java_event_listeners_config
Open

[Feature][Java] Add java event listeners config#641
twosom wants to merge 9 commits intoapache:mainfrom
twosom:add_java_event_listeners_config

Conversation

@twosom
Copy link
Copy Markdown
Contributor

@twosom twosom commented Apr 25, 2026

Linked issue: #640

Purpose of change

Added event listeners config in java

Tests

ok

API

Documentation

  • doc-needed
  • doc-not-needed
  • doc-included

@github-actions github-actions Bot added doc-needed Your PR changes impact docs. fixVersion/0.3.0 The feature or bug should be implemented/fixed in the 0.3.0 version. priority/major Default priority of the PR or issue. labels Apr 25, 2026
Copy link
Copy Markdown
Collaborator

@wenjin272 wenjin272 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 your contribution, @twosom. Overall looks good to me. I left two minor comments.

Comment thread api/src/main/java/org/apache/flink/agents/api/listener/EventListener.java Outdated
@twosom
Copy link
Copy Markdown
Contributor Author

twosom commented May 2, 2026

Thanks for your contribution, @twosom. Overall looks good to me. I left two minor comments.

Thanks for the review! @wenjin272
I’ve applied both minor suggestions!
Please take another look when you have a moment. Thanks!

mailboxExecutor.submit(() -> tryProcessActionTaskForKey(key), "process action task");
}

notifyEventProcessed(event);
Copy link
Copy Markdown
Collaborator

@wenjin272 wenjin272 May 6, 2026

Choose a reason for hiding this comment

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

Hi, @twosom. I carefully reviewed the logic of processEvent. In this function, only an ActionTask is created and submitted. It's actual execution occurs in subsequent mailbox iterations. Therefore, merely moving notifyEventProcessed to the end of the function still remains inconsistent with the documentation of EventListener. After considering all factors, I think it's simpler to place it at the beginning and update the EventListener documentation accordingly.

Copy link
Copy Markdown
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 catch, @wenjin272

I missed the point that the actual execution happens in subsequent mailbox iterations. I agree that calling it at the beginning and updating the Javadoc is the best way to avoid inconsistency. I'll update the code and the documentation accordingly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi @wenjin272
I've updated the PR as suggested. Could you take another look? Thanks!

- Move notifyEventProcessed to the start of processEvent to ensure consistency with documentation.
- Update EventListener Javadoc to clarify that listeners are notified when an event is received.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-needed Your PR changes impact docs. fixVersion/0.3.0 The feature or bug should be implemented/fixed in the 0.3.0 version. priority/major Default priority of the PR or issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants