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

Implement lambda BatchInvoke #99

Merged
merged 3 commits into from Mar 12, 2019

Conversation

Projects
None yet
2 participants
@bboure
Copy link
Collaborator

bboure commented Mar 12, 2019

Uses data-loader in order to group requests and call the resolver by batch.

@bboure bboure requested review from lightsofapollo and cbaron Mar 12, 2019

@bboure bboure changed the title Implement lambda BatchInvoke [WIP] Implement lambda BatchInvoke Mar 12, 2019

@bboure bboure changed the title [WIP] Implement lambda BatchInvoke Implement lambda BatchInvoke Mar 12, 2019

@@ -187,31 +188,58 @@ const dispatchRequestToSource = async (
}
};

const batchLoaders = {};

This comment has been minimized.

@lightsofapollo

lightsofapollo Mar 12, 2019

Contributor

I am not a fan of having these be global. Is there another place we can put these (such as a new object in generateTypeResolve along with requestPath?)

This comment has been minimized.

@bboure

bboure Mar 12, 2019

Author Collaborator

Yeah, I guess we could move that somewhere in a better place.
Ideally, the batchRequests should be injected into the load function (instead of a key), but the dataloader does not work like that.
An option would be a custom implementation of the dataloader but I did not want to overcomplicate this either.

data loaders could probably go in generateTypeResolve(), that sounds like the right thing to do.

@lightsofapollo
Copy link
Contributor

lightsofapollo left a comment

@bboure Thanks for addressing this I am +1 on the approach but have a concern about the implementation (see my comment). Let me know if you need any help.

@bboure bboure force-pushed the bboure:feature/batch-invoke branch from 7878e1c to da20f7d Mar 12, 2019

@bboure bboure force-pushed the bboure:feature/batch-invoke branch from da20f7d to d23780e Mar 12, 2019

@bboure

This comment has been minimized.

Copy link
Collaborator Author

bboure commented Mar 12, 2019

@lightsofapollo
Here is what I did:

  • instead of injecting keys, I inject the request into the Dataloader. That works well.
  • wrapped the data loader generator into another function. Each field has its own generator.

note: tests are broken. it seems that they were broken here

@bboure bboure requested a review from lightsofapollo Mar 12, 2019

@lightsofapollo
Copy link
Contributor

lightsofapollo left a comment

LGTM

@lightsofapollo

This comment has been minimized.

Copy link
Contributor

lightsofapollo commented Mar 12, 2019

@bboure You can land this anytime (My preferred workflow is to have someone review then land myself after I get approval). Looks like problem in CI is specific to node 8 ?

@bboure

This comment has been minimized.

Copy link
Collaborator Author

bboure commented Mar 12, 2019

I haven't looked at the issue but it seems related to GoLang.
Issue seems random though. Could be related to env setup?
https://travis-ci.org/ConduitVC/aws-utils/builds/505350795
https://travis-ci.org/ConduitVC/aws-utils/builds/505360448

@bboure bboure merged commit d819fc8 into ConduitVC:master Mar 12, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@bboure bboure deleted the bboure:feature/batch-invoke branch Mar 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.