vector-store: expose index build progress in status endpoint#506
vector-store: expose index build progress in status endpoint#506nuivall wants to merge 2 commits into
Conversation
|
@DarioMirovic could you pre-check if this rust code makes sense? |
| ), | ||
| // version should be updated manually when there are changes in API | ||
| version = "2.0.0" | ||
| version = "2.1.0" |
There was a problem hiding this comment.
nit: this change should be in first commit. Along the actual API change.
| "type": "number", | ||
| "format": "double", | ||
| "description": "Progress of the initial full table scan that backfills the index,\nexpressed as a percentage in the range `0.0..=100.0`. It is `100.0`\nonce the index is `SERVING`. Defaults to `0.0` for backward\ncompatibility with older clients that omit the field." | ||
| }, |
There was a problem hiding this comment.
nit: This file (auto-generated) should be in fist commit alongside with the actual API change.
| /// Progress of the initial full table scan that backfills the index, | ||
| /// expressed as a percentage in the range `0.0..=100.0`. It is `100.0` | ||
| /// once the index is `SERVING`. Defaults to `0.0` for backward | ||
| /// compatibility with older clients that omit the field. |
There was a problem hiding this comment.
"Defaults to 0.0 for backward compatibility with older clients that omit the field." - I think "defaults to 0.0" is anyhow related with backward compatibility. I mean that this is backward compatible change regardless if the default value.
There was a problem hiding this comment.
I'd say that we need default value of 100.0.
The caller using the new client would be checking both status and build_progress.
New client + old server = use default value.
Current code: the caller never progresses, because build_progress hasn't reached 100%.
100.0 default: the caller effectively just waits for status, as do the current callers that don't have build_status exposed - they only look at status.
There was a problem hiding this comment.
Currently build_progress is only information about the progress of fullscan - it shouldn't be taken as a source of truth for the index state. The valid information is the state of the index - if this is SERVING it means the index is serving regardless of build_progress. If build_progress doesn't reach 100% it might be a trigger for investigation, what happened.
| "count" | ||
| ], | ||
| "properties": { | ||
| "build_progress": { |
There was a problem hiding this comment.
nit: could specify:
minimum: 0
maximum: 100
| #[tokio::test] | ||
| #[ntest::timeout(10_000)] | ||
| #[cfg_attr(not(feature = "slow-test-hooks"), ignore = "requires slow-test-hooks")] | ||
| async fn index_status_reports_build_progress_while_bootstrapping() { |
There was a problem hiding this comment.
These tests are in the wrong file. routing.rs is meant for search query routing to the correct index. We could move them to status.rs or create a new index_status.rs file, as status.rs currently only covers node status.
| /// Progress of the initial full table scan that backfills the index, | ||
| /// expressed as a percentage in the range `0.0..=100.0`. It is `100.0` | ||
| /// once the index is `SERVING`. Defaults to `0.0` for backward | ||
| /// compatibility with older clients that omit the field. |
There was a problem hiding this comment.
I'd say that we need default value of 100.0.
The caller using the new client would be checking both status and build_progress.
New client + old server = use default value.
Current code: the caller never progresses, because build_progress hasn't reached 100%.
100.0 default: the caller effectively just waits for status, as do the current callers that don't have build_status exposed - they only look at status.
| /// expressed as a percentage in the range `0.0..=100.0`. It is `100.0` | ||
| /// once the index is `SERVING`. Defaults to `0.0` for backward |
There was a problem hiding this comment.
In initial_scan, if at least one preform_range_scan call fails, it drops permit, as it should, but never increments completed_scan_length for that range. The counter can never reach u64::MAX, so we'll never reach 100% build progress, even when we transition to the serving state:
impl From<u64> for Percentage {
fn from(value: u64) -> Self {
Percentage::try_from((value as f64 / u64::MAX as f64) * 100.0).unwrap()
}
}
impl From<u64> for Progress {
fn from(value: u64) -> Self {
if value == u64::MAX {
Progress::Done
} else {
Progress::InProgress(Percentage::from(value))
}
}
}This is a pre-existing issue. The focus of this comment is compliance of the doc comment with the actual behavior.
There was a problem hiding this comment.
You're right, currently we are skipping errors without increasing fullscan progress. There is an issue to design better handling of errors during fullscan and cdc: https://scylladb.atlassian.net/browse/VECTOR-726
|
v2:
|
Add a build_progress field to IndexStatusResponse so clients can observe the initial full-scan backfill progress. Defaults to 100.0 when omitted, so a new client against an older server relies solely on the authoritative status field. Schema is bounded 0..=100. Includes the regenerated api/openapi.json and API version bump (2.1.0). The handler uses a placeholder 100.0 for now; real progress is wired up next.
The status handler now reports the real build_progress from the index entry's full-scan progress (Progress::Done maps to 100%), replacing the placeholder introduced with the API type. Add integration tests in a dedicated index_status.rs module (routing.rs is about query routing) covering the BOOTSTRAPPING stage with partial progress and a serving index at 100%. Shared routing.rs helpers are made pub(crate) for reuse.
4 participants
The GET /api/v1/indexes/{keyspace}/{index}/status handler now reports the
build_progress percentage taken from the index entry's full-scan progress
(Progress::Done maps to 100%). This lets clients observe how far the
initial backfill scan has advanced while an index is BOOTSTRAPPING.
Refs: https://scylladb.atlassian.net/browse/SCYLLADB-2327