Skip to content
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

feat: transaction batcher module #1348

Merged
merged 25 commits into from
Feb 23, 2022

Conversation

samuelAndalon
Copy link
Contributor

@samuelAndalon samuelAndalon commented Jan 24, 2022

📝 Description

transaction-batcher module is an alternative to data-loaders, using reactive stream to apply batching and deduplication of transactions needed by data fetchers, the implementation is completely agnostic of GraphQL and compatible with any asynchronous approach used to resolve data.
it also supports caching to avoid making a request to a previous completed transaction.

🔗 Related Issues

@samuelAndalon samuelAndalon marked this pull request as ready for review January 26, 2022 18:09
@dariuszkuc
Copy link
Collaborator

@samuelAndalon can you add copyright to all new source files?

@samuelAndalon samuelAndalon added the type: enhancement New feature or request label Jan 31, 2022
* Interface representing a publisher with input [TInput] type and output [TOutput] type
*/
@Suppress(
"ReactiveStreamsSubscriberImplementation",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we suppressing this warning? is this valid?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is valid, given that Subscriber is an interface, so it's valid to implement it, However a warning appears in the IDE when trying to do it.

i can see a lot of Subscriber implementations that need to add that suppress as well
https://github.com/search?p=2&q=%22ReactiveStreamsSubscriberImplementation%22&type=Code

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unsure, I am not getting any warning in my local

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

@dariuszkuc dariuszkuc merged commit 4591fde into ExpediaGroup:master Feb 23, 2022
@rocketraman
Copy link
Contributor

Why does this use reactive-streams instead of kotlinx-coroutines Flow? Flow can be transformed to reactive-streams by users, if desired.

@dariuszkuc
Copy link
Collaborator

We decided to use generic reactive streams Publisher for flexibility to allow users to use various implementations (including coroutines).

@samuelAndalon samuelAndalon deleted the feat/transaction-batcher branch February 23, 2022 17:26
@samuelAndalon
Copy link
Contributor Author

samuelAndalon commented Feb 23, 2022

reactive streams and all his implementations like project-reactor have 100% interoperability with other async technologies like Java Flow, coroutines, completableFuture etc.
So the most generic choice was RX, at the beginning we were scoping for java flow, but that requires target JVM to be 9.

@rocketraman
Copy link
Contributor

Ok. Given this is a Kotlin project, I would have thought coroutines Flow would be the obvious choice as first-party implementation. As stated, it is easy to convert from Flow to other reactive streams implementations i.e. flow.asPublisher().

Is there any documentation in the works for this feature? Seems really interesting.

@samuelAndalon
Copy link
Contributor Author

samuelAndalon commented Feb 23, 2022

Once we add the instrumentation that will signal when to dispatch, we will add the documentation.
It will be pretty much similar to the DataLoaderDispatcherInstrumentation from graphql-java but applied to more than 1 operation if client sent a request with batched operations.

for now you can refer to the unit tests

@dariuszkuc dariuszkuc added the changes: minor Changes require a minor version label Feb 28, 2022
dariuszkuc pushed a commit to dariuszkuc/graphql-kotlin that referenced this pull request Aug 5, 2022
### 📝 Description
`transaction-batcher` module is an alternative to `data-loaders`, using reactive stream to apply batching and deduplication of transactions needed by data fetchers, the implementation is completely agnostic of GraphQL and compatible with any asynchronous approach used to resolve data.
it also supports caching to avoid making a request to a previous completed transaction.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes: minor Changes require a minor version type: enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

4 participants