nussknacker icon indicating copy to clipboard operation
nussknacker copied to clipboard

Fixup to async DefaultAsyncExecutionConfigPreparer

Open raphaelsolarski opened this issue 1 year ago • 2 comments

Fixup to async DefaultAsyncExecutionConfigPreparer - using class loader release hook instead of counting prepareExecutionContext/close method calls:

  • Closed ExecutorService could leak into execution after restart because lack of synchronization in close method (cached reference to Some in asyncExecutionContext var for thread)
  • prepare/close counter could fall below zero and lead to odd behaviour in cases when close was called without a prior open method call (e.g. in case of operator initializeState error - look into org.apache.flink.streaming.runtime.tasks.RegularOperatorChain.initializeStateAndOpenOperators and org.apache.flink.runtime.taskmanager.Task.restoreAndInvoke)

Checklist before merge

  • [ ] Added entry in Changelog.md describing the change from the perspective of a public distribution user
  • [ ] Added MigrationGuide.md entry in the appropriate subcategory if introducing a breaking change
  • [ ] Verify that PR will be squashed during merge

raphaelsolarski avatar Nov 14 '23 11:11 raphaelsolarski

created: #5007 :warning: Be careful! Snapshot changes are not necessarily the cause of the error. Check the logs.

github-actions[bot] avatar Nov 14 '23 12:11 github-actions[bot]

I'm still thinking about enabling this mechanism to work in our tests, because unfortunately there is a slightly different classloader arrangement in them, so DefaultAsyncExecutionConfigPreparer is not loaded in FlinkUserCodeClassLoader but in AppClassLoader

raphaelsolarski avatar Nov 14 '23 13:11 raphaelsolarski

fixed in https://github.com/TouK/nussknacker/pull/6055 and https://github.com/TouK/nussknacker/pull/6204

raphaelsolarski avatar Jul 30 '24 10:07 raphaelsolarski