Skip to content
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

Indices not closing CloseableIterators #74

Closed
yrashk opened this issue Jun 28, 2016 · 5 comments
Closed

Indices not closing CloseableIterators #74

yrashk opened this issue Jun 28, 2016 · 5 comments

Comments

@yrashk
Copy link

yrashk commented Jun 28, 2016

See the discussion at 0e343eb#commitcomment-18041324

It looks like the workaround of self-closing CloseableIterators doesn't always work so it is best to ensure we're closing those. As a first step, we can do a quick fix that checks if the iterator is closeable in all applicable situations and closes it if necessary.

In the longer term, we can improve the internal API.

Thoughts?

@npgall
Copy link
Owner

npgall commented Jul 4, 2016

Just a heads up that I have started work on a better internal API for this.

Basically, I'm replacing the Collection, with a new abstraction called ObjectSet. ObjectSet is like an Iterable, and it can wrap an ObjectStore or a Collection. But it keeps track of the iterators which have been opened and it implements Closeable where close() will close the open iterators.

It will be a fairly simple fix. However I'm working on another feature for my employer at the moment, so I hope to complete this issue in the next week or so.

@yrashk
Copy link
Author

yrashk commented Jul 5, 2016

How hard will it be to migrate other index implementations to this ObjectSet thing, relatively straightforward?

@npgall
Copy link
Owner

npgall commented Jul 5, 2016

Yes it will be very straightforward - fairly trivial. It will probably take 30 seconds of coding to migrate the index.

@npgall npgall closed this as completed in cf34091 Jul 6, 2016
@npgall
Copy link
Owner

npgall commented Jul 6, 2016

I've committed the change to introduce ObjectSet.

I'm still not sure that there was a resource leak, because closeRequestScopeResourcesIfNecessary() was always called anyway. But anyway, this change does make it easier for indexes to close iterators and release connections sooner than before. Let me know if you have any issues!

@yrashk
Copy link
Author

yrashk commented Jul 11, 2016

Thank you so much! This change looks good to me — I will upgrade my indices once new cqengine is released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants