[
https://issues.apache.org/jira/browse/METRON-1790?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16647775#comment-16647775
]
ASF GitHub Bot commented on METRON-1790:
----------------------------------------
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.
> Unsubscribe from every observable in the pcap panel UI component
> ----------------------------------------------------------------
>
> Key: METRON-1790
> URL: https://issues.apache.org/jira/browse/METRON-1790
> Project: Metron
> Issue Type: Improvement
> Reporter: Tamas Fodor
> Assignee: Tamas Fodor
> Priority: Minor
>
> There are a lot of http requests performed in the pcap panel ui component and
> we just unsubscribe from some of them when the component is no longer
> rendered on the screen. It could cause memory consumption issues. Because of
> the active subscriptions, the garbage collector is not able to remove these
> objects from the memory, however they're not needed to be there anymore.
> There's another benefit of unsubscribing from these http calls. If the user
> leaves the pcap tab but there are pending requests, the unsubscribe method
> cancels the active xhrs immediately so it won't wait for fulfilment
> unnecessarily.
> [https://github.com/apache/metron/blob/master/metron-interface/metron-alerts/src/app/pcap/pcap-panel/pcap-panel.component.ts#L54]
> I would also refactor that part when we convert an observable to a promise. I
> would keep it as an observable. By doing this, we would be able to
> unsubscribe from it as well in the destructor method. Promises are not
> cancelable.
> [https://github.com/apache/metron/blob/master/metron-interface/metron-alerts/src/app/pcap/pcap-panel/pcap-panel.component.ts#L70]
> Resources:
> [https://angular.io/guide/lifecycle-hooks#ondestroy]
> This is the place to free resources that won't be garbage collected
> automatically. Unsubscribe from Observables and DOM events. Stop interval
> timers.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
