Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[SNAP-3270] removing streaming query listener in finalize method #195

Merged
merged 6 commits into from
Dec 13, 2019

Conversation

vatsalmevada
Copy link

@vatsalmevada vatsalmevada commented Dec 12, 2019

What changes were proposed in this pull request?

For streaming UI, SnappyStreamingQueryListener is registered on listener bus
at the time of creating SnappySession. However, this listener is never removed
from the listener bus. Hence even if the SnappySession is collected by GC,
SnappyStreamingQueryListener is left orphan on the listener bus which is never
eligible for GC collection.

To fix this we are removing the listener from the listener bus when the session
the instance is getting collected by GC (i.e. in finalize method) which will make
the listener instance eligible for GC during the next GC cycle.

It should be OK if the listener instance gets collected in the next GC cycle as the
the memory footprint of the listener object is not big.

Another possible place to remove the listener in close method of the session,
however close method of session is not required to be closed explicitly.

How was this patch tested?

Reproduced the issue by running the following code as part of a snappy job:

while(true){
      session.newSession()
}

Collected histogram of leader process using jmap and observed that instances of
SnappyStreamingQueryListener is increasing indefinitely and never garbage
collected whereas SnappySession instances are garbage collected:

jmap -histo:live <leader pid>|grep "SnappySession\|SnappyStreamingQueryListener"

Followed the same steps after applying the changes and noticed that
SnappyStreamingQueryListener instances are garbage collected.


  • precheckin -Pspark
  • SnappyStreamingQueryListenerSuite is passing now. Added a call to finalize
    method in StreamingQueryListenerSuite to get this change tested.

Vatsal Mevada added 2 commits December 12, 2019 08:37
query manager when session instance is getting collected for GC.
finalize method block if it is already not initialized.
@vatsalmevada vatsalmevada changed the title [SNAP-3270] [SNAP-3270] removing streaming query listener in finalize block Dec 12, 2019
Copy link

@dshirish dshirish left a comment

Choose a reason for hiding this comment

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

LGTM

Vatsal Mevada added 2 commits December 13, 2019 11:14
@vatsalmevada vatsalmevada merged commit c09aa1d into snappy/branch-2.1 Dec 13, 2019
@vatsalmevada vatsalmevada deleted the SNAP-3270 branch December 13, 2019 08:12
@vatsalmevada vatsalmevada changed the title [SNAP-3270] removing streaming query listener in finalize block [SNAP-3270] removing streaming query listener in finalize method Dec 13, 2019
sumwale pushed a commit to sumwale/spark that referenced this pull request Nov 5, 2020
…OSoftware#195)

For streaming UI, `SnappyStreamingQueryListener` is registered on listener bus
at the time of creating `SnappySession`. However, this listener is never removed
from the listener bus. Hence even if the `SnappySession` is collected by GC,
`SnappyStreamingQueryListener` is left orphan on the listener bus which is never
eligible for GC collection.

To fix this we are removing the listener from the listener bus when the session
the instance is getting collected by GC (i.e. in finalize method) which will make
the listener instance eligible for GC during the next GC cycle.

It should be OK if the listener instance gets collected in the next GC cycle as the
the memory footprint of the listener object is not big.

Another possible place to remove the listener in `close` method of the session,
however close method of session is not required to be closed explicitly.

Reproduced the issue by running the following code as part of a snappy job:

```
while(true){
      session.newSession()
}
```

Collected histogram of leader process using `jmap` and observed that instances of
`SnappyStreamingQueryListener` is increasing indefinitely and never garbage
collected whereas `SnappySession` instances are garbage collected:

`jmap -histo:live <leader pid>|grep "SnappySession\|SnappyStreamingQueryListener"`

Followed the same steps after applying the changes and noticed that
 `SnappyStreamingQueryListener` instances are garbage collected.

---

- `SnappyStreamingQueryListenerSuite` is passing now. Added a call to `finalize`
method in `StreamingQueryListenerSuite` to get this change tested.
sumwale pushed a commit that referenced this pull request Jul 11, 2021
For streaming UI, `SnappyStreamingQueryListener` is registered on listener bus
at the time of creating `SnappySession`. However, this listener is never removed
from the listener bus. Hence even if the `SnappySession` is collected by GC,
`SnappyStreamingQueryListener` is left orphan on the listener bus which is never
eligible for GC collection.

To fix this we are removing the listener from the listener bus when the session
the instance is getting collected by GC (i.e. in finalize method) which will make
the listener instance eligible for GC during the next GC cycle.

It should be OK if the listener instance gets collected in the next GC cycle as the
the memory footprint of the listener object is not big.

Another possible place to remove the listener in `close` method of the session,
however close method of session is not required to be closed explicitly.

Reproduced the issue by running the following code as part of a snappy job:

```
while(true){
      session.newSession()
}
```

Collected histogram of leader process using `jmap` and observed that instances of
`SnappyStreamingQueryListener` is increasing indefinitely and never garbage
collected whereas `SnappySession` instances are garbage collected:

`jmap -histo:live <leader pid>|grep "SnappySession\|SnappyStreamingQueryListener"`

Followed the same steps after applying the changes and noticed that
 `SnappyStreamingQueryListener` instances are garbage collected.

---

- `SnappyStreamingQueryListenerSuite` is passing now. Added a call to `finalize`
method in `StreamingQueryListenerSuite` to get this change tested.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants