-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat: add total accounts stats #1960
base: master
Are you sure you want to change the base?
Conversation
if v1 < v2, do: v1, else: v2 | ||
end) | ||
end) | ||
|> Enum.reduce({State.new(TempStore.new()), []}, fn {pubkey, time}, {state, mutations} -> |
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 do we need a new type of store to store values and then extract these values to create mutations? Can we instead move all of this logic to this migration and avoid using state with a new type of store?
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.
We can, but this way we are reusing the logic, that would generate those entries outside of the migration (that way there is less chance for the migration to generate different entries than the ones that would be generated during normal work).
This type of store could come in handy in the future, as many functionalities take the state as a parameter and that could simplify introduction of new tables and make it less error-prone (because that way we could write the logic for generating entries once, instead of writing one way of generating them for the migration, and another one for the regular syncing).
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.
Is all we're reusing the increment_statistics
function? I'd rather copy and paste that function here than define a new store in which we're going to define new mutations execute them to build another set of mutations. It all sounds too complex for what we need to do here. I think it can be simplified a lot TBH
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 am not executing the mutations to generate another set of mutations. Basically the accounts creation list is big, so I am trying to generate the mutations for AccountCreation
and meanwhile calculate in memory the statistics for :total_accounts
(so that I don't update them constantly, which would generate a lot of writes to the database) in a single pass. In the next step I am adding the final entries for the statistics (as mutations) to the already generated list of mutations (so that there is a single list of mutations).
I don't mind changing but, but just to clarify, what do you suggest:
- Copying the
increment_statistics
function in the migration? (I tried to escape that, because that would mean many writes in the database, which is why I store them in memory to have a single write for each index) - Copying the
increment_statistics
function, but using a map or something to increment the count for each index in memory and then produce the mutations (which is what I tried to extract as logic so that we can reuse the existing logic and maybe reuse this tempstore for similar future endeavors)?
1f96e45
to
830910b
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.
defmodule AeMdw.Migrations.AddAccountCreationTable do
@moduledoc """
Add account creation table and update account creation statistics.
"""
alias AeMdw.Collection
alias AeMdw.Db.State
alias AeMdw.Db.Model
alias AeMdw.Db.WriteMutation
alias AeMdw.Db.DeleteKeysMutation
alias AeMdw.Db.StatisticsMutation
alias AeMdw.Db.Sync.Stats, as: SyncStats
alias AeMdw.Db.RocksDbCF
alias AeMdw.Sync.Transaction
require Model
@spec run(State.t(), boolean()) :: {:ok, non_neg_integer()}
def run(state, _from_start?) do
keys_to_delete = state |> Collection.stream(Model.AccountCreation, nil) |> Enum.to_list()
clear_mutation = DeleteKeysMutation.new(%{Model.AccountCreation => keys_to_delete})
state = State.commit_db(state, [clear_mutation])
protocol_accounts =
for {protocol, height} <- :aec_hard_forks.protocols(),
protocol <= :aec_hard_forks.protocol_vsn(:lima),
{account, _balance} <- :aec_fork_block_settings.accounts(protocol),
into: %{} do
{account, height}
end
Model.Tx
|> RocksDbCF.stream()
|> Task.async_stream(fn Model.tx(id: tx_hash, time: time) ->
tx_hash
|> :aec_db.get_signed_tx()
|> Transaction.get_ids_from_tx()
|> Enum.reduce(%{}, fn
{:id, :account, pubkey}, acc ->
Map.put_new(acc, pubkey, time)
_other, acc ->
acc
end)
end)
|> Enum.reduce(protocol_accounts, fn {:ok, new_map}, acc_times ->
Map.merge(acc_times, new_map, fn _k, v1, v2 ->
min(v1, v2)
end)
end)
|> Enum.reduce({%{}, []}, fn {pubkey, time}, {statistics, mutations} ->
new_statistics =
time
|> SyncStats.time_intervals()
|> Enum.map(fn {interval_by, interval_start} ->
{:total_accounts, interval_by, interval_start}
end)
|> Enum.reduce(statistics, fn key, statistics ->
Map.update(statistics, 1, &(&1 + 1))
end)
{new_statistics,
[
WriteMutation.new(
Model.AccountCreation,
Model.account_creation(index: pubkey, creation_time: time)
)
| mutations
]}
end)
|> then(fn {statistics, mutations} ->
Stream.concat(mutations, [StatisticsMutation.new(statistics)])
end)
|> Stream.chunk_every(1000)
|> Enum.reduce({state, 0}, fn mutations, {acc_state, count} ->
{
State.commit_db(acc_state, mutations),
count + length(mutations)
}
end)
|> then(fn {_state, count} -> {:ok, count} end)
end
end
Not tested, but no need for TempStore here
830910b
to
28e388e
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.
Can we get rid of TempStore? Otherwise LGTM
refs: #1893