Cloud SQL (MySQL): Add Cloud Run support and fix broken implementation #1508
Conversation
googlebot
added
the
cla: yes
cloud-sql/mysql/mysql/server.js
Outdated
| @@ -43,7 +43,7 @@ const logger = winston.createLogger({ | |||
| }); | |||
|
|
|||
| // [START cloud_sql_mysql_mysql_create] | |||
| const pool = mysql.createPool({ | |||
| const connectionOptions = { | |||
There was a problem hiding this comment.
This changes moves the pool creation outside of the snippet, which we probably don't want to do since that's what this snippet is for.
There was a problem hiding this comment.
Could you double-check the diff? I deliberately tried to keep things inside the correct region tags, and it still reads that way to me.
There was a problem hiding this comment.
My mistake, I mismatched the tags.
Downgrading this to a small nit: Can we move connectionOptions inline? It's not being used anywhere else, so I think it would be more clear inside of mysql.createPool.
There was a problem hiding this comment.
Can do. I broke it out because I like to be able to read the reasoning around a try/await/catch without scrolling.
I took a quick look at the test for this sample, and the test has a high reliance on stubbing. I expect the stubbing mechanism prevented the test from making the distinction between returning the pool and returning a promise. I'm happy to fix things up a bit, but I don't have bandwidth to deepen the tests. |
kokoro-team
removed
the
kokoro:run
|
lgtm. Using less stubbing in the tests is out of scope. Can you make the linter happy? |
cloud-sql/mysql/mysql/server.js
Outdated
| @@ -43,7 +43,7 @@ const logger = winston.createLogger({ | |||
| }); | |||
|
|
|||
| // [START cloud_sql_mysql_mysql_create] | |||
| const pool = mysql.createPool({ | |||
| const connectionOptions = { | |||
There was a problem hiding this comment.
My mistake, I mismatched the tags.
Downgrading this to a small nit: Can we move connectionOptions inline? It's not being used anywhere else, so I think it would be more clear inside of mysql.createPool.
|
Feedback incorporated, linting fixed. Waiting for tests. |
This PR originally intended just the following:
In the course of the Cloud Run work, I found the current state of the sample is broken. This PR includes fixes for those problems:
mysql.createPool(). pool instantiation in the master implementation cannot have worked since the library update. Fix: Hacky top-level async.app.on('listening', async () => {... is not being fired (express 4, node 10), meaning the votes table is never created and the client is left hanging while the Node process crashes. Fix: Move the table creation directly after pool creation. Note also the "votes tables" query is an important step in validating the pool configuration as part of service startup. Cloud Run will flag a revision as unhealthy if it fails before listening.The top-level async/await is a little kludgy, happy to clean it up with additional guidance. One consideration: the potential errors from the mysql library do not seem to benefit from a stack trace, the error message itself is sufficient for troubleshooting purposes (though maybe the goal is to have automatic Stackdriver Error Reporting support?)