-
Notifications
You must be signed in to change notification settings - Fork 161
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
Gh 4581 remove jerven design mistakes #4582
Gh 4581 remove jerven design mistakes #4582
Conversation
b12ed0f
to
4b30d24
Compare
b241e9b
to
f8f7eee
Compare
@hmottestad could this still be included in the next 5.x Milestone build? |
Yeah. I haven't started on the build process yet. I saw you requested a review from me, so I'll take a look asap. |
try { | ||
cf.close(); | ||
} catch (QueryEvaluationException e) { | ||
super.handleClose(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this maybe be try-finally or is super.handleClose() only supposed to be called when there is an exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a try-finally, although right now super.handleClose is a noop. That could change in the future.
will-fix.
f8f7eee
to
95acc2a
Compare
From what I understand looking through the code you've moved the set creation to a higher level in the code (EvaluationFactory?) so that it's easier to override in the specific stores. Only issue I found in your code was the try-catch instead of try-finally. If you're happy with everything then you can go ahead and merge. |
As well as supplier methods later better done with the collection factory API.
This allows to always be sure that we can fall back to disk if required. Also allows optimized datastructures to be injected.
95acc2a
to
07e4174
Compare
Yes. Exactly, now with collection factories one does not need to extend the store. One can just inject the limited size collections. Also now there are fewer and fewer places left where collections are not coming from a collection factory that can spill to disk. So the original goal I had with these LimitedSize* all those years ago is no longer valid. |
GitHub issue resolved: #
Briefly describe the changes proposed in this PR:
Remove LimitedSize evaluation strategy code, as well as a few methods in the EvaluationStrategy API that was replaced by the collection factory API.
Closes #4581