-
Notifications
You must be signed in to change notification settings - Fork 139
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
backend: remove postgres, use redis for api/ledgers data #209
Conversation
Preview deployed to development environment: https://dashboard.prototypes.kube001.services.stellar-ops.com |
Preview deployed to development environment: https://dashboard.prototypes.kube001.services.stellar-ops.com |
Preview deployed to development environment: https://dashboard.prototypes.kube001.services.stellar-ops.com |
Preview deployed to development environment: https://dashboard.prototypes.kube001.services.stellar-ops.com |
Preview deployed to development environment: https://dashboard.prototypes.kube001.services.stellar-ops.com |
Preview deployed to development environment: https://dashboard.prototypes.kube001.services.stellar-ops.com |
Preview deployed to development environment: https://dashboard.prototypes.kube001.services.stellar-ops.com |
Preview deployed to development environment: https://dashboard.prototypes.kube001.services.stellar-ops.com |
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 have a few questions (❔❓), I think driven by just lack of knowledge on my part on how this works. These questions don't need to block anything, except maybe the question (❓) on how ledger data is being added together, I didn't expect that.
backend/ledgers.ts
Outdated
cachedLedgers.splice(index, 1, { | ||
date: date, | ||
transaction_count: | ||
cachedLedgers[index].transaction_count + | ||
ledger.successful_transaction_count + | ||
ledger.failed_transaction_count, | ||
operation_count: | ||
cachedLedgers[index].operation_count + ledger.operation_count, |
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 add to a ledger we already have? I would have expected once we know about a ledger's data that we wouldn't mutate it after then because the ledger shouldn't change.
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.
maybe the naming should be changed. The cache is stored like so:
[
{
"date": "01-13",
"transaction_count": 5245946,
"operation_count": 10088417
},
{
"date": "01-12",
"transaction_count": 5787617,
"operation_count": 11333624
},
...
which is what cachedLedgers
is referring to and is what's being updated. Which technically isn't a ledger, I couldn't think of a better word 🤔 . This code no longer stores every LedgerRecord
from horizon like it did before with postgres.
I think maybe this would be clearer when LedgerRecord is not any
anymore
Lines 13 to 14 in ccc4c75
// TODO - use any until https://github.com/stellar/js-stellar-sdk/issues/731 resolved | |
export type LedgerRecord = any; |
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.
LedgerStat
? this was referring to something else with postgres before I removed it, but I think could be fitting here. LedgerAggregate
might be better, but a mouth full. Thoughts on either of 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.
I like LedgerStat
the more I think of it: e6fb860
if you disagree lmk though
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.
My comments are more like eslint
stuff. 😊 I'm not familiar with redis or postgress, so I would rely on a backend dev to check that.
Preview deployed to development environment: https://dashboard.prototypes.kube001.services.stellar-ops.com |
Preview deployed to development environment: https://dashboard.prototypes.kube001.services.stellar-ops.com |
Preview deployed to development environment: https://dashboard.prototypes.kube001.services.stellar-ops.com |
1 similar comment
Preview deployed to development environment: https://dashboard.prototypes.kube001.services.stellar-ops.com |
Preview deployed to development environment: https://dashboard.prototypes.kube001.services.stellar-ops.com |
Preview deployed to development environment: https://dashboard.prototypes.kube001.services.stellar-ops.com |
1 similar comment
Preview deployed to development environment: https://dashboard.prototypes.kube001.services.stellar-ops.com |
WHAT
remove use of postgres on the backend, and use redis for the
api/ledgers/public
dataWHY
The dashboard api endpoints essentially cache organized data given from Horizon, which is more of a use case for redis than postgres. And since postgres was just being used for a single purpose (storing all ledgers), we can now remove it and have a single storage option on the backend, reducing the complexity of the backend. In order to do this the data is now organized before being stored in the cache, as opposed to after like it was with postgres.
closes #198
part of the backend refactor plan