-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[BREAKING] feat: bring dgraph-lambda to dgraph, alpha launches lambda server #7973
Conversation
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.
Nice work. lambda should be its own flag, so DQL queries can call into lambdas later. And by default, we can set it to 1, so at least one lambda server is always running. I don't think we need lambda-url
. We'd be forking lambda servers from within Dgraph.
Reviewed 25 of 36 files at r3, 2 of 3 files at r4, 10 of 16 files at r5, 19 of 19 files at r6, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @NamanJain8)
dgraph/cmd/alpha/run.go, line 236 at r6 (raw file):
" Use --lambda-cnt to launch official lambda server."+ " This flag if set, overrides the other lambda flags."). Flag("lambda-cnt",
lambda should have its own flag. We'd want to run lambda functions via DQL too.
dgraph/cmd/alpha/run.go, line 747 at r6 (raw file):
PollInterval: graphql.GetDuration("poll-interval"), LambdaUrl: graphql.GetString("lambda-url"), LambdaCnt: graphql.GetUint32("lambda-cnt"),
move them to their own flags.x
graphql/e2e/directives/docker-compose.yml, line 4 at r6 (raw file):
services: zero1: image: namanj1811/dgraph:lambda
Would this need to be changed?
lambda/slash-graphql-lambda-types/package.json, line 15 at r6 (raw file):
}, "author": "Tejas Dinkar <[email protected]>", "license": "Apache-2.0"
can you remove the "slash-graphql" name from everywhere?
Also, license should be Dgraph license. Author should be removed. Here and elsewhere.
x/x.go, line 1480 at r6 (raw file):
func LambdaUrl(ns uint64) string { lambdaUrl := Config.GraphQL.LambdaUrl if len(lambdaUrl) > 0 {
Do we need lambdaUrl? We just need the port number, right? Lambdas would be running in the same process.
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.
Dismissed @manishrjain from 2 discussions.
Reviewable status: 31 of 58 files reviewed, 3 unresolved discussions (waiting on @danielmai and @manishrjain)
dgraph/cmd/alpha/run.go, line 236 at r6 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
lambda should have its own flag. We'd want to run lambda functions via DQL too.
Done.
dgraph/cmd/alpha/run.go, line 747 at r6 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
move them to their own flags.x
Done.
graphql/e2e/directives/docker-compose.yml, line 4 at r6 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Would this need to be changed?
Yes, this needs to be changed. @danielmai can you please push a new image to dockerhub?
x/x.go, line 1480 at r6 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Do we need lambdaUrl? We just need the port number, right? Lambdas would be running in the same process.
Lambda URL is kept so that community is free to create their own lambda servers in the language of their choice.
lambda/slash-graphql-lambda-types/package.json, line 15 at r6 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
can you remove the "slash-graphql" name from everywhere?
Also, license should be Dgraph license. Author should be removed. Here and elsewhere.
I have replaced the slash-graphql to dgraph-lambda now.
As discussed, I have kept it under Apache License.
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.
Reviewed 27 of 27 files at r7, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @danielmai and @manishrjain)
dgraph/cmd/alpha/run.go, line 236 at r6 (raw file):
Previously, NamanJain8 (Naman Jain) wrote…
Done.
Don't remove vowels. If you want shorter, use num
.
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.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @danielmai and @manishrjain)
NVM was only needed and loaded when building Ratel. Now, Node is needed to build lambdas. Load NVM always in the release srcipt and use the Node LTS release for the build.
This PR brings in dgraph-lambda to dgraph repo.
--lambda
. It has 3 subflagsurl
(earlier--graphql lambda-url
),cnt
andport
.--lambda url
when set, overrides other subflags.Note: Users are free to set up their own lambda server and use
--lambda url
flag. That would need changes in the lambda response.This change is