-
Notifications
You must be signed in to change notification settings - Fork 534
RUBY-3149 Test on AWS Lambda #2800
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
Conversation
2c8833c
to
1677895
Compare
1677895
to
0b8a747
Compare
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.
It looks good so far! Do you feel like refactoring the app to avoid global variables is worth doing? I kind of lean that way myself, but if you feel strongly about it, I'm not willing to shed blood over it or anything. 😆
@@ -510,6 +514,52 @@ task_groups: | |||
tasks: | |||
- test-full-atlas-task |
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.
I was adding the FaaS task here, as another task under the atlas task group, so we only set up a single cluster per evergreen run. I think the fewer atlas instances we start, the better. It may be that this task group and the matrix variant that runs it need a better, more general name now, though.
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.
Oh, that is a great idea! Updated accordingly.
|
||
cd "${TEST_LAMBDA_DIRECTORY}" | ||
|
||
sam build --use-container |
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.
❤️
end | ||
|
||
def lambda_handler(event:, context:) | ||
db = $client.use('lambda_test') |
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.
Does $client
need to be a global, here? Is there any reason not to instantiate the client within the handler function?
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.
It is recommended to have a global client instance:
Don't define a new MongoClient object each time you invoke your function. Doing so causes the driver to create a new database connection with each function call. This can be expensive and can result in your application exceeding database connection limits.
So, we need some kind of the global object here. We can work it around, i.e. using a singleton, but I'd keep it simple and dumb :-)
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.
Ah! I didn't know about that constraint. Never mind me, then. 👍 Looks good!
$client.use('lambda_test').database.list_collections | ||
puts 'Connected' | ||
|
||
def reset_counters |
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.
The global variables make me think that maybe this should be encapsulated in an object, like a FaaSRunner
class or something.
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.
Got rid of most of the global variables, except $client
and $stats_aggregator
. I think it is cleaner now, thank you!
2f4226c
to
587d507
Compare
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.
Looks great!
7b18028
to
e08a574
Compare
e08a574
to
b3d4fcc
Compare
https://jira.mongodb.org/browse/RUBY-3149
https://jira.mongodb.org/browse/RUBY-3283