GeezerVison

Note to self to follow up on - I believe thinking on this again this may be a greedy level by level traversal rather than true BFS. I will come back to this and add a more complicated test to replicate the example I have to verify if async are waited on a level basis or we continue dispatching to child levels when work is known and only wait if we are blocked on just async calls

Note to self to follow up on - I believe thinking on this again this may be a greedy level by level traversal rather than true BFS. I will come back to this and add a more complicated test to replicate the example I have to verify if async are waited on a level basis or we continue dispatching to child levels when work is known and only wait if we are blocked on just async calls

Wrote out a fairly complicated test in GreedyLevelByLevelChainedDispatchStrategyStressTest to verify the behavior in practice. As a result renamed this as its not a true BFS. In practice we see increased batch sizes compared to our previous ticker mode with scheduled registry so while this is not a true level by level traversal we still see a great balance of speed + efficiency in our real life scenario

@andimarek This one should be about ready. Let me know what you think when you're available to review

ps the test are failing

GreedyLevelByLevelChainedDispatchStrategyTest > singleDepthLoadSucceedsMultipleTimes() FAILED
    java.lang.NullPointerException: Cannot invoke "java.lang.Runnable.run()" because "this.onIteration" is null
        at org.dataloader.strategy.GreedyLevelByLevelChainedDispatchStrategy.triggerDeterministicDispatch(GreedyLevelByLevelChainedDispatchStrategy.java:82)
        at org.dataloader.strategy.GreedyLevelByLevelChainedDispatchStrategy.loadCalled(GreedyLevelByLevelChainedDispatchStrategy.java:61)
        at org.dataloader.DataLoaderHelper.load(DataLoaderHelper.java:191)
        at org.dataloader.DataLoader.loadImpl(DataLoader.java:202)
        at org.dataloader.DataLoader.load(DataLoader.java:216)
        at 

ps the test are failing

GreedyLevelByLevelChainedDispatchStrategyTest > singleDepthLoadSucceedsMultipleTimes() FAILED
    java.lang.NullPointerException: Cannot invoke "java.lang.Runnable.run()" because "this.onIteration" is null
        at org.dataloader.strategy.GreedyLevelByLevelChainedDispatchStrategy.triggerDeterministicDispatch(GreedyLevelByLevelChainedDispatchStrategy.java:82)
        at org.dataloader.strategy.GreedyLevelByLevelChainedDispatchStrategy.loadCalled(GreedyLevelByLevelChainedDispatchStrategy.java:61)
        at org.dataloader.DataLoaderHelper.load(DataLoaderHelper.java:191)
        at org.dataloader.DataLoader.loadImpl(DataLoader.java:202)
        at org.dataloader.DataLoader.load(DataLoader.java:216)
        at 

I fixed that.

@zsmoore @bbakerman I changed the strategy callback a bit to ensure it is always called before the load itself is returned.

@zsmoore the stress test is failing now (not sure if that was the result of my change, it could be).

I will continue to review.

@zsmoore some questions, clarifications:

In your test your "chained dataloader" is actually a few DataLoader.load calls in parallel. What we refer normally to as chained dataloader is

dl1.load(key1).compose(result1 -> var key2 <based on result1)>; return dl2.load(key2))

In CF terms we compose the result of 1 DL to call a second one.

We also have a loadMany function that would server the as the same purpose I believe.

In your test dispatchGoesByLevel you do actually something we have not really seen before I believe: you use a new DataLoader inside the BatchLoader itself, correct? You do it twice inside "root" and inside "secondLevel".

This is again a bit different than the use cased described above we called "chained".

So that is what you are referring to as "level"?

Regarding your strategy:

In my understanding it works like this:
First load triggers dispatch. The dispatch keep calling dispatch as long as there is work todo. You keep a lock for the whole dispatch time. In case the work is not done synchronously it will triggers a delayed dispatch after 30ms to dispatch all DataLoader that were asynchronously queued.

@bbakerman @andimarek apologies for the delay, I was ill the past couple of weeks.

Changes since last commits -
1 - Folded the 2 whenCompletes into 1, i agree we can keep the order deterministic for the callbacks this way
2 - Changes to tests WRT equality, dependencies, and exception unfolding Why? With the latest change which made the returned future the chained '.whenComplete' future, we now cannot use an equality assertion as the future is not pass through. This is most likely the preferred behavior as we won't lose exceptions and will bubble them up.

@zsmoore some questions, clarifications:

In your test your "chained dataloader" is actually a few DataLoader.load calls in parallel. What we refer normally to as chained dataloader is

dl1.load(key1).compose(result1 -> var key2 <based on result1)>; return dl2.load(key2))

In CF terms we compose the result of 1 DL to call a second one.

We also have a loadMany function that would server the as the same purpose I believe.

In your test dispatchGoesByLevel you do actually something we have not really seen before I believe: you use a new DataLoader inside the BatchLoader itself, correct? You do it twice inside "root" and inside "secondLevel".

This is again a bit different than the use cased described above we called "chained".

So that is what you are referring to as "level"?

Regarding your strategy:

In my understanding it works like this: First load triggers dispatch. The dispatch keep calling dispatch as long as there is work todo. You keep a lock for the whole dispatch time. In case the work is not done synchronously it will triggers a delayed dispatch after 30ms to dispatch all DataLoader that were asynchronously queued.

For the understanding of the strategy yes that is the correct understanding of how the strategy is working.

As for the verbage around chained I see the point around the mixing of chaining. We actually have both cases in our use case take for example

Nested chaining
First Name DataLoader (input profile nickname) ->
Call Profile DataLoader (input profile nickname)
return profile.firstName

Top level chaining
(Get Location)
Call Profile DataLoader.compose(call location dataloader using profile id)

When referring to level, I think of the entire call chain as a tree structure.

Take both example above

Nested Chaining
Root function = 0
Call FirstName DL = 1
Call Profile DL = 2

Top level Chaining
Root function = 0
Call ProfileDL = 1
Call LocationDL = 2

In both cases the 'level' or depth of the chain is the same as it takes the same amount of subsequent DL dependents to get the overall result but i believe the difference comes down to organization. You could still swap the structure to be
LocationDL (input profile nickname)
Call ProfileDL (find by profile nickname)
use profileID to call Location endpoint and return

Where this composition is encapsulated in the overall DL itself instead of a top level unfolding of the calls.

Let me know if i understood the question correctly. Thanks again and apologies for the delay

Also WRT the parallelization, that is just added to bump up the complexity for the stress test, nothing really core to the test here

@bbakerman @andimarek friendly ping on this one

hey @zsmoore ... sorry it has been really busy the last week. Will try to come back to this soon.

No worries at all, the beginning of the year is always crazy

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.

Hello, as this pull request has been inactive for 90 days, we're closing this pull request. We always welcome contributions, and if you would like to continue, please open a new pull request.

Reopening - I wanted to chat about this before the bot closes it

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.

This bot is a scourge...