-
Notifications
You must be signed in to change notification settings - Fork 59
support querying next snapshot after a timestamp #3552
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
Signed-off-by: Itai Segall <[email protected]>
Signed-off-by: Itai Segall <[email protected]>
Signed-off-by: Itai Segall <[email protected]>
Signed-off-by: Itai Segall <[email protected]>
Signed-off-by: Itai Segall <[email protected]>
Signed-off-by: Itai Segall <[email protected]>
Signed-off-by: Itai Segall <[email protected]>
Signed-off-by: Itai Segall <[email protected]>
Signed-off-by: Itai Segall <[email protected]>
Signed-off-by: Itai Segall <[email protected]>
Signed-off-by: Itai Segall <[email protected]>
Signed-off-by: Itai Segall <[email protected]>
Signed-off-by: Itai Segall <[email protected]>
Signed-off-by: Itai Segall <[email protected]>
Signed-off-by: Itai Segall <[email protected]>
Signed-off-by: Itai Segall <[email protected]>
Signed-off-by: Itai Segall <[email protected]>
Signed-off-by: Itai Segall <[email protected]>
Signed-off-by: Itai Segall <[email protected]>
rautenrieth-da
left a comment
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.
Will be useful for streaming ACS snapshots to S3
Code looks fine, but why are we adding a new HTTP endpoint? Is the process that uploads to S3 going to implemented outside of scan? If it will live inside scan, then we don't need a new endpoint, no?
| type: integer | ||
| format: int64 | ||
| description: | | ||
| The endpoint will return the record time of the first snapshot for this migration id or larger. |
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 different from the /v0/state/acs/snapshot-timestamp endpoint, which only looks at snapshots from exactly the given migration id.
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.
TBH, I think that the semantics in the existing one is less useful. People don't really care about migration IDs, it's almost an implementation detail. They just want the previous/next snapshot from a specific timestamp.
| and history_id = $historyId """ ++ orderLimit | ||
|
|
||
| val query = | ||
| sql"select * from ((" ++ sameMig ++ sql") union all (" ++ largerMig ++ sql")) all_queries order by snapshot_record_time asc limit 1" |
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.
A bit complicated for a table with only thousands of rows, but should make optimal use of the available primary key index.
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.
yup, that was my thinking exactly
Mostly so that I can call it in the integration test TBH. |
Signed-off-by: Itai Segall <[email protected]>
@rautenrieth-da @ray-roestenburg-da I'm going to merge this as-is. Please shout if you think that having this endpoint is a mistake, we have until the next release to remove it easily if so. |
Will be useful for streaming ACS snapshots to S3
Pull Request Checklist
Cluster Testing
/cluster_teston this PR to request it, and ping someone with access to the DA-internal system to approve it./hdm_teston this PR to request it, and ping someone with access to the DA-internal system to approve it.PR Guidelines
Fixes #n, and mention issues worked on using#nMerge Guidelines