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

Refactor cluster event service #4387

Merged
merged 1 commit into from Apr 29, 2020
Merged

Conversation

deepthidevaki
Copy link
Contributor

@deepthidevaki deepthidevaki commented Apr 27, 2020

Description

  • Use member properties to propagate subscription info instead of custom gossip
  • EventService can now only broadcast. Methods for unicast and send are removed from the interface.

Related issues

closes #4154
closes #4307

Pull Request Checklist

  • All commit messages match our commit message guidelines
  • The submitting code follows our code style
  • If submitting code, please run mvn clean install -DskipTests locally before committing

Copy link
Member

@Zelldon Zelldon left a comment

Choose a reason for hiding this comment

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

Cool 👍 much better I like it, just smaller things.

@deepthidevaki
Copy link
Contributor Author

@Zelldon I found one issue with adding and removing local subscriptions. For every new subscription it was adding new handler to messaging service which might invoke the same subscribers multiple times for a single message. Similarly when closing we should unregister the handler when local subscribers are empty. I have fixed it and added more tests. Please have a look.

@Zelldon
Copy link
Member

Zelldon commented Apr 29, 2020

@Zelldon I found one issue with adding and removing local subscriptions. For every new subscription it was adding new handler to messaging service which might invoke the same subscribers multiple times for a single message. Similarly when closing we should unregister the handler when local subscribers are empty. I have fixed it and added more tests. Please have a look.

Thanks @deepthidevaki. Cool that you found more issues. I think the thing with the multiple handler calls can't happen since internal a map is used, which just replaces the handler. Furthermore if I remove your changes only the test which verifies that the other subscription is called fails. But still I think both tests makes sense, so thanks for that.

@deepthidevaki
Copy link
Contributor Author

I think the thing with the multiple handler calls can't happen since internal a map is used, which just replaces the handler.

You are right. I mistook it for communication service where it keeps tracks of multiple handlers.

 * use member properties to propogate event subscription info instead of custom gossip
@deepthidevaki
Copy link
Contributor Author

bors r+

zeebe-bors bot added a commit that referenced this pull request Apr 29, 2020
4387: Refactor cluster event service r=deepthidevaki a=deepthidevaki

## Description

* Use member properties to propagate subscription info instead of custom gossip
* EventService can now only  `broadcast`. Methods for unicast and send are removed from the interface. 

## Related issues

<!-- Which issues are closed by this PR or are related -->

closes #4154 
closes #4307 

#

Co-authored-by: Deepthi Devaki Akkoorath <deepthidevaki@gmail.com>
@zeebe-bors
Copy link
Contributor

zeebe-bors bot commented Apr 29, 2020

Build failed

@deepthidevaki
Copy link
Contributor Author

bors retry

@zeebe-bors
Copy link
Contributor

zeebe-bors bot commented Apr 29, 2020

Build succeeded

@zeebe-bors zeebe-bors bot merged commit 86d86d1 into develop Apr 29, 2020
@zeebe-bors zeebe-bors bot deleted the 4154-fix-clusterevent-service branch April 29, 2020 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants