-
Notifications
You must be signed in to change notification settings - Fork 499
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
exp/ingest: ledger backend support #1404
exp/ingest: ledger backend support #1404
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.
Looks good. There are a few things to change but in general it goes in the right direction. I answered your questions in code comments. Let me know if I missed anything.
var _ LedgerBackend = (*DatabaseBackend)(nil) | ||
|
||
// DatabaseBackend implements a database data store. | ||
type DatabaseBackend struct { |
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 open to suggestions for how best to do that, or if there's another approach to testing that would be better.
I think we should just mock db.Session
:
- Create a private interface in
ledgerbackend
package with methods we use (SelectRaw
,GetRaw
...). - Create a mock.
- Then in tests program a mock to set the results to what you need using
Call.Run
.
However, I think we need at least a few tests that work against a running database to ensure queries are correct etc. These can be run in CI only.
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 is a great idea. It's not possible at present because we access session.DB.Close()
in DatabaseBackend.Close()
. Since DB
is a struct field, not a method, it can't be accessed through an interface.
One solution would be to add a new method session.Close()
to the db
package. This could delegate to the underlying *sqlx.DB.Close()
for the real thing, and be a no-op for the mock. What do you think?
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.
Yeah, let's do it.
@bartekn I've implemented all requested changes except the three unresolved issues above:
Please take a look and let me know your preferences and I'll finish this up. |
@bartekn remaining changes have been made - PTAL. I can work on adding more detailed unit and CI tests now, but I think I'd prefer to merge this and add them in a new PR, so the code is generally available (it's still in |
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.
Last issues. The one in DBLedgerReadCloser.GetHeader()
is important so will approve once it's fixed.
… into es-1285-ledger_backend
If you're making a doc PR or something tiny where the below is irrelevant, just delete this
template and use a short description.
PR Structure
otherwise).
services/friendbot
Thoroughness
.md
files, etc... affected by this change). Take a look in the
docs
folder for a given service,like this one.
Release planning
needed with deprecations, added features, breaking changes, and DB schema changes.
semver, or if it's mainly a patch change. The PR is targeted at the next
release branch if it's not a patch change.
Summary
Goal and scope
This PR adds support for reading transaction information and headers of individual ledgers from a stellar-core database. This work is part of the ingest project, and is intended to allow a data pipeline to poll for current ledger information for a detailed and current view of the ledger state.
Summary of changes
Following the design described in the ingestion plan, this PR provides
io.LedgerReadCloser
-> implemented byio.DBLedgerReadCloser
ledgerbackend.LedgerBackend
-> implemented byledgerbackend.DatabaseBackend
ingestadapters.LedgerBackendAdapter
LedgerTransaction
andLedgerCloseMeta
)A simple demo running against a local stellar-core docker container is included (
ingest/cmd/main
).Known limitations & issues
I ran into a number of questions during the implementation and would appreciate any feedback.
Close
methods to allow the DB session to be cleared up. I also added anInit
method for theLedgerReadCloser
, to allow the internal data to be loaded in advance of the firstRead()
call. Does this make sense?LedgerBackendAdapter
should conform to?support/db
.What shouldn't be reviewed
Should all be reviewed.