From 6c3388da68fc9f33275548f29dba9f5a174bedf3 Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Thu, 31 Aug 2017 09:06:47 +1000 Subject: [PATCH 1/3] A registry and a dispatcher for graphql --- build.gradle | 1 + .../org/dataloader/DataLoaderRegistry.java | 54 ++++++++++++++++++ .../DataLoaderDispatcherInstrumentation.java | 28 ++++++++++ .../dataloader/DataLoaderRegistryTest.java | 41 ++++++++++++++ ...taLoaderDispatcherInstrumentationTest.java | 56 +++++++++++++++++++ 5 files changed, 180 insertions(+) create mode 100644 src/main/java/org/dataloader/DataLoaderRegistry.java create mode 100644 src/main/java/org/dataloader/graphql/DataLoaderDispatcherInstrumentation.java create mode 100644 src/test/java/org/dataloader/DataLoaderRegistryTest.java create mode 100644 src/test/java/org/dataloader/graphql/DataLoaderDispatcherInstrumentationTest.java diff --git a/build.gradle b/build.gradle index 9bcf09b..b2294db 100644 --- a/build.gradle +++ b/build.gradle @@ -49,6 +49,7 @@ compileJava { } dependencies { + compile "com.graphql-java:graphql-java:4.0" testCompile "junit:junit:$junitVersion" testCompile 'org.awaitility:awaitility:2.0.0' } diff --git a/src/main/java/org/dataloader/DataLoaderRegistry.java b/src/main/java/org/dataloader/DataLoaderRegistry.java new file mode 100644 index 0000000..5cf7349 --- /dev/null +++ b/src/main/java/org/dataloader/DataLoaderRegistry.java @@ -0,0 +1,54 @@ +package org.dataloader; + +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; + +/** + * This allows data loaders to be registered together into a single place so + * they can be dispatched as one. + */ +public class DataLoaderRegistry { + private final List> dataLoaders = new CopyOnWriteArrayList<>(); + + /** + * @return the currently registered data loaders + */ + public List> getDataLoaders() { + return new ArrayList<>(dataLoaders); + } + + /** + * This will register a new dataloader + * + * @param dataLoader the data loader to register + * + * @return this registry + */ + public DataLoaderRegistry register(DataLoader dataLoader) { + if (!dataLoaders.contains(dataLoader)) { + dataLoaders.add(dataLoader); + } + return this; + } + + /** + * This will unregister a new dataloader + * + * @param dataLoader the data loader to unregister + * + * @return this registry + */ + public DataLoaderRegistry unregister(DataLoader dataLoader) { + dataLoaders.remove(dataLoader); + return this; + } + + /** + * This will called {@link org.dataloader.DataLoader#dispatch()} on each of the registered + * {@link org.dataloader.DataLoader}s + */ + public void dispatchAll() { + dataLoaders.forEach(DataLoader::dispatch); + } +} diff --git a/src/main/java/org/dataloader/graphql/DataLoaderDispatcherInstrumentation.java b/src/main/java/org/dataloader/graphql/DataLoaderDispatcherInstrumentation.java new file mode 100644 index 0000000..f9fbbe9 --- /dev/null +++ b/src/main/java/org/dataloader/graphql/DataLoaderDispatcherInstrumentation.java @@ -0,0 +1,28 @@ +package org.dataloader.graphql; + +import graphql.ExecutionResult; +import graphql.execution.instrumentation.InstrumentationContext; +import graphql.execution.instrumentation.NoOpInstrumentation; +import graphql.execution.instrumentation.parameters.InstrumentationExecutionStrategyParameters; +import org.dataloader.DataLoaderRegistry; + +import java.util.concurrent.CompletableFuture; + +/** + * This graphql {@link graphql.execution.instrumentation.Instrumentation} will dispatch + * all the contained {@link org.dataloader.DataLoader}s when each level of the graphql + * query is executed. + */ +public class DataLoaderDispatcherInstrumentation extends NoOpInstrumentation { + + private final DataLoaderRegistry dataLoaderRegistry; + + public DataLoaderDispatcherInstrumentation(DataLoaderRegistry dataLoaderRegistry) { + this.dataLoaderRegistry = dataLoaderRegistry; + } + + @Override + public InstrumentationContext> beginExecutionStrategy(InstrumentationExecutionStrategyParameters parameters) { + return (result, t) -> dataLoaderRegistry.dispatchAll(); + } +} \ No newline at end of file diff --git a/src/test/java/org/dataloader/DataLoaderRegistryTest.java b/src/test/java/org/dataloader/DataLoaderRegistryTest.java new file mode 100644 index 0000000..897bdf3 --- /dev/null +++ b/src/test/java/org/dataloader/DataLoaderRegistryTest.java @@ -0,0 +1,41 @@ +package org.dataloader; + +import org.hamcrest.Matchers; +import org.junit.Test; + +import java.util.concurrent.CompletableFuture; + +import static java.util.Arrays.asList; +import static org.junit.Assert.assertThat; + +public class DataLoaderRegistryTest { + final BatchLoader identityBatchLoader = CompletableFuture::completedFuture; + + @Test + public void registration_works() throws Exception { + DataLoader dlA = new DataLoader<>(identityBatchLoader); + DataLoader dlB = new DataLoader<>(identityBatchLoader); + DataLoader dlC = new DataLoader<>(identityBatchLoader); + + DataLoaderRegistry registry = new DataLoaderRegistry(); + + registry.register(dlA).register(dlB).register(dlC); + + assertThat(registry.getDataLoaders(), Matchers.equalTo(asList(dlA, dlB, dlC))); + + // the same dl twice is one add + + + registry = new DataLoaderRegistry(); + + registry.register(dlA).register(dlB).register(dlC).register(dlA).register(dlB); + + assertThat(registry.getDataLoaders(), Matchers.equalTo(asList(dlA, dlB, dlC))); + + + // and unregister + registry.unregister(dlC); + + assertThat(registry.getDataLoaders(), Matchers.equalTo(asList(dlA, dlB))); + } +} \ No newline at end of file diff --git a/src/test/java/org/dataloader/graphql/DataLoaderDispatcherInstrumentationTest.java b/src/test/java/org/dataloader/graphql/DataLoaderDispatcherInstrumentationTest.java new file mode 100644 index 0000000..4ce80d6 --- /dev/null +++ b/src/test/java/org/dataloader/graphql/DataLoaderDispatcherInstrumentationTest.java @@ -0,0 +1,56 @@ +package org.dataloader.graphql; + +import graphql.ExecutionResult; +import graphql.execution.instrumentation.InstrumentationContext; +import org.dataloader.BatchLoader; +import org.dataloader.DataLoader; +import org.dataloader.DataLoaderRegistry; +import org.junit.Test; + +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.atomic.AtomicInteger; + +import static java.util.Arrays.asList; +import static java.util.Collections.singletonList; +import static org.hamcrest.Matchers.equalTo; +import static org.junit.Assert.assertThat; + +public class DataLoaderDispatcherInstrumentationTest { + + @Test + public void basic_invocation() throws Exception { + + AtomicInteger invocationCount = new AtomicInteger(); + final List loadedKeys = new ArrayList<>(); + final BatchLoader identityBatchLoader = keys -> { + invocationCount.incrementAndGet(); + loadedKeys.add(keys); + return CompletableFuture.completedFuture(keys); + }; + + DataLoader dlA = new DataLoader<>(identityBatchLoader); + DataLoader dlB = new DataLoader<>(identityBatchLoader); + DataLoader dlC = new DataLoader<>(identityBatchLoader); + DataLoaderRegistry registry = new DataLoaderRegistry() + .register(dlA) + .register(dlB) + .register(dlC); + + DataLoaderDispatcherInstrumentation dispatcher = new DataLoaderDispatcherInstrumentation(registry); + InstrumentationContext> context = dispatcher.beginExecutionStrategy(null); + + // cause some activity + dlA.load("A"); + dlB.load("B"); + dlC.load("C"); + + context.onEnd(null, null); + + assertThat(invocationCount.get(), equalTo(3)); + + // will be [[A],[B],[C]] + assertThat(loadedKeys, equalTo(asList(singletonList("A"), singletonList("B"), singletonList("C")))); + } +} \ No newline at end of file From d862eeb51acb1b8d386a097bf2b473591fdb7137 Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Thu, 31 Aug 2017 12:43:31 +1000 Subject: [PATCH 2/3] A registry and a dispatcher for graphql with tests and readme --- README.md | 127 +++++++++++++++++- .../DataLoaderDispatcherInstrumentation.java | 7 +- src/test/java/ReadmeExamples.java | 86 ++++++++++++ .../dataloader/DataLoaderRegistryTest.java | 8 +- ...taLoaderDispatcherInstrumentationTest.java | 51 +++++-- 5 files changed, 259 insertions(+), 20 deletions(-) diff --git a/README.md b/README.md index d857be5..8ac5729 100644 --- a/README.md +++ b/README.md @@ -165,13 +165,136 @@ a list of user ids in one call. ``` -That said, with key caching turn on (the default), it may still be more efficient using `dataloader` than without it. +That said, with key caching turn on (the default), it will still be more efficient using `dataloader` than without it. + +# Using dataloader in graphql for maximum efficiency + + +If you are using `graphql`, you are likely to making queries on a graph of data (surprise surprise). `dataloader` will help +you to make this a more efficient process by both caching and batching requests for that graph of data items. If `dataloader` +has previously see a data item before, it will cached the value and will return it without having to ask for it again. + +Imagine we have the StarWars query outlined below. It asks us to find a hero and their friend's names and their friend's friend's +names. It is likely that many of these people will be friends in common. + + + + { + hero { + name + friends { + name + friends { + name + } + } + } + } + +The result of this query is displayed below. You can see that Han, Leia, Luke and R2-D2 are tight knit bunch of friends and +share many friends in common. + + + [hero: [name: 'R2-D2', friends: [ + [name: 'Luke Skywalker', friends: [ + [name: 'Han Solo'], [name: 'Leia Organa'], [name: 'C-3PO'], [name: 'R2-D2']]], + [name: 'Han Solo', friends: [ + [name: 'Luke Skywalker'], [name: 'Leia Organa'], [name: 'R2-D2']]], + [name: 'Leia Organa', friends: [ + [name: 'Luke Skywalker'], [name: 'Han Solo'], [name: 'C-3PO'], [name: 'R2-D2']]]]] + ] + +A naive implementation would called a `DataFetcher` to retrieved a person object every time it was invoked. + +In this case it would be *15* calls over the network. Even though the group of people have a lot of common friends. +With `dataloader` you can make the `graphql` query much more efficient. + +As `graphql` descends each level of the query ( eg as it processes `hero` and then `friends` and then for each their `friends`), +the data loader is called to "promise" to deliver a person object. At each level `dataloader.dispatch()` will be +called to fire off the batch requests for that part of the query. With caching turned on (the default) then +any previously returned person will be returned as is for no cost. + +In the above example there are only *5* unique people mentioned but with caching and batching retrieval in place their will be only +*3* calls to the batch loader function. *3* calls over the network or to a database is much better than *15* calls you will agree. + +If you use capabilities like `java.util.concurrent.CompletableFuture.supplyAsync()` then you can make it even more efficient by making the +the remote calls asynchronous to the rest of the query. This will make it even more timely since multiple calls can happen at once +if need be. + +Here is how you might put this in place: + + + ```java + + // a batch loader function that will be called with N or more keys for batch loading + BatchLoader characterBatchLoader = new BatchLoader() { + @Override + public CompletionStage> load(List keys) { + // + // we use supplyAsync() of values here for maximum parellisation + // + return CompletableFuture.supplyAsync(() -> getCharacterDataViaBatchHTTPApi(keys)); + } + }; + + // a data loader for characters that points to the character batch loader + DataLoader characterDataLoader = new DataLoader(characterBatchLoader); + + // + // use this data loader in the data fetchers associated with characters and put them into + // the graphql schema (not shown) + // + DataFetcher heroDataFetcher = new DataFetcher() { + @Override + public Object get(DataFetchingEnvironment environment) { + return characterDataLoader.load("2001"); // R2D2 + } + }; + + DataFetcher friendsDataFetcher = new DataFetcher() { + @Override + public Object get(DataFetchingEnvironment environment) { + StarWarsCharacter starWarsCharacter = environment.getSource(); + List friendIds = starWarsCharacter.getFriendIds(); + return characterDataLoader.loadMany(friendIds); + } + }; + + // + // DataLoaderRegistry is a place to register all data loaders in that needs to be dispatched together + // in this case there is 1 but you can have many + // + DataLoaderRegistry registry = new DataLoaderRegistry(); + registry.register(characterDataLoader); + + // + // this instrumentation implementation will dispatched all the dataloaders + // as each level fo the graphql query is executed and hence make batched objects + // available to the query and the associated DataFetchers + // + DataLoaderDispatcherInstrumentation dispatcherInstrumentation + = new DataLoaderDispatcherInstrumentation(registry); + + // + // now build your graphql object and execute queries on it. + // the data loader will be invoked via the data fetchers on the + // schema fields + // + GraphQL graphQL = GraphQL.newGraphQL(buildSchema()) + .instrumentation(dispatcherInstrumentation) + .build(); +``` + +One thing to note is the above only works if you use `DataLoaderDispatcherInstrumentation` which makes sure `dataLoader.dispatch()` is called. If +this was not in place, then all the promises to data will never be dispatched ot the batch loader function and hence nothing would ever resolve. + +See below for more details on `dataLoader.dispatch()` ## Differences to reference implementation ### Manual dispatching -The original data loader was written in Javascript for NodeJS. NodeJS is single-threaded in nature, but simulates +The original [Facebook DataLoader](https://github.com/facebook/dataloader) was written in Javascript for NodeJS. NodeJS is single-threaded in nature, but simulates asynchronous logic by invoking functions on separate threads in an event loop, as explained [in this post](http://stackoverflow.com/a/19823583/3455094) on StackOverflow. diff --git a/src/main/java/org/dataloader/graphql/DataLoaderDispatcherInstrumentation.java b/src/main/java/org/dataloader/graphql/DataLoaderDispatcherInstrumentation.java index f9fbbe9..b98ec7a 100644 --- a/src/main/java/org/dataloader/graphql/DataLoaderDispatcherInstrumentation.java +++ b/src/main/java/org/dataloader/graphql/DataLoaderDispatcherInstrumentation.java @@ -23,6 +23,11 @@ public DataLoaderDispatcherInstrumentation(DataLoaderRegistry dataLoaderRegistry @Override public InstrumentationContext> beginExecutionStrategy(InstrumentationExecutionStrategyParameters parameters) { - return (result, t) -> dataLoaderRegistry.dispatchAll(); + return (result, t) -> { + if (t == null) { + // only dispatch when there are no errors + dataLoaderRegistry.dispatchAll(); + } + }; } } \ No newline at end of file diff --git a/src/test/java/ReadmeExamples.java b/src/test/java/ReadmeExamples.java index 562543c..b58f7c1 100644 --- a/src/test/java/ReadmeExamples.java +++ b/src/test/java/ReadmeExamples.java @@ -1,15 +1,23 @@ +import graphql.GraphQL; +import graphql.schema.DataFetcher; +import graphql.schema.DataFetchingEnvironment; +import graphql.schema.GraphQLSchema; import org.dataloader.BatchLoader; import org.dataloader.DataLoader; +import org.dataloader.DataLoaderRegistry; import org.dataloader.fixtures.User; import org.dataloader.fixtures.UserManager; +import org.dataloader.graphql.DataLoaderDispatcherInstrumentation; import java.util.List; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionStage; import java.util.stream.Collectors; +@SuppressWarnings("ALL") public class ReadmeExamples { + UserManager userManager = new UserManager(); public static void main(String[] args) { @@ -68,4 +76,82 @@ public CompletionStage> load(List userIds) { userLoader.dispatchAndJoin(); } + + class StarWarsCharacter { + List getFriendIds() { + return null; + } + } + + void starWarsExample() { + + // a batch loader function that will be called with N or more keys for batch loading + BatchLoader characterBatchLoader = new BatchLoader() { + @Override + public CompletionStage> load(List keys) { + // + // we use supplyAsync() of values here for maximum parellisation + // + return CompletableFuture.supplyAsync(() -> getCharacterDataViaBatchHTTPApi(keys)); + } + }; + + // a data loader for characters that points to the character batch loader + DataLoader characterDataLoader = new DataLoader(characterBatchLoader); + + // + // use this data loader in the data fetchers associated with characters and put them into + // the graphql schema (not shown) + // + DataFetcher heroDataFetcher = new DataFetcher() { + @Override + public Object get(DataFetchingEnvironment environment) { + return characterDataLoader.load("2001"); // R2D2 + } + }; + + DataFetcher friendsDataFetcher = new DataFetcher() { + @Override + public Object get(DataFetchingEnvironment environment) { + StarWarsCharacter starWarsCharacter = environment.getSource(); + List friendIds = starWarsCharacter.getFriendIds(); + return characterDataLoader.loadMany(friendIds); + } + }; + + // + // DataLoaderRegistry is a place to register all data loaders in that needs to be dispatched together + // in this case there is 1 but you can have many + // + DataLoaderRegistry registry = new DataLoaderRegistry(); + registry.register(characterDataLoader); + + // + // this instrumentation implementation will dispatched all the dataloaders + // as each level fo the graphql query is executed and hence make batched objects + // available to the query and the associated DataFetchers + // + DataLoaderDispatcherInstrumentation dispatcherInstrumentation + = new DataLoaderDispatcherInstrumentation(registry); + + // + // now build your graphql object and execute queries on it. + // the data loader will be invoked via the data fetchers on the + // schema fields + // + GraphQL graphQL = GraphQL.newGraphQL(buildSchema()) + .instrumentation(dispatcherInstrumentation) + .build(); + + } + + private GraphQLSchema buildSchema() { + return null; + } + + private List getCharacterDataViaBatchHTTPApi(List keys) { + return null; + } + + } diff --git a/src/test/java/org/dataloader/DataLoaderRegistryTest.java b/src/test/java/org/dataloader/DataLoaderRegistryTest.java index 897bdf3..607bb19 100644 --- a/src/test/java/org/dataloader/DataLoaderRegistryTest.java +++ b/src/test/java/org/dataloader/DataLoaderRegistryTest.java @@ -1,11 +1,11 @@ package org.dataloader; -import org.hamcrest.Matchers; import org.junit.Test; import java.util.concurrent.CompletableFuture; import static java.util.Arrays.asList; +import static org.hamcrest.Matchers.equalTo; import static org.junit.Assert.assertThat; public class DataLoaderRegistryTest { @@ -21,7 +21,7 @@ public void registration_works() throws Exception { registry.register(dlA).register(dlB).register(dlC); - assertThat(registry.getDataLoaders(), Matchers.equalTo(asList(dlA, dlB, dlC))); + assertThat(registry.getDataLoaders(), equalTo(asList(dlA, dlB, dlC))); // the same dl twice is one add @@ -30,12 +30,12 @@ public void registration_works() throws Exception { registry.register(dlA).register(dlB).register(dlC).register(dlA).register(dlB); - assertThat(registry.getDataLoaders(), Matchers.equalTo(asList(dlA, dlB, dlC))); + assertThat(registry.getDataLoaders(), equalTo(asList(dlA, dlB, dlC))); // and unregister registry.unregister(dlC); - assertThat(registry.getDataLoaders(), Matchers.equalTo(asList(dlA, dlB))); + assertThat(registry.getDataLoaders(), equalTo(asList(dlA, dlB))); } } \ No newline at end of file diff --git a/src/test/java/org/dataloader/graphql/DataLoaderDispatcherInstrumentationTest.java b/src/test/java/org/dataloader/graphql/DataLoaderDispatcherInstrumentationTest.java index 4ce80d6..c5b86af 100644 --- a/src/test/java/org/dataloader/graphql/DataLoaderDispatcherInstrumentationTest.java +++ b/src/test/java/org/dataloader/graphql/DataLoaderDispatcherInstrumentationTest.java @@ -10,7 +10,7 @@ import java.util.ArrayList; import java.util.List; import java.util.concurrent.CompletableFuture; -import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.CompletionStage; import static java.util.Arrays.asList; import static java.util.Collections.singletonList; @@ -19,20 +19,26 @@ public class DataLoaderDispatcherInstrumentationTest { - @Test - public void basic_invocation() throws Exception { + class CountingLoader implements BatchLoader { + int invocationCount = 0; + List loadedKeys = new ArrayList<>(); - AtomicInteger invocationCount = new AtomicInteger(); - final List loadedKeys = new ArrayList<>(); - final BatchLoader identityBatchLoader = keys -> { - invocationCount.incrementAndGet(); + @Override + public CompletionStage> load(List keys) { + invocationCount++; loadedKeys.add(keys); return CompletableFuture.completedFuture(keys); - }; + } + } + + @Test + public void basic_invocation() throws Exception { + + final CountingLoader batchLoader = new CountingLoader(); - DataLoader dlA = new DataLoader<>(identityBatchLoader); - DataLoader dlB = new DataLoader<>(identityBatchLoader); - DataLoader dlC = new DataLoader<>(identityBatchLoader); + DataLoader dlA = new DataLoader<>(batchLoader); + DataLoader dlB = new DataLoader<>(batchLoader); + DataLoader dlC = new DataLoader<>(batchLoader); DataLoaderRegistry registry = new DataLoaderRegistry() .register(dlA) .register(dlB) @@ -48,9 +54,28 @@ public void basic_invocation() throws Exception { context.onEnd(null, null); - assertThat(invocationCount.get(), equalTo(3)); + assertThat(batchLoader.invocationCount, equalTo(3)); // will be [[A],[B],[C]] - assertThat(loadedKeys, equalTo(asList(singletonList("A"), singletonList("B"), singletonList("C")))); + assertThat(batchLoader.loadedKeys, equalTo(asList(singletonList("A"), singletonList("B"), singletonList("C")))); + } + + @Test + public void exceptions_wont_cause_dispatches() throws Exception { + final CountingLoader batchLoader = new CountingLoader(); + + DataLoader dlA = new DataLoader<>(batchLoader); + DataLoaderRegistry registry = new DataLoaderRegistry() + .register(dlA); + + DataLoaderDispatcherInstrumentation dispatcher = new DataLoaderDispatcherInstrumentation(registry); + InstrumentationContext> context = dispatcher.beginExecutionStrategy(null); + + // cause some activity + dlA.load("A"); + + context.onEnd(null, new RuntimeException("Should not run")); + + assertThat(batchLoader.invocationCount, equalTo(0)); } } \ No newline at end of file From e9c801a0f5dd071962b1b8e5bc7d0743904df4a8 Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Fri, 1 Sep 2017 15:48:47 +1000 Subject: [PATCH 3/3] Bad test design --- src/test/java/org/dataloader/DataLoaderTest.java | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/test/java/org/dataloader/DataLoaderTest.java b/src/test/java/org/dataloader/DataLoaderTest.java index 652a97a..7b71ac9 100644 --- a/src/test/java/org/dataloader/DataLoaderTest.java +++ b/src/test/java/org/dataloader/DataLoaderTest.java @@ -831,6 +831,8 @@ public void should_Batch_loads_occurring_within_futures() { Supplier nullValue = () -> null; + AtomicBoolean v4Called = new AtomicBoolean(); + CompletableFuture.supplyAsync(nullValue).thenAccept(v1 -> { identityLoader.load("a"); CompletableFuture.supplyAsync(nullValue).thenAccept(v2 -> { @@ -838,12 +840,16 @@ public void should_Batch_loads_occurring_within_futures() { CompletableFuture.supplyAsync(nullValue).thenAccept(v3 -> { identityLoader.load("c"); CompletableFuture.supplyAsync(nullValue).thenAccept( - v4 -> - identityLoader.load("d")); + v4 -> { + identityLoader.load("d"); + v4Called.set(true); + }); }); }); }); + await().untilTrue(v4Called); + identityLoader.dispatchAndJoin(); assertThat(loadCalls, equalTo(