exp/ingest/io: Refactor readers #2644
Conversation
bartekn
changed the title
| return next, nil | ||
| } | ||
|
|
||
| switch r.state { |
There was a problem hiding this comment.
@bartekn would it make sense to put this whole section in its own function to better describe what this is doing? it seems like this function not only "reads" but also if there are no pending, loads tx data and fills pending.
There was a problem hiding this comment.
It would be helpful to document state and how we use it to move across the different kind of changes
There was a problem hiding this comment.
I added a comment inside this method that explains what it's doing.
I can move parts to separate function but since this method actually fits the screen I think it's more readable that way. Let me know what you think.
|
How about |
|
|
||
| // LedgerReader reads ledger header from a given backend and ledger sequence. | ||
| // Use NewLedgerReader to create a new instance. | ||
| type LedgerReader struct { |
There was a problem hiding this comment.
will there be another component that uses LedgerReader besides LedgerTransactionReader? If not, I don't see the point in having a separate type which is just a wrapper around backend.GetLedger(sequence)
Yes, I wanted to remove it completely but we need something like this in |
| @@ -158,7 +158,7 @@ func (s *ProcessorRunner) validateBucketList(ledgerSequence uint32) error { | |||
| return errors.Wrap(err, "Error getting bucket list hash") | |||
| } | |||
|
|
|||
| ledgerReader, err := io.NewDBLedgerReader(s.ctx, ledgerSequence, s.ledgerBackend) | |||
| ledgerReader, err := io.NewLedgerReader(s.ledgerBackend, ledgerSequence) | |||
There was a problem hiding this comment.
I think it would be better to get rid of the LedgerReaderType and instead call ledgerBackend directly like:
exists, ledgerCloseMeta, err := backend.GetLedger(sequence)
if err != nil {
return nil, errors.Wrap(err, "error getting ledger from the backend")
}
if !exists {
return nil, ErrNotFound
}
ledgerBucketHashList := ledgerCloseMeta.LedgerHeader.Header. BucketListHashit's true that the code which calls ledger backend will also be present in LedgerTransactionReader but I don't consider that code duplication. It's ok for backend.GetLedger to be called in multiple places
There was a problem hiding this comment.
I think that there are several advantages of keeping LedgerReader reader:
- It's embedded in
LedgerTransactionReaderandLedgerChangeReader. For example, we callLedgerTransactionReader.GetHeader()in Horizon ingestion processors. - Although it now has just two methods that are rather simple I imagine we will add more helpers and complicated methods in the future. For example:
GetCloseTime()that transformsScpValue.CloseTimetotime.Timeor a verify function that checks if the ledger hash actually matches the contents or even validating bucket hash method (would accept history adapter).
What do you think?
There was a problem hiding this comment.
regarding the first point, LedgerReader is only embedded in LedgerTransactionReader. LedgerChangeReader embeds LedgerTransactionReader. If the functions of LedgerReader become part ofLedgerTransactionReader then LedgerChangeReader will remain unaffected.
regarding the second point, I would rather those utility functions be defined on ledgerCloseMeta. The reason why I believe that's better is that it starts to get confusing when we have to distinguish between LedgerBackend, LedgerTransactionReader, LedgerChangeReader, and LedgerReader.
There was a problem hiding this comment.
but if I'm the only one that finds it confusing then you can ignore my comment and leave it as is
There was a problem hiding this comment.
@tamirms can you take a look?
regarding the second point, I would rather those utility functions be defined on
ledgerCloseMeta. The reason why I believe that's better is that it starts to get confusing when we have to distinguish betweenLedgerBackend,LedgerTransactionReader,LedgerChangeReader, andLedgerReader.
Good point. Not doing this in this PR because this will likely be changed in #2633. I'm also not sure we should expose xdr.LedgerCloseMeta in the public release of ingest package because of #2128.
PR Checklist
PR Structure
otherwise).
services/friendbot, orallordocif the changes are broad or impact manypackages.
Thoroughness
.mdfiles, etc... affected by this change). Take a look in the
docsfolder 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.
LedgerReaderthat getsLedgerCloseMetafrom a provided ledger backend and is embedded by other readers.DBLedgerReadertoLedgerTransactionReaderbecause it works with any ledger backend. It now embedsLedgerReader.LedgerChangeReaderhas been refactored and now embedsLedgerTransactionReader.GetChangesFromLedgerEntryChangesto be a public function.ctx.Contextfrom readers because it's not idiomatic.Part of #2187.