OGM-767 Add support for @BatchSize in OGM #564
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Will you send another PR for actually implementing |
| else { | ||
| for ( EntityKey entityKey : keys ) { | ||
| // TODO use the multiload API once introduced | ||
| Tuple entry = gridDialect.getTuple( entityKey, persister.getTupleContext() ); |
There was a problem hiding this comment.
Should be doable now, right?
|
@emmanuelbernard: Done with the review. My main concern are the remaining to-dos:
What is the "legacy" stuff about? Didn't fully grok that. I'd vote for doing as less as possible on the OGM side and push back the required changes to ORM. |
| @@ -59,6 +61,14 @@ public Tuple getTuple(EntityKey key, TupleContext tupleContext) { | |||
| } | |||
|
|
|||
| @Override | |||
| public List<Tuple> getTuples(EntityKey[] keys, TupleContext tupleContext) { | |||
| if ( log.isTraceEnabled() ) { | |||
| log.tracef( "Reading tuple with keys %1$s and context %2$s", Arrays.toString( keys ), tupleContext ); | |||
I was planning on other people to give a hand :) |
Disabled all but one strategy for the MVP Introduce ability to customize how the inner loader is built as OGM and ORM have different loaders. Introduce notion of BatchableEntityLoader to abstract OgmLoader, the legacy EntityLoader and plan.EntityLoader. Methods of this interface is to be moved to UniqueEntityLoader when moving things back to ORM.
Add support for accepting several ids in OgmLoader (for entities). Make OgmEntityPersister use the batch fetching framework to build batching loaders. Adapt OgmLoader creators to instead get their instances from OgmEntityPersister
No dialect is currently making use of it
…getGridDialect This avoid skewing the ORM fetch statistics
It was only containing a single entry per call to the loader The compiler might be able to optimize better now that one call happens
…od is used Make sure getTuples is effectively used when expected basically
MultigetGridDialect don't provide the right params to build prepared statements ahead of time. Also no dialect have this specific need.
|
@gunnarmorling I've fixed a lot of the remarks and todos form OGM-767. You can review the major changes starting at 91adb76 |
emmanuelbernard
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Only for entities for now. Will come in a different issue and PR for later:
The "legacy" and default batching strategy is not implemented yet as it requires yet another big pile of code duplication and is almost like the padded one. I'm tempted to not implement it and wait for the batch fetching framework improvements to be moved back to ORM to benefit them.
Also no dialect currently make use of
MultigetGridDialectbut that is hopefully easier to do that the core enablement of this PR.95% because of the legacy batching strategy. Hence feedback season is opened.