-
Notifications
You must be signed in to change notification settings - Fork 238
Support two-way traversal in blob feed API #1286
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
Conversation
disperser/dataapi/v2/blobs.go
Outdated
c.JSON(http.StatusOK, response) | ||
|
||
// TODO(jianxiao): this is just a placeholder as GetBlobMetadataByRequested is doing forward retrieval | ||
blobs, nextCursor, err := s.blobMetadataStore.GetBlobMetadataByRequestedAt( |
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 one is TBD in followup
disperser/dataapi/v2/server_v2.go
Outdated
@@ -263,7 +263,8 @@ func (s *ServerV2) Start() error { | |||
{ | |||
blobs := v2.Group("/blobs") | |||
{ | |||
blobs.GET("/feed", s.FetchBlobFeed) | |||
blobs.GET("/feed/forward", s.FetchBlobFeedForward) |
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.
Didn't take a deep look into logic but is this simpler than just adding a query param to determine the direction?
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 looks more clear to have separate APIs, but am open to merge them if these params can have clear semantics when they are used for both directions.
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.
(mostly parameters have to remain clear in semantics in both contexts, e.g. start
and end
makes sense in forward but not in backward any more)
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.
thinking it more, maybe using before
and after
naturally addresses this, like
- direction: string // "forward" or "backward"
- after: string // exclusive lower bound
- before: string // exclusive upper bound
- cursor: string // if present, must be within (after, before)
- limit: int // max items to return
For backward: fetches (after, cursor) in DESC order
For forward: fetches (cursor, before) in ASC order
ping? |
Why are these changes needed?
To make it easy and reliable to traverse the feed backward and forward in time
This PR changes the APIs to support this.
Note: the existing API is forward iteration, so
forward
mode works here already; butbackward
will need a change in DB layer (in a followup later)Checks