Repository navigation
End GetRecords iterators after closed shard exhaustion #372
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,7 +23,7 @@ use extenddb_core::types::{ | |
| use extenddb_storage::StreamEngine; | ||
| use extenddb_storage::error::StorageError; | ||
| use extenddb_storage::util::{parse_stream_arn, stream_arn}; | ||
| use extenddb_storage::{StreamListResult, StreamRecordsResult}; | ||
| use extenddb_storage::{StreamContinuation, StreamListResult, StreamRecordsResult}; | ||
|
|
||
| use crate::MongoEngine; | ||
|
|
||
|
|
@@ -293,6 +293,12 @@ impl StreamEngine for MongoEngine { | |
| let shard_id = shard_id.to_owned(); | ||
| let after_sequence = after_sequence.map(std::borrow::ToOwned::to_owned); | ||
| Box::pin(async move { | ||
| let limit = usize::try_from(limit) | ||
| .ok() | ||
| .filter(|limit| (1..=1000).contains(limit)) | ||
| .ok_or_else(|| { | ||
| StorageError::Validation("Limit must be between 1 and 1000".to_owned()) | ||
| })?; | ||
| // Ownership guard: only return records if the shard's backing table | ||
| // belongs to the calling account. `stream_shards`/`stream_records` | ||
| // live in the data database while the `tables` catalog (which | ||
|
|
@@ -305,6 +311,9 @@ impl StreamEngine for MongoEngine { | |
| .find_one(doc! { "shard_id": &shard_id }) | ||
| .await | ||
| .map_err(|e| StorageError::Internal(e.to_string()))?; | ||
| let closed = shard_doc | ||
| .as_ref() | ||
| .is_some_and(|shard| shard.get_str("ending_sequence_number").is_ok()); | ||
| let owned = match shard_doc.as_ref().and_then(|d| d.get_str("table_id").ok()) { | ||
| // Look the table up by its globally-unique `table_id` (a | ||
| // top-level field), then compare the owning account_id read out | ||
|
|
@@ -348,7 +357,7 @@ impl StreamEngine for MongoEngine { | |
|
|
||
| let opts = FindOptions::builder() | ||
| .sort(doc! { "sequence_number": 1 }) | ||
| .limit(limit) | ||
| .limit((limit + 1) as i64) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: the lookahead row is fetched on every call but read only when the shard is closed, so until shards close this is one extra row per GetRecords on each of the three backends. Fetching it only for a closed shard would leave the open path as it is. |
||
| .build(); | ||
|
|
||
| let cursor = records_coll | ||
|
|
@@ -362,8 +371,10 @@ impl StreamEngine for MongoEngine { | |
| .await | ||
| .map_err(|e| StorageError::Internal(e.to_string()))?; | ||
|
|
||
| let has_more = docs.len() > limit; | ||
| let records: Vec<StreamRecord> = docs | ||
| .into_iter() | ||
| .take(limit) | ||
| .map(|d| { | ||
| let record_bson = d | ||
| .get("record_data") | ||
|
|
@@ -376,7 +387,12 @@ impl StreamEngine for MongoEngine { | |
| .collect::<Result<Vec<_>, _>>()?; | ||
|
|
||
| let last_seq = records.last().map(|r| r.dynamodb.sequence_number.clone()); | ||
| Ok((records, last_seq)) | ||
| let continuation = if closed && !has_more { | ||
| StreamContinuation::End | ||
| } else { | ||
|
Comment on lines
+390
to
+392
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As is, this change has no effect in ExtendDB: none of our backends closes a shard, so |
||
| StreamContinuation::More(last_seq) | ||
| }; | ||
| Ok((records, continuation)) | ||
| }) | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,9 +7,9 @@ use extenddb_core::types::{ | |
| SequenceNumberRange, Shard, StreamDescription, StreamRecord, StreamStatus, StreamSummary, | ||
| StreamViewType, | ||
| }; | ||
| use extenddb_storage::StreamEngine; | ||
| use extenddb_storage::error::StorageError; | ||
| use extenddb_storage::util::{parse_stream_arn, stream_arn}; | ||
| use extenddb_storage::{StreamContinuation, StreamEngine, StreamRecordsResult}; | ||
| use futures::future::BoxFuture; | ||
| use sqlx::PgPool; | ||
|
|
||
|
|
@@ -128,11 +128,17 @@ impl StreamEngine for PostgresEngine { | |
| shard_id: &str, | ||
| after_sequence: Option<&str>, | ||
| limit: i64, | ||
| ) -> BoxFuture<'_, Result<(Vec<StreamRecord>, Option<String>), StorageError>> { | ||
| ) -> BoxFuture<'_, StreamRecordsResult> { | ||
| let account_id = account_id.to_string(); | ||
| let shard_id = shard_id.to_string(); | ||
| let after_sequence = after_sequence.map(std::string::ToString::to_string); | ||
| Box::pin(async move { | ||
| let limit = usize::try_from(limit) | ||
| .ok() | ||
| .filter(|limit| (1..=1000).contains(limit)) | ||
| .ok_or_else(|| { | ||
| StorageError::Validation("Limit must be between 1 and 1000".to_owned()) | ||
| })?; | ||
| // Ownership guard: only return records if the shard's backing table | ||
| // belongs to the calling account. `stream_shards`/`stream_records` | ||
| // rows live in the data database while the `tables` catalog (which | ||
|
|
@@ -141,14 +147,18 @@ impl StreamEngine for PostgresEngine { | |
| // (data pool), then table_id + account_id (catalog pool). A shard | ||
| // iterator for a table the caller does not own is rejected as an | ||
| // invalid shard iterator (below). | ||
| let shard_table_id: Option<(String,)> = | ||
| sqlx::query_as("SELECT table_id FROM stream_shards WHERE shard_id = $1") | ||
| .bind(&shard_id) | ||
| .fetch_optional(&self.data_pool) | ||
| .await | ||
| .map_err(|e| StorageError::Internal(e.to_string()))?; | ||
| let shard_table_id: Option<(String, Option<String>)> = sqlx::query_as( | ||
| "SELECT table_id, ending_sequence_number FROM stream_shards WHERE shard_id = $1", | ||
| ) | ||
| .bind(&shard_id) | ||
| .fetch_optional(&self.data_pool) | ||
| .await | ||
| .map_err(|e| StorageError::Internal(e.to_string()))?; | ||
| let closed = shard_table_id | ||
| .as_ref() | ||
| .is_some_and(|(_, end)| end.is_some()); | ||
| let owned = match &shard_table_id { | ||
| Some((table_id,)) => sqlx::query_as::<_, (i32,)>( | ||
| Some((table_id, _)) => sqlx::query_as::<_, (i32,)>( | ||
| "SELECT 1 FROM tables WHERE table_id = $1 AND account_id = $2", | ||
| ) | ||
| .bind(table_id) | ||
|
|
@@ -178,7 +188,7 @@ impl StreamEngine for PostgresEngine { | |
| ) | ||
| .bind(&shard_id) | ||
| .bind(&after) | ||
| .bind(limit) | ||
| .bind((limit + 1) as i64) | ||
| .fetch_all(&self.data_pool) | ||
| .await | ||
| .map_err(|e| StorageError::Internal(e.to_string()))? | ||
|
|
@@ -189,21 +199,28 @@ impl StreamEngine for PostgresEngine { | |
| ORDER BY sequence_number LIMIT $2", | ||
| ) | ||
| .bind(&shard_id) | ||
| .bind(limit) | ||
| .bind((limit + 1) as i64) | ||
| .fetch_all(&self.data_pool) | ||
| .await | ||
| .map_err(|e| StorageError::Internal(e.to_string()))? | ||
| }; | ||
|
|
||
| let has_more = rows.len() > limit; | ||
| let records: Vec<StreamRecord> = rows | ||
| .into_iter() | ||
| .take(limit) | ||
| .map(|(data,)| { | ||
| serde_json::from_value(data).map_err(|e| StorageError::Internal(e.to_string())) | ||
| }) | ||
| .collect::<Result<Vec<_>, _>>()?; | ||
|
|
||
| let last_seq = records.last().map(|r| r.dynamodb.sequence_number.clone()); | ||
| Ok((records, last_seq)) | ||
| let continuation = if closed && !has_more { | ||
| StreamContinuation::End | ||
| } else { | ||
| StreamContinuation::More(last_seq) | ||
| }; | ||
| Ok((records, continuation)) | ||
|
Comment on lines
+218
to
+223
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add tests for the new continuation path on PostgreSQL and MongoDB, not only SQLite. Please also add an end-to-end test that disables a stream, reads each shard to the end, and checks that every record arrives once and
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Major: the continuation path has tests on SQLite only, so the |
||
| }) | ||
| } | ||
|
|
||
|
|
||
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.
Blocking on rebase: main builds this token through
encode_shard_iterator, which carries the stream ARN as a fifth field (#419, merged after you opened this). A four-field token is classifiedExpiredand the server answers ExpiredIteratorException before the handler runs (crates/server/src/request_helpers.rs:210-218), so a consumer would fail on its second call. Callencode_shard_iteratorand passit.stream_arnthrough, and assert the result withdecode_shard_iteratorin the unit test rather than against the four-field string.