-
Notifications
You must be signed in to change notification settings - Fork 79
Add lookupOpenMiningRoundByNumber #5060
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
base: dfordivam/cip-104-sv-app-rewards-feature-branch
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| -- Copyright (c) 2024 Digital Asset (Switzerland) GmbH and/or its affiliates. All rights reserved. | ||
| -- SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| -- Index on round column for efficient lookups by round number. | ||
| create index scan_rewards_reference_store_active_round | ||
| on scan_rewards_reference_store_active (store_id, migration_id, round) | ||
| where round is not null; | ||
|
|
||
| create index scan_rewards_reference_store_archived_round | ||
| on scan_rewards_reference_store_archived (store_id, migration_id, round) | ||
| where round is not null; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,11 +18,21 @@ import org.lfdecentralizedtrust.splice.scan.store.ScanRewardsReferenceStore | |
| import org.lfdecentralizedtrust.splice.store.{Limit, TcsStore} | ||
| import org.lfdecentralizedtrust.splice.store.db.{ | ||
| AcsArchiveConfig, | ||
| AcsQueries, | ||
| AcsTables, | ||
| DbAppStore, | ||
| DbTcsStore, | ||
| StoreDescriptor, | ||
| } | ||
| import org.lfdecentralizedtrust.splice.util.{ContractWithState, TemplateJsonDecoder} | ||
| import org.lfdecentralizedtrust.splice.store.db.AcsQueries.SelectFromAcsTableResult | ||
| import org.lfdecentralizedtrust.splice.util.{ | ||
| Contract, | ||
| ContractWithState, | ||
| PackageQualifiedName, | ||
| TemplateJsonDecoder, | ||
| } | ||
| import org.lfdecentralizedtrust.splice.util.FutureUnlessShutdownUtil.futureUnlessShutdownToFuture | ||
| import slick.jdbc.canton.ActionBasedSQLInterpolation.Implicits.actionBasedSQLInterpolationCanton | ||
|
|
||
| import scala.concurrent.{ExecutionContext, Future} | ||
|
|
||
|
|
@@ -62,7 +72,9 @@ class DbScanRewardsReferenceStore( | |
| ) | ||
| ), | ||
| ) | ||
| with ScanRewardsReferenceStore { | ||
| with ScanRewardsReferenceStore | ||
| with AcsTables | ||
| with AcsQueries { | ||
|
|
||
| override def waitUntilInitialized: Future[Unit] = multiDomainAcsStore.waitUntilAcsIngested() | ||
|
|
||
|
|
@@ -137,4 +149,47 @@ class DbScanRewardsReferenceStore( | |
| tc: TraceContext | ||
| ): Future[Seq[ContractWithState[OpenMiningRound.ContractId, OpenMiningRound]]] = | ||
| tcsStore.listAllContractsAsOf(OpenMiningRound.COMPANION, asOf) | ||
|
|
||
| override def lookupOpenMiningRoundByNumber( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should at least make use of If doing wait here is not possible, then it may be better to return something other than If
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The scenario would happen when scan has restarted and is catching up, The
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Definitely add IMO it's fine to have ACS stores return data for the current state of the store (i.e., return As for triggers, after As long as you make sure I would still make sure we are not failing tasks just because some store is not caught up. Tasks that fail even after retries produce log warnings, and we alert on those in production. So either have
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed: Thanks for all the feedback here, I've added |
||
| roundNumber: Long | ||
| )(implicit | ||
| tc: TraceContext | ||
| ): Future[Option[Contract[OpenMiningRound.ContractId, OpenMiningRound]]] = | ||
| waitUntilInitialized.flatMap { _ => | ||
| lookupOpenMiningRoundByNumberQuery(roundNumber) | ||
| } | ||
|
|
||
| private def lookupOpenMiningRoundByNumberQuery( | ||
| roundNumber: Long | ||
| )(implicit | ||
| tc: TraceContext | ||
| ): Future[Option[Contract[OpenMiningRound.ContractId, OpenMiningRound]]] = { | ||
| val storeId = multiDomainAcsStore.acsStoreId | ||
| val migrationId = multiDomainAcsStore.domainMigrationId | ||
| val pqn = PackageQualifiedName.fromJavaCodegenCompanion(OpenMiningRound.COMPANION) | ||
| val columns = SelectFromAcsTableResult.sqlColumnsCommaSeparated() | ||
| val query = | ||
| sql"""( | ||
| select #$columns | ||
| from #${ScanRewardsReferenceTables.acsTableName} acs | ||
| where acs.store_id = $storeId | ||
| and acs.migration_id = $migrationId | ||
| and acs.package_name = ${pqn.packageName} | ||
| and acs.template_id_qualified_name = ${pqn.qualifiedName} | ||
| and acs.round = $roundNumber | ||
| ) union all ( | ||
| select #$columns | ||
| from #${ScanRewardsReferenceTables.archiveTableName} acs | ||
| where acs.store_id = $storeId | ||
| and acs.migration_id = $migrationId | ||
| and acs.package_name = ${pqn.packageName} | ||
| and acs.template_id_qualified_name = ${pqn.qualifiedName} | ||
| and acs.round = $roundNumber | ||
| ) limit 1""".as[SelectFromAcsTableResult] | ||
| for { | ||
| result <- futureUnlessShutdownToFuture( | ||
| storage.query(query, "lookupOpenMiningRoundByNumber") | ||
| ) | ||
| } yield result.headOption.map(contractFromRow(OpenMiningRound.COMPANION)(_)) | ||
| } | ||
| } | ||
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'm always nervous when we're creating indexes on non-empty tables in flyway migrations, because migrations are blocking application startup. AFAICT, this table should be sufficiently small that the index creation completes in a second, so it's fine.
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.
Noted, thanks!