GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Allow scheduling of CF completion in parallel by fxbonnet · Pull Request #277 · graphql-java/java-dataloader

Conversation

@fxbonnet

@fxbonnet fxbonnet commented May 18, 2026

Copy link
Copy Markdown

When a batch loading function returns, we currently complete all CompletableFutures sequentially on the calling thread. Since CompletableFuture.complete(...) also executes any continuations attached via thenApply(...) or thenCompose(...), large batches can significantly delay the return of dispatch().

This PR introduces an option to execute these continuations in parallel instead.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
bbakerman added 2 commits May 17, 2026 12:47
@bbakerman
Allow scheduling of CF completion
3075fcc
@bbakerman
Allow scheduling of CF completion
8eadcb2
bbakerman
bbakerman reviewed May 18, 2026
View reviewed changes
Comment thread src/main/java/org/dataloader/DataLoaderHelper.java
clearCacheKeys.add(key);
}
} else {
future.complete(value);

@bbakerman bbakerman May 18, 2026

Copy link
Copy Markdown
Member

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 misses the Try path of completion

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@andimarek

andimarek commented May 18, 2026

Copy link
Copy Markdown
Member

@fxbonnet I am not sure about this PR: with the new hook introduced by @bbakerman in #275 you can already offload every completion onto a new thread and hence it will run in parallel if you choose todo so. I have problems seeing how this approach is different/better.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@fxbonnet
Allow BatchLoaderScheduler process all CF completions as a batch and …
e04c76d
…potentially in parallel
@fxbonnet
fxbonnet force-pushed the allow-scheduling-of-future-completions-in-parallel branch from dd2af99 to e04c76d Compare May 18, 2026 22:29
@fxbonnet

fxbonnet commented May 18, 2026

Copy link
Copy Markdown
Author

@andimarek I created this PR after my comment on @bbakerman 's PR #275 (comment)
The PR allowed to offload the completions to a separate thread, but in case the batch load contains multiple keys, the completions for each key would still happen sequentially.
For clarity I just rebased this PR onto @bbakerman 's branch. The main change is that instead of passing a single Runnable to scheduleCompletion, we would pass a List<Runnable> (one for each key), giving the option to execute them in parallel.

bbakerman reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

fxbonnet
fxbonnet commented May 18, 2026
View reviewed changes
Comment thread src/main/java/org/dataloader/DataLoaderHelper.java
}

List<K> clearCacheKeys = new ArrayList<>();
Collection<K> clearCacheKeys = new ConcurrentLinkedQueue<>();

@fxbonnet fxbonnet May 18, 2026

Copy link
Copy Markdown
Author

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.

clearCacheKeys may now be accessed concurrently by multiple thread if we execute the Runnables in parallel.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@bbakerman bbakerman May 19, 2026

Copy link
Copy Markdown
Member

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.

good call - price of side effects outside a thread

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
bbakerman
bbakerman approved these changes May 19, 2026
View reviewed changes
andimarek
andimarek requested changes May 20, 2026
View reviewed changes

@andimarek andimarek left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

Requesting changes for a semantic regression around ValueCache hits and context-aware batch loaders.

I opened #278 as a small draft PR showing one possible fix: #278

The PR now constructs a BatchLoaderEnvironment before value-cache filtering:

BatchLoaderEnvironment environment = mkBatchLoaderEnv(keys, keyContexts);
CompletableFuture<List<V>> batchLoad = invokeLoader(environment, keys, keyContexts, queuedFutures, loaderOptions.cachingEnabled());

But invokeLoader(..., cachingEnabled=true) can then remove value-cache hits and invoke the actual batch loader with only missedKeys / missedKeyContexts while still passing the original environment:

CompletableFuture<List<V>> batchLoad = invokeLoader(environment, missedKeys, missedKeyContexts, missedQueuedFutures);

That breaks the existing semantics of BatchLoaderEnvironment: environment.getKeyContextsList() is expected to be aligned with the keys argument passed to the loader. A concrete failure case:

  1. The queued dispatch contains keys=[A, B] and keyContexts=[ctxA, ctxB].
  2. ValueCache returns a hit for A and a miss for B.
  3. The actual batch loader is invoked with keys=[B].
  4. The reused environment was built from [A, B], so environment.getKeyContextsList().get(0) is ctxA, not ctxB.

That means a BatchLoaderWithContext implementation that indexes environment.getKeyContextsList() alongside its keys argument can load B using A's context. This is observable behavior and can be a security/data-isolation issue if key contexts carry tenant, auth, locale, or request-scoped metadata.

Please build the BatchLoaderEnvironment from the actual key/context list passed to the concrete loader call after value-cache filtering, or otherwise preserve the invariant that environment.getKeyContextsList() is ordered and sized to match the loader's keys argument. This should also get a regression test with a partial ValueCache hit and a context-aware batch loader asserting that the missed key receives its own context.

PR #278 does exactly that by rebuilding the environment for missedKeys / missedKeyContexts before the concrete loader invocation and adding the regression test.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@andimarek andimarek mentioned this pull request May 20, 2026
Fix value cache batch loader environment #278
Draft
@andimarek

andimarek commented May 20, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

I opened a small draft PR with one possible fix for the ValueCache / BatchLoaderEnvironment context alignment issue: #278

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@andimarek @fxbonnet
Fix value cache batch loader environment
2eb399b
@fxbonnet

fxbonnet commented May 21, 2026

Copy link
Copy Markdown
Author

Thanks @andimarek I have added your commit to the PR

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@fxbonnet
fxbonnet requested a review from andimarek May 21, 2026 07:12
@bbakerman

bbakerman commented May 23, 2026

Copy link
Copy Markdown
Member

This should now be ready - it has the extra fix @andimarek wanted

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@fxbonnet

fxbonnet commented Jun 16, 2026

Copy link
Copy Markdown
Author

Hi @andimarek is there something I need to do for this PR to be ready to merge?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions

github-actions Bot commented Aug 16, 2026

Copy link
Copy Markdown

Hello, this pull request has been inactive for 60 days, so we're marking it as stale. If you would like to continue working on this pull request, please make an update within the next 30 days, or we'll close the pull request.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot added the Stale label Aug 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bbakerman bbakerman bbakerman approved these changes
@andimarek andimarek Awaiting requested review from andimarek

Requested changes must be addressed to merge this pull request.

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

Stale

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@fxbonnet @andimarek @bbakerman
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.