Skip to content

Comments

TINKERPOP-2238 Fixed all iterator leaks#1144

Merged
spmallette merged 8 commits intotp33from
TINKERPOP-2238
Jun 25, 2019
Merged

TINKERPOP-2238 Fixed all iterator leaks#1144
spmallette merged 8 commits intotp33from
TINKERPOP-2238

Conversation

@spmallette
Copy link
Contributor

https://issues.apache.org/jira/browse/TINKERPOP-2238

Fixed all the iterator leaks. Remove the temporary "ignore leak" annotation.

All tests pass with docker/build.sh -t -n -i

VOTE +1

@divijvaidya
Copy link
Member

+1 - I am aware of the logic and the code change look good.

Thanks for completing this!

@spmallette spmallette merged commit cba5081 into tp33 Jun 25, 2019
@spmallette spmallette deleted the TINKERPOP-2238 branch June 25, 2019 19:34
@spmallette
Copy link
Contributor Author

@divijvaidya i was reviewing a bit in preparation for release next week and I think i have some questions related to this change:

  1. Was there a reason why TinkerGraphIterator didn't implement our CloseableIterator?
  2. Going a step further, was there a reason not to somehow fold the functionality of the TinkerGraphIterator into CloseableIterator infrastructure?

It seems like, at a minimum, we should do item 1 which is pretty painless. For item 2, I guess DefaultCloseableIterator could take much of the functionality of the TinkerGraphIterator and then we'd use CloseableIterator.asCloseable() in these sorts of places:

https://github.com/apache/tinkerpop/pull/1118/files#diff-e146457e1c98d0e94695d31b0e5ed6f4

thoughts?

@divijvaidya
Copy link
Member

We can do 1 but I don't see a lot of benefit of doing this. Having said that, I don't have any strong opinion on this. Will change.

But for 2, I would keep prefer that CloseableIterator and DefaultCloseableIterator act as extensions to vanilla iterator operators (as their name suggests) without any domain specific business logic.

TinkerGraphIterator contains storage specific logic, i.e. knowledge of when to increment/decrement StoreIteratorCounter (different storage might release the iterator at different events) and hence, I am not in favor of adding its functionality to DefaultCloseableIterator.

@spmallette
Copy link
Contributor Author

Having said that, I don't have any strong opinion on this. Will change.

ok...i will look for a PR. i think that we should consistently try to use that CloseableIterator interface. it was created explicitly for this purpose (requested by OrientDB originally i believe).

I'm going to sleep on your reasoning for 2, but what you're saying there sounds right to me.

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

Successfully merging this pull request may close these issues.

2 participants