-
Notifications
You must be signed in to change notification settings - Fork 95
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
EventStream refactoring #238
Conversation
f23532f
to
1677555
Compare
JFI: I pushed a rebased version of these changes against v0.14 in https://github.com/SUSE/salt-netapi-client/tree/v0.15.0-event-stream-refactoring I am not opening a PR unless you tell me otherwise. |
protected void clearListeners(int code, String phrase) { | ||
synchronized (listeners) { | ||
listeners.stream() | ||
.forEach(listener -> listener.eventStreamClosed(code, phrase)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a nitpick: there is different indentation used here and above, maybe those expressions would even fit on one line? By the way: it seems that it is possible to omit the stream()
and call forEach()
directly on Java 8 Iterable
s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Polish added
1677555
to
53a0bae
Compare
I added one extra commit to handle the case in which event listeners are removed from some method called by I am merging as soon as Travis is happy. |
This is a set of 5 refactorings to detach
EventStream
from its WebSocket implementation and allow alternative implementations.This is needed in order to replace reading from WebSockets completely in Uyuni where we plan to read them from a Postgres database filled by a custom Salt Engine.
Best reviewed commit-by-commit.