CORE-133: Optimize SQL in billing account change synchronizer #3145
Conversation
davidangb
requested review from
samanehsan and
trholdridge
and removed request for
a team
| @@ -540,6 +541,7 @@ trait RawlsBillingProjectComponent { | |||
| ) | |||
| } | |||
|
|
|||
| @VisibleForTesting | |||
| def getLastChange(billingProject: RawlsBillingProjectName): ReadAction[Option[BillingAccountChange]] = | |||
| BillingAccountChanges | |||
| .withProjectName(billingProject) | |||
There was a problem hiding this comment.
this is now only used in tests, so I annotated it
| .filter(_.id.in(latestChangeIds)) | ||
| .sortBy(_.id.asc) | ||
| } | ||
|
|
There was a problem hiding this comment.
this is the old unoptimized query, now unused.
| @@ -47,7 +46,7 @@ object BillingAccountChangeSynchronizer { | |||
| Behaviors.setup { context => | |||
| val actor = BillingAccountChangeSynchronizer(dataSource, gcsDAO, samDAO) | |||
| Behaviors.withTimers { scheduler => | |||
| scheduler.startTimerAtFixedRate(UpdateBillingAccounts, initialDelay, pollInterval) | |||
| scheduler.startTimerWithFixedDelay(UpdateBillingAccounts, initialDelay, pollInterval) | |||
There was a problem hiding this comment.
see https://doc.akka.io/japi/akka/snapshot/akka/actor/TimerScheduler.html for detailed explanation of the difference
| BillingAccountChanges.latestChanges.unsynced | ||
| .take(1) | ||
| .result | ||
| BillingAccountChanges.nextOutstanding().result |
There was a problem hiding this comment.
this is the core of the PR - changing from the old query to the new one
| _ <- validateStatusColumn.recover { case e: Exception => | ||
| logger.warn(s"BillingAccountChangeStatus logic failed with ${e.getMessage}", e) | ||
| } | ||
|
|
There was a problem hiding this comment.
we no longer need to validate the old query and new query return the same results, since we're getting rid of the old query
| } yield changes shouldBe List(change1, change2).map(_.value) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
the old query is gone, so this test can go too
@trholdridge imagine a task that runs for 7 seconds. If that task is scheduled via However, if that task is scheduled via In this PR's use case of polling for billing account changes, we aren't worried about delays of a few seconds; we'll get to the change when we get to it. So, let's choose the scheduling approach that is lighter on the system. If we had strict latency requirements we might choose otherwise. |
Ticket: https://broadworkbench.atlassian.net/browse/CORE-133
Followup to #3139. In that other PR, we added a new
STATUScolumn to theBILLING_ACCOUNT_CHANGEStable, with logic to populate that column's values. We also added validation into theBillingAccountChangeSynchronizerwhich compares the polling query it currently uses to a prospective new query using theSTATUScolumn. After running for a week or so, we verified that the business logic aroundSTATUSis correct.In this PR, we switch the
BillingAccountChangeSynchronizeraway from the current expensive polling query to the new optimized query using theSTATUScolumn.As a bonus, this PR also changes the
BillingAccountChangeSynchronizerfrom running at a fixed rate via startTimerAtFixedRate to running at a fixed delay via startTimerWithFixedDelay. Instead of running every N seconds, we'll now wait N seconds between runs. The difference is that the former does not take into account how long the task runs, while the latter does.PR checklist
model/, then you should publish a new officialrawls-modeland updaterawls-modelin Orchestration's dependencies.