[GitHub] metron issue #1208: METRON-1790: Unsubscribe from every observable in the pc...

[tiborm <gi...@git.apache.org>]

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.



---