-
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
services/horizon/internal/actions: Add latest ingested ledger in experimental ingestion endpoints #1830
services/horizon/internal/actions: Add latest ingested ledger in experimental ingestion endpoints #1830
Conversation
93e6948
to
a8c2b76
Compare
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! I think it's different for /order_book
, the action code should probably overwrite the header to send ledger sequence from in-memory order book graph. Also, please add tests to check if header is sent from DB and order book endpoints.
fc1bed8
to
e9c73cf
Compare
Include a Latest-Ledger header with the sequence number of the last processed ledger by the experimental ingestion system. Also, ensure the endpoint responses contains only data consistent with the sequence number included in the header
e9c73cf
to
1ca39bc
Compare
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!
@@ -60,6 +60,10 @@ func (tx *orderBookBatchedUpdates) apply() error { | |||
} | |||
tx.committed = true | |||
|
|||
if ledger < tx.orderbook.lastLedger { | |||
return errOutdatedLedger | |||
} |
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.
We should probably check: tx.orderbook.lastLedger != 0 && ledger != tx.orderbook.lastLedger + 1
to make sure no ledger is skipped.
GetResource(r *http.Request) (actions.StreamableObjectResponse, error) | ||
GetResource( | ||
r *http.Request, | ||
w actions.HeaderWriter, |
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.
What do you think about flipping the order so it matches ServeHTTP
?
@@ -28,7 +28,7 @@ type Path struct { | |||
// Finder finds paths. | |||
type Finder interface { | |||
// Returns path for a Query of a maximum length `maxLength` | |||
Find(q Query, maxLength uint) ([]Path, error) | |||
Find(q Query, maxLength uint) ([]Path, uint32, error) |
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.
Please update the comment to explain what returned uint32
is.
Include a Latest-Ledger header with the sequence number of the last processed ledger by the
experimental ingestion system. Also, ensure the endpoint responses contains only data consistent
with the sequence number included in the header
PR Checklist
PR Structure
otherwise).
services/friendbot
, orall
ordoc
if the changes are broad or impact manypackages.
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
Add
Latest-Ledger
header with the sequence number of the last processed ledger by the experimental ingestion system. The endpoints built using the experimental ingestion system will always respond with data which is consistent with the ledger inLatest-Ledger
.Fixes #1791
Goal and scope
See #1791 for motivation