-
Notifications
You must be signed in to change notification settings - Fork 2
Pass reserver_ids to SQLite to pre-filter the queried chunks
#138
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
Open
ReinierMaas
wants to merge
3
commits into
master
Choose a base branch
from
reinier/pass_reserver_ids
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+373
−144
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,12 +3,13 @@ pub mod reserver; | |
|
|
||
| use crate::{ | ||
| common::{ | ||
| chunk::{Chunk, ChunkId}, | ||
| submission::Submission, | ||
| chunk::{Chunk, ChunkId, ChunkIndex}, | ||
| submission::{Submission, SubmissionId}, | ||
| }, | ||
| db::{magic::Bool, Connection, Pool, ReaderPool}, | ||
| }; | ||
| use futures::stream::{StreamExt as _, TryStreamExt as _}; | ||
| use libsqlite3_sys as ffi; | ||
| use metastate::MetaState; | ||
| use reserver::Reserver; | ||
| use sqlx::QueryBuilder; | ||
|
|
@@ -21,6 +22,63 @@ use std::sync::Arc; | |
| use super::strategy; | ||
| use crate::common::StrategicMetadataMap; | ||
|
|
||
| unsafe extern "C" fn sqlite_reserved_chunk_lookup( | ||
|
Contributor
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. Some documentation would be useful here, in particular about the parameters: as the parameter names and types are quite general. |
||
| context: *mut ffi::sqlite3_context, | ||
| n_args: i32, | ||
| args: *mut *mut ffi::sqlite3_value, | ||
| ) { | ||
| if n_args != 2 { | ||
| tracing::error!( | ||
| n_args, | ||
| "opsqueue_is_reserved called with unexpected argument count" | ||
| ); | ||
| // Fail open: this callback is an optimization only. | ||
| ffi::sqlite3_result_int(context, 0); | ||
| return; | ||
| } | ||
|
|
||
| let user_data = ffi::sqlite3_user_data(context) as *const Reserver<ChunkId, ChunkId>; | ||
| if user_data.is_null() { | ||
| tracing::error!("opsqueue_is_reserved called without registered reserver user_data"); | ||
| // Fail open: this callback is an optimization only. | ||
| ffi::sqlite3_result_int(context, 0); | ||
| return; | ||
| } | ||
|
|
||
| let submission_id_raw = ffi::sqlite3_value_int64(*args.add(0)); | ||
| let chunk_index_raw = ffi::sqlite3_value_int64(*args.add(1)); | ||
|
|
||
| let Ok(submission_id) = SubmissionId::try_from(submission_id_raw) else { | ||
| tracing::error!( | ||
| submission_id_raw, | ||
| "opsqueue_is_reserved got invalid submission_id" | ||
| ); | ||
| // Fail open: this callback is an optimization only. | ||
| ffi::sqlite3_result_int(context, 0); | ||
| return; | ||
| }; | ||
| let Ok(chunk_index) = ChunkIndex::try_from(chunk_index_raw) else { | ||
| tracing::error!( | ||
| chunk_index_raw, | ||
| "opsqueue_is_reserved got invalid chunk_index" | ||
| ); | ||
| // Fail open: this callback is an optimization only. | ||
| ffi::sqlite3_result_int(context, 0); | ||
| return; | ||
| }; | ||
|
|
||
| let chunk_id = ChunkId::from((submission_id, chunk_index)); | ||
| let is_reserved = (*user_data).is_reserved(&chunk_id); | ||
| ffi::sqlite3_result_int(context, i32::from(is_reserved)); | ||
| } | ||
|
|
||
| unsafe extern "C" fn sqlite_reserved_chunk_lookup_destructor(ptr: *mut std::ffi::c_void) { | ||
| if ptr.is_null() { | ||
| return; | ||
| } | ||
| let _boxed: Box<Reserver<ChunkId, ChunkId>> = Box::from_raw(ptr.cast()); | ||
| } | ||
|
|
||
| #[derive(Debug, Clone)] | ||
| pub struct Dispatcher { | ||
| reserver: Reserver<ChunkId, ChunkId>, | ||
|
|
@@ -66,6 +124,8 @@ impl Dispatcher { | |
| stale_chunks_notifier: &UnboundedSender<ChunkId>, | ||
| ) -> Result<Vec<(Chunk, Submission)>, sqlx::Error> { | ||
| let mut conn = pool.reader_conn().await?; | ||
| self.register_reserved_chunk_lookup(conn.get_inner()) | ||
| .await?; | ||
| let mut query_builder = QueryBuilder::new(""); | ||
| let stream = strategy | ||
| .build_query(&mut query_builder, &self.metastate) | ||
|
|
@@ -79,6 +139,43 @@ impl Dispatcher { | |
| .await | ||
| } | ||
|
|
||
| async fn register_reserved_chunk_lookup( | ||
| &self, | ||
| conn: &mut sqlx::SqliteConnection, | ||
| ) -> Result<(), sqlx::Error> { | ||
| let mut handle = conn.lock_handle().await?; | ||
| let sqlite = handle.as_raw_handle().as_ptr(); | ||
| let function_name = b"opsqueue_is_reserved\0"; | ||
|
|
||
| // Register the current reserver state on this connection. | ||
| // Re-registering replaces any previous callback on this handle. | ||
| let user_data = Box::new(self.reserver.clone()); | ||
| let user_data = Box::into_raw(user_data).cast::<std::ffi::c_void>(); | ||
|
|
||
| let rc = unsafe { | ||
| ffi::sqlite3_create_function_v2( | ||
| sqlite, | ||
| function_name.as_ptr().cast(), | ||
| 2, | ||
| ffi::SQLITE_UTF8, | ||
| user_data, | ||
| Some(sqlite_reserved_chunk_lookup), | ||
| None, | ||
| None, | ||
| Some(sqlite_reserved_chunk_lookup_destructor), | ||
| ) | ||
| }; | ||
|
|
||
| if rc != ffi::SQLITE_OK { | ||
| unsafe { sqlite_reserved_chunk_lookup_destructor(user_data) }; | ||
| return Err(sqlx::Error::Protocol(format!( | ||
| "sqlite3_create_function_v2 failed with rc={rc}" | ||
| ))); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| async fn reserve_chunk<E>( | ||
| &self, | ||
| chunk: Chunk, | ||
|
|
@@ -144,3 +241,58 @@ impl Dispatcher { | |
| .run_pending_tasks_periodically(cancellation_token); | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| #[cfg(feature = "server-logic")] | ||
| mod test { | ||
| use super::*; | ||
| use crate::common::chunk::ChunkId; | ||
| use crate::common::chunk::ChunkSize; | ||
| use crate::db::DBPools; | ||
| use tokio::sync::mpsc::unbounded_channel; | ||
| use ux::u63; | ||
|
|
||
| #[sqlx::test] | ||
| async fn fetch_and_reserve_chunks_excludes_already_reserved(db: sqlx::SqlitePool) { | ||
| let pools = DBPools::from_test_pool(&db); | ||
| let dispatcher = Dispatcher::new(Duration::from_secs(60)); | ||
| let (stale_chunks_notifier, mut _stale_chunks_receiver) = unbounded_channel::<ChunkId>(); | ||
|
|
||
| let mut writer_conn = pools.writer_conn().await.unwrap(); | ||
| let submission_id = crate::common::submission::db::insert_submission_from_chunks( | ||
| None, | ||
| vec![Some("a".into()), Some("b".into()), Some("c".into())], | ||
| None, | ||
| StrategicMetadataMap::default(), | ||
| ChunkSize::default(), | ||
| &mut writer_conn, | ||
| ) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| let pre_reserved_chunk = ChunkId::from((submission_id, u63::new(0).into())); | ||
| dispatcher | ||
| .reserver() | ||
| .try_reserve( | ||
| pre_reserved_chunk, | ||
| pre_reserved_chunk, | ||
| &stale_chunks_notifier, | ||
| ) | ||
| .expect("precondition: pre-reserving chunk should succeed"); | ||
|
|
||
| let reserved = dispatcher | ||
| .fetch_and_reserve_chunks( | ||
| pools.reader_pool(), | ||
| strategy::Strategy::Oldest, | ||
| 10, | ||
| &stale_chunks_notifier, | ||
| ) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| assert_eq!(reserved.len(), 2); | ||
| assert!(reserved.iter().all(|(chunk, _submission)| { | ||
| ChunkId::from((chunk.submission_id, chunk.chunk_index)) != pre_reserved_chunk | ||
| })); | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
The error could include more information ? Such as the source value and target type?
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.
Yes it could. We haven't done that for any of the other integer errors.