Github user tiborm commented on the issue:
https://github.com/apache/metron/pull/1208
@nickwallen The concept here is observable is a stream of events. The basic
rule is to unsubscribe when you no longer want to know about new values from a
particular stream. (RxJs/Angular handling observables like streams, even if we
know that there will be just one value. It's highly recommended to ignore that.)
An error is just a possible new value from a stream. It could be followed
by an actual value and then an error again.
Actually, you are right with your concerns. The original implementation
here gives no chance to handle events in a reactive way. In my opinion, our
real streams here are the following:
- the stream of clicks on query button (leads to a request)
- the stream of clicks on the cancel button (leads to a cancel request)
- the stream of PCAP values (leads to rendering table)
- a stream of status changes (leads to updating several UI controls)
- a stream of percentages (leads to updating progress bar)
And we only want to unsubscribe from these streams when the user navigates
away to the alert tab.
If an error occurs, the job fails, user click cancel we like to continue
observing them.
Unfortunately, the implementation creates new observables on every click
instead of observing the stream of clicks. This leads to the fact that we
unsubscribing and resubscribing all the time. Also makes this code hard to
understand and explain any change or small improvement.
However, IMHO unsubscribe implemented in this PR is an improvement, still
not the perfect state of this code part.
I suggest @ruffle1986 to create a followup ticket which is about
"Implementing proper reactive event handling in PCAP".
And another about "Refactor out PCAP querying logic from PCAP panel and
move it to the service". That would also make things way clearer here, hence
right now the panel contains all the query logic.
---
