-
Notifications
You must be signed in to change notification settings - Fork 59
[ci] pre-calculating total amulet balance #2407
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
Conversation
5666c77 to
7d841c9
Compare
6c5c43d to
05b9c45
Compare
| select greatest( | ||
| 0, | ||
| sum_cumulative_change_to_initial_amount_as_of_round_zero - | ||
| sum_cumulative_change_to_holding_fees_rate * ($asOfEndOfRound + 1) |
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.
Pre-computed sum, so sum_change * ($asOfEndOfRound + 1) instead of sum(change_for_round * ($asOfEndOfRound + 1)): (a+b)*c = ac + bc
05b9c45 to
00c7d69
Compare
|
/cluster_test |
|
Deploy cluster test triggered for Commit 00c7d69b618b5d33b1b943c953b08c5150ea9cd9 in , please contact a Contributor to approve it in CircleCI: https://app.circleci.com/pipelines/github/DACH-NY/canton-network-internal/33947 |
00c7d69 to
a709535
Compare
| @@ -413,8 +413,15 @@ class HttpScanHandler( | |||
| HttpErrorHandler.onGrpcNotFound(s"Data for round ${asOfEndOfRound} not yet computed") | |||
| ) | |||
| } yield { | |||
| definitions.GetTotalAmuletBalanceResponse( | |||
| Codec.encode(total) | |||
| total.fold( | |||
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.
Changed to respond with NotFound for rounds that were not computed. The change in this PR calculates these going forward. Rather 'error-out' with NotFound than fallback to a very slow query. If we get a signal on this from logs that there is a lot of requests for older rounds, we can put some effort in computing all rounds in the round_total_amulet_balance table, but hopefully that will not be needed.
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.
(the UI already uses getInstrument and this only uses getTotalAmuletBalance if holdingfees are deducted.)
d1571cf to
c076088
Compare
to improve performance of v0/wallet-balance, v0/total-amulet-balance, /v0/top-providers-by-app-rewards Signed-off-by: Raymond Roestenburg <raymond.roestenburg@digitalasset.com>
Signed-off-by: Raymond Roestenburg <raymond.roestenburg@digitalasset.com>
Signed-off-by: Raymond Roestenburg <raymond.roestenburg@digitalasset.com>
Signed-off-by: Raymond Roestenburg <raymond.roestenburg@digitalasset.com>
Signed-off-by: Raymond Roestenburg <raymond.roestenburg@digitalasset.com>
…work wel for large number of parties. Signed-off-by: Raymond Roestenburg <raymond.roestenburg@digitalasset.com>
Signed-off-by: Raymond Roestenburg <raymond.roestenburg@digitalasset.com>
Signed-off-by: Raymond Roestenburg <raymond.roestenburg@digitalasset.com>
In the case that the efficient total amulet balance has not been computed for an older round. Signed-off-by: Raymond Roestenburg <raymond.roestenburg@digitalasset.com>
Signed-off-by: Raymond Roestenburg <raymond.roestenburg@digitalasset.com>
c076088 to
f8748be
Compare
|
/cluster_test |
|
Deploy cluster test triggered for Commit f8748be6b86def950b4c79feaa41f9b0ff6a4482 in , please contact a Contributor to approve it in CircleCI: https://app.circleci.com/pipelines/github/DACH-NY/canton-network-internal/34047 |
|
Before I review:
|
|
|
@meiersi-da |
@OriolMunoz-da : aren't these the "as-of-round zero" endpoints? For these not deducting holding fees is not easily possible, and we said we'll leave them with their current behavior as legacy features. |
OriolMunoz-da
left a comment
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 think I'm mostly coming-back-from-vacation-confused 😄
| sql""" | ||
| select greatest( | ||
| 0, | ||
| coalesce( |
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.
what are you coalescing here?
both sum_cumulative* columns are not null, and you're not left-joining
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.
ah thanks, cumulative_* in round_party_totals are null, probably misremembered. (it can't really hurt but will remove)
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.
done.
apps/scan/src/main/scala/org/lfdecentralizedtrust/splice/scan/store/db/DbScanStore.scala
Outdated
Show resolved
Hide resolved
| on conflict (store_id, closed_round) | ||
| do update set sum_cumulative_change_to_initial_amount_as_of_round_zero = excluded.sum_cumulative_change_to_initial_amount_as_of_round_zero, | ||
| sum_cumulative_change_to_holding_fees_rate = excluded.sum_cumulative_change_to_holding_fees_rate; |
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 don't understand when a conflict would happen and why. Why is it safe to replace the amounts?
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.
Unlikely a conflict occurs, but all of this has to be idempotent because of 'reasons outside of my preference ;-)', so just safer, don't want to fail. it's on conflict pk so it would have to mean a previous aggregation already completed.
Since the aggregation calculates this from round_party_totals and most recent active_parties that is created by this aggregation (and all of this is transactional), it's safe to replace the amounts (it would be the same result).
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.
it would be the same result
then you could as well ignore it, right? I was mostly surprised because some lines above it's ignored. No strong feelings one way or the other
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.
ah, right, yes could just do on conflict do nothing, let me do that instead for clarity
| create temp table active_parties_for_aggr_rounds on commit drop as | ||
| select party, aggr_round as aggr_round, active_round from active_parties_before | ||
| union | ||
| -- all the parties that were active in the newly aggregated rounds | ||
| select party, round as aggr_round, round as active_round from temp_cumulative_totals | ||
| union | ||
| -- adding the lastClosedRound as aggr_round, for parties that were not active but have been aggregated in that round | ||
| select party, | ||
| $lastClosedRound as aggr_round, | ||
| closed_round as active_round | ||
| from active_parties | ||
| where store_id = $roundTotalsStoreId; |
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.
open and close parentheses on commit drop as (...), this got very confusing when reviewing
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.
Ok I'll add those
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.
done.
| sqlu""" | ||
| create temp table active_parties_before on commit drop as | ||
| select party, | ||
| (select max(closed_round) from active_parties where store_id = $roundTotalsStoreId) as aggr_round, |
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.
this doesn't depend on the rest of the query, have you checked that the query plan doesn't run this for every party?
it might still be better to extract it
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.
why for every party? This happens per aggregation, which is not per party. It aggregates everything for all rounds it finds.
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.
oh sorry you mean the sub select.. let me check to be safe
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.
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.
Another alternative is:
with max_closed as (select max(closed_round) as aggr_round from scan_sv_1.active_parties where store_id = 1)
select party,
max_closed.aggr_round,
closed_round as active_round
from scan_sv_1.active_parties,
max_closed
where store_id = 1
but this has the same explain plan, rather keep it simple with scalar sub-query.
Signed-off-by: Raymond Roestenburg <raymond.roestenburg@digitalasset.com>
Signed-off-by: Raymond Roestenburg <raymond.roestenburg@digitalasset.com>
Signed-off-by: Raymond Roestenburg <raymond.roestenburg@digitalasset.com>
Signed-off-by: Raymond Roestenburg <raymond.roestenburg@digitalasset.com>
|
Have another look please @OriolMunoz-da |
OriolMunoz-da
left a comment
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.
thanks! let me know if the sub-select does something crazy, otherwise lgtm
Signed-off-by: Raymond Roestenburg <raymond.roestenburg@digitalasset.com>

Calculating total amulet balance for rounds during aggregation to improve performance of queries in
v0/total-amulet-balance.Note: Only implemented new rounds onward, which I think is enough. (otherwise will have to run a catch up process to add old rounds from round_party_totals table, if we really want total amulet balances by round for all historical rounds).
Responding with
NotFoundfor total amulet balances that have not been calculated.Pull Request Checklist
Cluster Testing
/cluster_teston this PR to request it, and ping someone with access to the DA-internal system to approve it./hdm_teston this PR to request it, and ping someone with access to the DA-internal system to approve it.PR Guidelines
Fixes #n, and mention issues worked on using#nMerge Guidelines