-
Notifications
You must be signed in to change notification settings - Fork 1
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
ingest: service created #14
Conversation
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.
Thank you for the hard work! I've left some comments.
.vscode | ||
captive-core*/ |
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 this name correct?
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.
Yes, whenever you run captive core it creates a folder named like captive-core-9f8g7a9f
in the application's root folder to store the ledger files it downloads. The "*" is to capture the random name given to the folder.
cmd/ingest.go
Outdated
Usage: "Path to Captive Core's binary file.", | ||
OptType: types.String, | ||
ConfigKey: &cfg.CaptiveCoreBinPath, | ||
FlagDefault: "/usr/local/bin/stellar-core", |
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 wonder if this one couldn't also point to a local directory like the captive-core-config-dir
.
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.
Whenever you install captive core locally using these instructions, /usr/local/bin/stellar-core
will be the default install location. If we wanted it to live inside a project directory we would need to change that install path.
I thought it was easier to use the default location and set it here accordingly. What do you think?
cmd/ingest.go
Outdated
Name: "captive-core-bin-path", | ||
Usage: "Path to Captive Core's binary file.", | ||
OptType: types.String, | ||
ConfigKey: &cfg.CaptiveCoreBinPath, |
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 was thinking about adding a CustomSetValue
for it since it's a folder, we could add validations to see if the folder exists before the application starts.
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.
Good one! c81a985
cmd/ingest.go
Outdated
Name: "captive-core-config-dir", | ||
Usage: "Path to Captive Core's configuration files directory.", | ||
OptType: types.String, | ||
ConfigKey: &cfg.CaptiveCoreConfigDir, |
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.
Same here (custom set value for folders value).
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.
Good one! c81a985
cmd/ingest.go
Outdated
}, | ||
{ | ||
Name: "ledger-cursor-name", | ||
Usage: "Name of last synced ledger cursor. Attention: there should never be more than one container running with a same cursor name.", |
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.
Could you elaborate on it? Is this value an enum or should it be a unique value? We should make it more clear and, also remove the FlagDefault
since it cannot have the same name.
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's not an enum, it's just an unique string value used as the key in the ingest_store
dictionary-like table.
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've made the description more clear, made it required and removed the default value. Good call, this will make people more aware that this has to be carefully configured when running parallel ingestion.
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.
operation_id bigint NOT NULL, | ||
operation_type text NOT NULL, | ||
transaction_id bigint NOT NULL, | ||
transaction_hash text NOT NULL, |
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.
same here
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.
LGTM 🚀
What
Implements the ingestion service in the wallet backend.
Why
New feature.
Known limitations
Dockerfile still needs to be setup.
Issue that this PR addresses
[OSB Ingestion] Move service over to new repo
Checklist
PR Structure
all
if the changes are broad or impact many packages.Thoroughness
Release