-
Notifications
You must be signed in to change notification settings - Fork 12
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
SLING-10502 - lazy loading helpers #25
Conversation
src/main/java/org/apache/sling/graphql/helpers/layzloading/LazyLoadingField.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/sling/graphql/helpers/layzloading/LazyLoadingMap.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/sling/graphql/helpers/layzloading/LazyLoadingMap.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/sling/graphql/helpers/layzloading/LazyLoadingMap.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/sling/graphql/helpers/layzloading/LazyLoadingMap.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/sling/graphql/helpers/layzloading/LazyLoadingMap.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/sling/graphql/helpers/layzloading/LazyLoadingMap.java
Outdated
Show resolved
Hide resolved
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.
I agree with all the observations that @jsedding made. There's no need to synchronize the LazyLoadingMap
if it extends HashMap
. Leave the synchronization to the caller - they can always wrap it in a Collections#synchronizedMap
, or extend ConcurrentHashMap
.
… improve LazyLoadingField
Thank you for the detailed review, all good suggestions! Of course, HashMap not being synchronized it doesn't make sense to make the I think I have implemented all other suggestions, please cross-check. |
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.
LGTM!
@Override | ||
public T remove(Object key) { | ||
super.remove(key); | ||
suppliers.remove(key); |
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.
Would it be too expensive to obey the Map
contract in this case?
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.
It's hard to say - I will make that optional, adding an Options
object to the class constructor.
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.
Committed that, using class methods to set options instead.
*/ | ||
private void computeAll() { | ||
if(!suppliers.isEmpty()) { | ||
log.info("computeAll called, all remaining lazy values will be evaluated now"); |
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.
I think this should be logged on debug
- I assume that with GraphQL in Sling catching on, we'll have a lot of LazyLoadingMaps
.
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.
+1 I'll do that
Kudos, SonarCloud Quality Gate passed! |
} | ||
super.remove(key); | ||
suppliers.remove(key); | ||
return null; |
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.
@bdelacretaz I think you meant to return super.remove(key)
here if computeValueOnRemove
is true
. How about this?
Supplier<T> supplier = suppliers.remove(key);
if (supplier != null) {
super.remove(key); // this should be unnecessary, otherwise I believe it would be a bug?!
return computeValueOnRemove ? supplier.get() : null;
} else {
return super.remove(key);
}
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.
Indeed, both the tests and the code were wrong!
Good idea to use the Supplier in this way, I have committed a slightly different fix that sets the Stats and that always returns null unless computeValueOnRemove
is set - I think that's less confusing. 4cce079
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.
👍 looks good to me
This is an initial implementation with a
LazyLoadingField
andLazyLoadingMap
that should help build GraphQL result sets with lazy loading values.