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.

Sign up for GitHub

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

Jump to bottom

feat(sql): use a connection pool named "read" for some read operations in SqlExecutionRepository #4803

Merged
merged 6 commits into from
Jan 3, 2025
Merged

feat(sql): use a connection pool named "read" for some read operations in SqlExecutionRepository #4803

merged 6 commits into from
Jan 3, 2025

Conversation

kirangodishala
Copy link
Contributor

@kirangodishala kirangodishala commented Nov 20, 2024 •
edited by dbyron-sf
Loading

Introduce a read pool for some read-only database queries in SqlExecutionRepository. Enable this by configuring an additional connection pool named "read", like this:

sql:
  connectionPools:
    default:
      <...>
    read:
      jdbcUrl: jdbc:...
      user: orca_service
      password: 
      connectionTimeoutMs: 
      validationTimeoutMs: 
      maxPoolSize:
      minIdle:
      maxLifetimeMs:
      idleTimeoutMs:

Additional read-only database queries happen in places that expect to see "just-written" contents. They're not safe to convert to use the read replica until there's logic in place to be aware of replication lag, and wait until the read replica is up-to-date.

dzhengg
dzhengg reviewed Nov 21, 2024
View reviewed changes
Copy link
Contributor

@dzhengg dzhengg left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we include an example (maybe in one of the commit messages or the description) for how to configure orca to use a read pool?

@dbyron-sf dbyron-sf changed the title feat(sql): use a connection pool named "read" for read operations in SqlExecutionRepository feat(sql): use a connection pool named "read" for some read operations in SqlExecutionRepository Nov 21, 2024
@dbyron-sf dbyron-sf marked this pull request as draft November 21, 2024 22:09
@dbyron-sf dbyron-sf mentioned this pull request Dec 3, 2024
Merged
@dbyron-sf @kirangodishala
test(sql): demonstrate that SqlConfiguration makes different types of…
9bcb1bb 
… DataSource bean available

depending on configuration
@kirangodishala kirangodishala force-pushed the use-read-replicas-in-execution-repository branch 2 times, most recently from d2a56fe to 53927e2 Compare December 24, 2024 14:38
@kirangodishala kirangodishala marked this pull request as ready for review December 24, 2024 15:16
@dbyron-sf @kirangodishala
feat(sql): use a connection pool named "read" for read operations in …
8ec1ff4 
…SqlExecutionRepository if configured to do so.

WIP: so far this is only configuration, nothing actually uses the read pool
@dbyron-sf @kirangodishala
refactor(sql): reorganize correlation id cleanup logic in SqlExecutio…
9366235 
…nRepository

to pave the way to use the read pool for selecting by correlation id
@dbyron-sf @kirangodishala
perf(sql): no need for a transaction to delete individual rows
78abe77 
- correlation ids for completed pipelines/orchestrations
- a stage
@kirangodishala kirangodishala force-pushed the use-read-replicas-in-execution-repository branch from 53927e2 to 942d48c Compare December 25, 2024 05:02
jasonmcintosh
jasonmcintosh reviewed Jan 2, 2025
View reviewed changes
...rc/main/kotlin/com/netflix/spinnaker/orca/sql/pipeline/persistence/SqlExecutionRepository.kt
jooq.transactional {
it.deleteFrom(table("correlation_ids")).where(field("id").eq(correlationId)).execute()
}
if (execution == null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A POTENTIAL concern since we ARE querying now from the read pool: Can the orchestration be on a delayed sync, so it'd actually exist in the RW & not be sync'd yet? Wondering if a fallback is needed in such a case. OR this could be handled upstream when it doesn't find it to retry?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We haven't seen problems with this, but I agree there's a hole if two orchestrations start very quickly. One use of retrieveByCorrelationId (the only function that calls this one) is in ExecutionLauncher.checkForCorrelatedExecution, called by ExecutionLauncher.start. Same deal for retrievePipelineForCorrelationId below.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it's a concern - I know of some workflows that use orchestration stuff that could get impacted by this, and read-sync issues are the "biggest" headaches of RO queries based on past. This seems to be the biggest spot that's a POTENTIAL risk as I THINK (and still tracing) the others do retries on an operation that throws an error or doesn't respond. It's possible these upstream may ALSO retry, but not 100% sure YET if that exception is encountered...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, reverted back to withPool(poolName)

@kirangodishala kirangodishala force-pushed the use-read-replicas-in-execution-repository branch from 8d9095c to 00de9c0 Compare January 2, 2025 18:15
@dbyron-sf dbyron-sf added the ready to merge Approved and ready for merge label Jan 3, 2025
@mergify mergify bot added the auto merged Merged automatically by a bot label Jan 3, 2025
@mergify mergify bot merged commit 386da27 into spinnaker:master Jan 3, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbyron-sf dbyron-sf dbyron-sf approved these changes

@dzhengg dzhengg dzhengg approved these changes

@christosarvanitis christosarvanitis christosarvanitis approved these changes

@jasonmcintosh jasonmcintosh jasonmcintosh approved these changes

Assignees
No one assigned
Labels
auto merged Merged automatically by a bot ready to merge Approved and ready for merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kirangodishala @jasonmcintosh @christosarvanitis @dzhengg @dbyron-sf