You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by GitBox <gi...@apache.org> on 2021/01/22 13:12:01 UTC
[GitHub] [sling-org-apache-sling-graphql-core] raducotescu opened a new pull request #17: SLING-10085 - Cache the GraphQL schemas in the DefaultQueryExecutor
raducotescu opened a new pull request #17:
URL: https://github.com/apache/sling-org-apache-sling-graphql-core/pull/17
* implemented a LRU cache for the GraphQL schemas
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-graphql-core] bdelacretaz commented on a change in pull request #17: SLING-10085 - Cache the GraphQL schemas in the DefaultQueryExecutor
Posted by GitBox <gi...@apache.org>.
bdelacretaz commented on a change in pull request #17:
URL: https://github.com/apache/sling-org-apache-sling-graphql-core/pull/17#discussion_r562640156
##########
File path: src/main/java/org/apache/sling/graphql/core/engine/DefaultQueryExecutor.java
##########
@@ -289,4 +332,71 @@ private String getDirectiveArgumentValue(Directive d, String name) {
throw up;
}
}
+
+ GraphQLSchema getSchema(@NotNull String sdl, @NotNull Resource currentResource, @NotNull String[] selectors) {
+ readLock.lock();
+ String newHash = SHA256Hasher.getHash(sdl);
+ String resourceToHashMapKey = getCacheKey(currentResource, selectors);
+ String oldHash = resourceToHashMap.get(resourceToHashMapKey);
+ if (!newHash.equals(oldHash)) {
+ readLock.unlock();
+ writeLock.lock();
+ try {
+ oldHash = resourceToHashMap.get(resourceToHashMapKey);
+ if (!newHash.equals(oldHash)) {
+ resourceToHashMap.put(resourceToHashMapKey, newHash);
+ TypeDefinitionRegistry typeRegistry = new SchemaParser().parse(sdl);
+ Iterable<GraphQLScalarType> scalars = scalarsProvider.getCustomScalars(typeRegistry.scalars());
+ RuntimeWiring runtimeWiring = buildWiring(typeRegistry, scalars, currentResource);
+ SchemaGenerator schemaGenerator = new SchemaGenerator();
Review comment:
how about calling these costly things before acquiring the write lock? To mininize the locked section
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-graphql-core] sonarcloud[bot] commented on pull request #17: SLING-10085 - Cache the GraphQL schemas in the DefaultQueryExecutor
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #17:
URL: https://github.com/apache/sling-org-apache-sling-graphql-core/pull/17#issuecomment-765390060
SonarCloud Quality Gate failed.
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&resolved=false&types=VULNERABILITY)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&resolved=false&types=SECURITY_HOTSPOT) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&resolved=false&types=SECURITY_HOTSPOT)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/60.png' alt='69.2%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&metric=new_coverage&view=list) [69.2% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&metric=new_duplicated_lines_density&view=list)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-graphql-core] raducotescu merged pull request #17: SLING-10085 - Cache the GraphQL schemas in the DefaultQueryExecutor
Posted by GitBox <gi...@apache.org>.
raducotescu merged pull request #17:
URL: https://github.com/apache/sling-org-apache-sling-graphql-core/pull/17
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-graphql-core] sonarcloud[bot] commented on pull request #17: SLING-10085 - Cache the GraphQL schemas in the DefaultQueryExecutor
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #17:
URL: https://github.com/apache/sling-org-apache-sling-graphql-core/pull/17#issuecomment-765575384
SonarCloud Quality Gate failed.
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&resolved=false&types=VULNERABILITY)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&resolved=false&types=SECURITY_HOTSPOT) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&resolved=false&types=SECURITY_HOTSPOT)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/60.png' alt='70.3%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&metric=new_coverage&view=list) [70.3% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&metric=new_duplicated_lines_density&view=list)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-graphql-core] sonarcloud[bot] removed a comment on pull request #17: SLING-10085 - Cache the GraphQL schemas in the DefaultQueryExecutor
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] removed a comment on pull request #17:
URL: https://github.com/apache/sling-org-apache-sling-graphql-core/pull/17#issuecomment-765390060
SonarCloud Quality Gate failed.
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&resolved=false&types=VULNERABILITY)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&resolved=false&types=SECURITY_HOTSPOT) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&resolved=false&types=SECURITY_HOTSPOT)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/60.png' alt='69.2%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&metric=new_coverage&view=list) [69.2% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-graphql-core&pullRequest=17&metric=new_duplicated_lines_density&view=list)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-graphql-core] bdelacretaz commented on a change in pull request #17: SLING-10085 - Cache the GraphQL schemas in the DefaultQueryExecutor
Posted by GitBox <gi...@apache.org>.
bdelacretaz commented on a change in pull request #17:
URL: https://github.com/apache/sling-org-apache-sling-graphql-core/pull/17#discussion_r562644983
##########
File path: src/main/java/org/apache/sling/graphql/core/engine/DefaultQueryExecutor.java
##########
@@ -289,4 +332,71 @@ private String getDirectiveArgumentValue(Directive d, String name) {
throw up;
}
}
+
+ GraphQLSchema getSchema(@NotNull String sdl, @NotNull Resource currentResource, @NotNull String[] selectors) {
+ readLock.lock();
+ String newHash = SHA256Hasher.getHash(sdl);
+ String resourceToHashMapKey = getCacheKey(currentResource, selectors);
+ String oldHash = resourceToHashMap.get(resourceToHashMapKey);
Review comment:
might be good to add a comment about why you're using a composite cache key, that was initially unclear to me. IIUC that's because buildWiring uses the Resource and the services that it uses can changes outside of our control.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-graphql-core] bdelacretaz commented on a change in pull request #17: SLING-10085 - Cache the GraphQL schemas in the DefaultQueryExecutor
Posted by GitBox <gi...@apache.org>.
bdelacretaz commented on a change in pull request #17:
URL: https://github.com/apache/sling-org-apache-sling-graphql-core/pull/17#discussion_r562630989
##########
File path: src/main/java/org/apache/sling/graphql/core/engine/DefaultQueryExecutor.java
##########
@@ -289,4 +332,71 @@ private String getDirectiveArgumentValue(Directive d, String name) {
throw up;
}
}
+
+ GraphQLSchema getSchema(@NotNull String sdl, @NotNull Resource currentResource, @NotNull String[] selectors) {
+ readLock.lock();
Review comment:
I think this can move to two lines later, just before resourceToHashMap.get ? No need to lock while computing the hash etc
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-graphql-core] raducotescu commented on a change in pull request #17: SLING-10085 - Cache the GraphQL schemas in the DefaultQueryExecutor
Posted by GitBox <gi...@apache.org>.
raducotescu commented on a change in pull request #17:
URL: https://github.com/apache/sling-org-apache-sling-graphql-core/pull/17#discussion_r562703132
##########
File path: src/main/java/org/apache/sling/graphql/core/engine/DefaultQueryExecutor.java
##########
@@ -289,4 +332,71 @@ private String getDirectiveArgumentValue(Directive d, String name) {
throw up;
}
}
+
+ GraphQLSchema getSchema(@NotNull String sdl, @NotNull Resource currentResource, @NotNull String[] selectors) {
+ readLock.lock();
+ String newHash = SHA256Hasher.getHash(sdl);
+ String resourceToHashMapKey = getCacheKey(currentResource, selectors);
+ String oldHash = resourceToHashMap.get(resourceToHashMapKey);
+ if (!newHash.equals(oldHash)) {
+ readLock.unlock();
+ writeLock.lock();
+ try {
+ oldHash = resourceToHashMap.get(resourceToHashMapKey);
+ if (!newHash.equals(oldHash)) {
+ resourceToHashMap.put(resourceToHashMapKey, newHash);
+ TypeDefinitionRegistry typeRegistry = new SchemaParser().parse(sdl);
+ Iterable<GraphQLScalarType> scalars = scalarsProvider.getCustomScalars(typeRegistry.scalars());
+ RuntimeWiring runtimeWiring = buildWiring(typeRegistry, scalars, currentResource);
+ SchemaGenerator schemaGenerator = new SchemaGenerator();
+ hashToSchemaMap.put(newHash, schemaGenerator.makeExecutableSchema(typeRegistry, runtimeWiring));
+ }
+ readLock.lock();
+ } finally {
+ writeLock.unlock();
+ }
+ }
+ try {
+ return hashToSchemaMap.get(newHash);
+ } finally {
Review comment:
There's no clean way to unlock both of them in a single `finally`, or I haven't found the way to correctly write this. You need to make sure that no matter the failures, both locks are unlocked, but it has to happen in the correct sequence and the locks have to be unlocked by the same thread that locked them.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-graphql-core] bdelacretaz commented on a change in pull request #17: SLING-10085 - Cache the GraphQL schemas in the DefaultQueryExecutor
Posted by GitBox <gi...@apache.org>.
bdelacretaz commented on a change in pull request #17:
URL: https://github.com/apache/sling-org-apache-sling-graphql-core/pull/17#discussion_r562629296
##########
File path: src/main/java/org/apache/sling/graphql/core/engine/DefaultQueryExecutor.java
##########
@@ -96,13 +114,40 @@
@Reference
private SlingScalarsProvider scalarsProvider;
+ @ObjectClassDefinition(
+ name = "Apache Sling Default GraphQL Query Executor"
+ )
+ @interface Config {
+ @AttributeDefinition(
+ name = "Schema Cache Size",
+ description = "The number of schemas to cache. Since a schema normally doesn't change often, they can be cached and " +
Review comment:
I would say "the compiled schemas can be cached"
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-graphql-core] bdelacretaz commented on a change in pull request #17: SLING-10085 - Cache the GraphQL schemas in the DefaultQueryExecutor
Posted by GitBox <gi...@apache.org>.
bdelacretaz commented on a change in pull request #17:
URL: https://github.com/apache/sling-org-apache-sling-graphql-core/pull/17#discussion_r562639343
##########
File path: src/main/java/org/apache/sling/graphql/core/engine/DefaultQueryExecutor.java
##########
@@ -289,4 +332,71 @@ private String getDirectiveArgumentValue(Directive d, String name) {
throw up;
}
}
+
+ GraphQLSchema getSchema(@NotNull String sdl, @NotNull Resource currentResource, @NotNull String[] selectors) {
+ readLock.lock();
+ String newHash = SHA256Hasher.getHash(sdl);
+ String resourceToHashMapKey = getCacheKey(currentResource, selectors);
+ String oldHash = resourceToHashMap.get(resourceToHashMapKey);
+ if (!newHash.equals(oldHash)) {
+ readLock.unlock();
+ writeLock.lock();
+ try {
+ oldHash = resourceToHashMap.get(resourceToHashMapKey);
+ if (!newHash.equals(oldHash)) {
+ resourceToHashMap.put(resourceToHashMapKey, newHash);
+ TypeDefinitionRegistry typeRegistry = new SchemaParser().parse(sdl);
+ Iterable<GraphQLScalarType> scalars = scalarsProvider.getCustomScalars(typeRegistry.scalars());
+ RuntimeWiring runtimeWiring = buildWiring(typeRegistry, scalars, currentResource);
+ SchemaGenerator schemaGenerator = new SchemaGenerator();
+ hashToSchemaMap.put(newHash, schemaGenerator.makeExecutableSchema(typeRegistry, runtimeWiring));
+ }
+ readLock.lock();
+ } finally {
+ writeLock.unlock();
+ }
+ }
+ try {
+ return hashToSchemaMap.get(newHash);
+ } finally {
Review comment:
might be clearer to use a single finally clause for the whole method, that unlocks both locks - to better express that whatever happens we want both of them to be unlocked
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-graphql-core] raducotescu commented on a change in pull request #17: SLING-10085 - Cache the GraphQL schemas in the DefaultQueryExecutor
Posted by GitBox <gi...@apache.org>.
raducotescu commented on a change in pull request #17:
URL: https://github.com/apache/sling-org-apache-sling-graphql-core/pull/17#discussion_r562705372
##########
File path: src/main/java/org/apache/sling/graphql/core/engine/DefaultQueryExecutor.java
##########
@@ -289,4 +332,71 @@ private String getDirectiveArgumentValue(Directive d, String name) {
throw up;
}
}
+
+ GraphQLSchema getSchema(@NotNull String sdl, @NotNull Resource currentResource, @NotNull String[] selectors) {
+ readLock.lock();
+ String newHash = SHA256Hasher.getHash(sdl);
+ String resourceToHashMapKey = getCacheKey(currentResource, selectors);
+ String oldHash = resourceToHashMap.get(resourceToHashMapKey);
+ if (!newHash.equals(oldHash)) {
+ readLock.unlock();
+ writeLock.lock();
+ try {
+ oldHash = resourceToHashMap.get(resourceToHashMapKey);
+ if (!newHash.equals(oldHash)) {
+ resourceToHashMap.put(resourceToHashMapKey, newHash);
+ TypeDefinitionRegistry typeRegistry = new SchemaParser().parse(sdl);
+ Iterable<GraphQLScalarType> scalars = scalarsProvider.getCustomScalars(typeRegistry.scalars());
+ RuntimeWiring runtimeWiring = buildWiring(typeRegistry, scalars, currentResource);
+ SchemaGenerator schemaGenerator = new SchemaGenerator();
Review comment:
With the code written like it is, we're only doing those costly operations _iff_ the current thread has to update the value. Moving this code outside of the lock will lead to it being called concurrently, defeating the purpose of the whole cache. The expensive operation is done by `new SchemaParser().parse(sdl);`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org