-
Notifications
You must be signed in to change notification settings - Fork 3
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
perf: Add connection pool to configure connection lifecycle. #289
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.
Reviewed 4 of 5 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tristanvuong2021)
src/main/kotlin/org/wfanet/measurement/gcloud/postgres/PostgresConnectionFactories.kt
line 38 at r2 (raw file):
.option(ConnectionFactoryOptions.DATABASE, flags.database) .option(ConnectionFactoryOptions.HOST, flags.cloudSqlInstance) .option(ConnectionFactoryOptions.STATEMENT_TIMEOUT, Duration.ofSeconds(120))
Have all of these values come from flags.
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: 4 of 6 files reviewed, 1 unresolved discussion (waiting on @SanjayVas)
src/main/kotlin/org/wfanet/measurement/gcloud/postgres/PostgresConnectionFactories.kt
line 38 at r2 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
Have all of these values come from flags.
Done.
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 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tristanvuong2021)
src/main/kotlin/org/wfanet/measurement/gcloud/postgres/PostgresFlags.kt
line 50 at r3 (raw file):
required = false, ) var statementTimeoutSeconds: Long = 120
Use Duration for these
Suggestion:
names = ["--statement-timeout"],
description = ["Statement timeout"],
required = false,
defaultValue = "120s",
)
lateinit var statementTimeout: Duration
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: 4 of 6 files reviewed, 1 unresolved discussion (waiting on @SanjayVas)
src/main/kotlin/org/wfanet/measurement/gcloud/postgres/PostgresFlags.kt
line 50 at r3 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
Use Duration for these
Done.
fee87f1
to
e24fa72
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.
Reviewed 4 of 4 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @tristanvuong2021)
src/main/kotlin/org/wfanet/measurement/gcloud/postgres/PostgresFlags.kt
line 51 at r4 (raw file):
required = false, ) var statementTimeout: Duration = Duration.ofSeconds(120)
Don't assign the default in Kotlin. Make these lateinit vars and use defaultValue from the Option annotation
Code quote:
= Duration.ofSeconds(120)
src/main/kotlin/org/wfanet/measurement/gcloud/postgres/PostgresFlags.kt
line 55 at r4 (raw file):
@CommandLine.Option( names = ["--max-idle-time-minutes"],
Suggestion:
--max-idle-time
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: 5 of 6 files reviewed, 2 unresolved discussions (waiting on @SanjayVas)
src/main/kotlin/org/wfanet/measurement/gcloud/postgres/PostgresFlags.kt
line 51 at r4 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
Don't assign the default in Kotlin. Make these lateinit vars and use defaultValue from the Option annotation
Done.
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: 5 of 6 files reviewed, 2 unresolved discussions (waiting on @SanjayVas)
src/main/kotlin/org/wfanet/measurement/gcloud/postgres/PostgresFlags.kt
line 55 at r4 (raw file):
@CommandLine.Option( names = ["--max-idle-time-minutes"],
Done.
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 all commit messages.
Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @tristanvuong2021)
src/main/kotlin/org/wfanet/measurement/gcloud/postgres/PostgresFlags.kt
line 50 at r5 (raw file):
description = ["Statement timeout duration."], required = false, defaultValue = "PT120S",
This should be using human readable durations. I've stated in a previous PR that org.wfanet.measurement.common.DurationFormat
shouldn't exist and it should always use the human-readable format, i.e. that #174 should never have been merged.
Suggestion:
120s
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: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @SanjayVas)
src/main/kotlin/org/wfanet/measurement/gcloud/postgres/PostgresFlags.kt
line 50 at r5 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
This should be using human readable durations. I've stated in a previous PR that
org.wfanet.measurement.common.DurationFormat
shouldn't exist and it should always use the human-readable format, i.e. that #174 should never have been merged.
Done.
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 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tristanvuong2021)
src/main/kotlin/org/wfanet/measurement/gcloud/postgres/PostgresFlags.kt
line 69 at r6 (raw file):
required = false, ) var maxPoolSize: Int = 16
Same here re: not setting defaults in Kotlin when they can be set using defaultValue
Suggestion:
var maxPoolSize by Delegates.notNull<Int>()
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: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @SanjayVas)
src/main/kotlin/org/wfanet/measurement/gcloud/postgres/PostgresFlags.kt
line 69 at r6 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
Same here re: not setting defaults in Kotlin when they can be set using defaultValue
Done.
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 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @stevenwarejones)
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 2 of 5 files at r1, 3 of 4 files at r4, 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @tristanvuong2021)
The pool will close connections that are idle for too long.