exp/lighthorizon/services: Fetch ledgers in parallel to them being processed. #4500
Conversation
Shaptic
changed the title
Shaptic
changed the title
Shaptic
force-pushed
the
lighthorizon_parallelFetch
branch
from
44b71f7 to
b779bf7
Compare
| On("GetLedger", mock.Anything, uint32(1586112)).Return(expectedLedger1, nil). | ||
| On("GetLedger", mock.Anything, uint32(1586113)).Return(expectedLedger2, nil). | ||
| On("GetLedger", mock.Anything, uint32(1586114)).Return(expectedLedger3, nil). | ||
| On("GetLedger", mock.Anything, mock.Anything).Return(xdr.LedgerCloseMeta{}, nil) |
There was a problem hiding this comment.
Does anyone know how to do a "any context.Context instance" equivalent here? I tried mock.AnythingOfType but that wasn't it.
There was a problem hiding this comment.
what was the syntax, looks like mock.AnythingOfType("context.Context") shoulda worked?
| @@ -10,6 +10,7 @@ import ( | |||
| type CursorManager interface { | |||
| Begin(cursor int64) (int64, error) | |||
| Advance() (int64, error) | |||
| Skip(count uint) (int64, error) | |||
There was a problem hiding this comment.
not suggestion change, just a question on interface ergo, do you think Advance(skip uint) would be less moving parts for same new functionality?
There was a problem hiding this comment.
interesting idea, I think it's pretty ergo. seeing Advance(0) becomes a little unintuitive, but I'll try it out
There was a problem hiding this comment.
ah, subtle, rather than skip semantics, it could be Advance(increment uint) so Advance(0) would be no-op ?
| // returned group have completed. In contrast, this function closes the output | ||
| // channel when all work has been submitted (or the context errors). | ||
| // | ||
| // FIXME: Should this be a part of archive.Archive? |
There was a problem hiding this comment.
+1, to refactor this ledger loading optimization to be encapsulated in Archive instead, i.e. this caller method would largely be unchanged, still uses Archive.GetLedger(ctx, nextLedger), the new perf optimizations for loading get relo'd within there ?
There was a problem hiding this comment.
I started working through refactoring this and realized that we actually probably don't need most of the Archive interface anymore after Bartek's work in #4488 which gives us a clean MetaArchive abstraction, which I think was a big point of what we were trying to accomplish with Archive.
I'll try to throw up a PR tomorrow to clean that up.
There was a problem hiding this comment.
looks really good, existing tests exercise this right? I think is worthwhile to follow your FIXME intuition now rather than wait, and refactor the logic into Archive.GetLedger , it may incur different unit test coverage..I think tests on this service mocked out Archive.
Shaptic
force-pushed
the
lighthorizon_parallelFetch
branch
from
b779bf7 to
b733b48
Compare
|
Abandoning in lieu of two separate PRs related to the comment about hiding this behind |
What
This modifies the ledger download+processing code to download subsets of the entire checkpoint range in parallel. Preliminary testing suggests that this can massively reduce request latency.
Before:
After:
(Note that the indices are local and no on-disk cache was used.)
Why
We can reduce latency by doing ledger downloads in parallel to processing. See #4468.
Known limitations
Obviously, neither of these represent acceptable levels of latency if we're talking about parity with classic Horizon. However, it's still a massive improvement over the previous code if we decide that high latency is acceptable for deeply historical requests.