wenjin272 commented on PR #688:
URL: https://github.com/apache/flink-agents/pull/688#issuecomment-4504719970
Hi, @twosom. Thanks for supporting EventListener in python sdk.
Overall, I think the current design has become somewhat overly complex. In
my view, we should have a single `PythonEventListenerWrapper` that, at runtime,
uses pemja to instantiate the corresponding Python `EventListener` object and
simply invokes its `on_event` method.
I think some of the current complexity stems from supporting
`str(MyPythonListener)`. Could we use a standard Python fully-qualified class
name (`"my_module.MyPythonListener"`) as the listener identifier, instead of
the `str(MyPythonListener)` → `"module:qualname.method_name"` protocol?
That would let us drop `EventListenerMeta`, `_resolve_module`, the
`<locals>` / `__main__` / `sys.modules` fallbacks, and the matching
`split(":")` on the Java side — Python-side instantiation collapses to
`importlib.import_module` + `getattr` + `rpartition(".")`.
The cost is that listeners must live at module top level (which the PR
already enforces against `<locals>`) and can't be defined in `__main__`.
Additionally, I believe there is no need to support event listeners in the
`local_runner`. In fact, we are also considering whether the `local_runner`
still needs to exist
Additionally, I believe the current testing is insufficient. We have some
unit tests, but no end-to-end tests. For this cross-language scenario, we need
to add real tests that submit jobs to the remote environment in
`e2e_tests_resource_cross_language`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
