Skip to content

[FLINK-8517] fix missing synchronization in TaskEventDispatcher#5621

Closed
NicoK wants to merge 1 commit intoapache:masterfrom
NicoK:flink-8517
Closed

[FLINK-8517] fix missing synchronization in TaskEventDispatcher#5621
NicoK wants to merge 1 commit intoapache:masterfrom
NicoK:flink-8517

Conversation

@NicoK
Copy link
Copy Markdown
Contributor

@NicoK NicoK commented Mar 2, 2018

What is the purpose of the change

The TaskEventDispatcher was missing synchronization accessing the registeredHandlers field for the new subscribeToEvent() and publish() methods. This was causing the StaticlyNestedIterationsITCase.testJobWithoutObjectReuse() test to sporadically fail (reproducible after running a couple of times).

Please merge into master and release-1.5 after accepting.

Brief change log

  • add synchronization around TaskEventDispatcher#subscribeToEvent()'s access to registeredHandlers
  • add synchronization around TaskEventDispatcher#publish()'s access to registeredHandlers

Verifying this change

This change is already covered by existing tests (indirectly), such as StaticlyNestedIterationsITCase.testJobWithoutObjectReuse(). I ran it almost 24000 times and could not reproduce it anymore with the change

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, Yarn/Mesos, ZooKeeper: no
  • The S3 file system connector: no

Documentation

  • Does this pull request introduce a new feature? no
  • If yes, how is the feature documented? not applicable

@NicoK
Copy link
Copy Markdown
Contributor Author

NicoK commented Mar 5, 2018

FYI: test failure is unrelated and relates to #5606 which apparently does not (completely) fix the issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants