-
Notifications
You must be signed in to change notification settings - Fork 188
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
feat(torii-indexer): natural priority in parallelized tasks for model… #2912
base: main
Are you sure you want to change the base?
Conversation
WalkthroughOhayo, sensei! The pull request introduces significant modifications to the task management system in the Changes
Possibly related PRs
Suggested Reviewers
Sensei, the changes look quite intriguing! The transition to a 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (4)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/torii/indexer/src/engine.rs (2)
881-896
: Document priority levels and consider using constants, sensei!The task identifier generation logic cleverly uses bit manipulation for priorities, but could benefit from better documentation and constant definitions:
+ // Priority levels for different event types + const PRIORITY_MODEL_EVENT: u64 = 0; + const PRIORITY_STORE_OPS: u64 = 2; + // Mask to clear priority bits + const PRIORITY_MASK: u64 = 0x00FFFFFFFFFFFFFF; + match processor.event_key().as_str() { "ModelRegistered" | "EventRegistered" => { let mut hasher = DefaultHasher::new(); event.keys[1].hash(&mut hasher); - // Priority 0 (highest) for model/event registration - let hash = hasher.finish() & 0x00FFFFFFFFFFFFFF; - hash | (0u64 << 56) + // Use highest 8 bits for priority, lower 56 bits for hash-based grouping + let hash = hasher.finish() & PRIORITY_MASK; + hash | (PRIORITY_MODEL_EVENT << 56) }
601-657
: Enhance error handling with more context, sensei!The task processing implementation looks solid, but could benefit from improved error handling. Consider adding more context to errors and enhancing debug logging:
if let Err(e) = processor .process( &world, &mut local_db, event.block_number, event.block_timestamp, &event.event_id, &event.event, &event_processor_config, ) .await { error!( target: LOG_TARGET, event_name = processor.event_key(), + block_number = %event.block_number, + timestamp = %event.block_timestamp, error = %e, task_id = %task_id, + "Failed to process event. This might impact dependent operations.", - "Processing parallelized event." ); + // Propagate error with additional context + return Err(anyhow::anyhow!("Failed to process event {}: {}", event.event_id, e)); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/indexer/src/engine.rs
(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (1)
crates/torii/indexer/src/engine.rs (1)
219-219
: Ohayo! Nice choice using BTreeMap for ordered task processing, sensei!The switch from
HashMap
toBTreeMap
enables natural priority ordering of tasks, which is crucial for maintaining the correct processing sequence of model registrations and store operations.Also applies to: 253-253
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.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/torii/indexer/src/engine.rs (2)
256-256
: Ohayo, sensei! Confirm consistent initialization oftasks
.Initializing
tasks
withBTreeMap::new()
is appropriate. However, if there are default priorities or initial tasks, consider initializing them here for clarity.
603-666
: Ohayo, sensei! Potential task starvation due to sequential priority processing.Processing tasks strictly sequentially by priority level may cause lower-priority tasks to be delayed indefinitely if higher-priority tasks are continuously added. Consider implementing a more dynamic scheduling strategy to balance task priorities and prevent starvation.
Do you want assistance in proposing an enhanced task scheduling mechanism?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/indexer/src/engine.rs
(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (3)
crates/torii/indexer/src/engine.rs (3)
210-212
: Ohayo, sensei! Good use of type aliases for clarity.Introducing
TaskPriority
andTaskId
improves code readability and ensures type safety.
222-222
: Ohayo, sensei! Verify the implications of changingtasks
to aBTreeMap
.Switching from
HashMap
toBTreeMap
fortasks
introduces ordered task management based on priority. Ensure this change aligns with the desired task processing logic and does not introduce unintended side effects.
889-903
:⚠️ Potential issueOhayo, sensei! Potential hash collisions in
task_identifier
generation.Using truncated hash values for
task_identifier
may lead to collisions, causing unrelated tasks to share the same identifier. Consider using a more collision-resistant method or including additional unique data to ensure task identifiers are unique.Please run the following script to check for hash collisions:
crates/torii/indexer/src/engine.rs
Outdated
self.tasks | ||
.entry(task_priority) | ||
.or_default() | ||
.entry(task_identifier) | ||
.or_default() | ||
.push(( | ||
contract_type, | ||
ParallelizedEvent { | ||
event_id: event_id.to_string(), | ||
event: event.clone(), | ||
block_number, | ||
block_timestamp, | ||
}, | ||
)); | ||
} else { | ||
// Process non-parallelized events immediately |
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.
Ohayo, sensei! Review handling of task_identifier
equal to zero.
When task_identifier
is zero, events are processed immediately outside of the priority queue. Ensure that zero cannot be a valid task_identifier
that should be queued, or adjust the logic to handle zero appropriately to prevent unintended bypassing of the task queue.
crates/torii/indexer/src/engine.rs
Outdated
let (task_priority, task_identifier) = match processor.event_key().as_str() { | ||
"ModelRegistered" | "EventRegistered" => { | ||
let mut hasher = DefaultHasher::new(); | ||
event.keys.iter().for_each(|k| k.hash(&mut hasher)); | ||
let hash = hasher.finish() & 0x00FFFFFFFFFFFFFF; | ||
(0usize, hash) // Priority 0 (highest) for model/event registration | ||
} | ||
"StoreSetRecord" | "StoreUpdateRecord" | "StoreUpdateMember" | "StoreDelRecord" => { | ||
let mut hasher = DefaultHasher::new(); | ||
// model selector | ||
event.keys[1].hash(&mut hasher); | ||
// entity id | ||
event.keys[2].hash(&mut hasher); | ||
hasher.finish() | ||
let hash = hasher.finish() & 0x00FFFFFFFFFFFFFF; | ||
(2usize, hash) // Priority 2 (lower) for store operations | ||
} | ||
_ => 0, | ||
_ => (0, 0) // No parallelization for other events |
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.
Ohayo, sensei! Guard against index out-of-bounds when accessing event.keys
.
Accessing event.keys[1]
and event.keys[2]
without checking their existence may lead to panics if the keys vector is shorter than expected. Ensure that the event.keys
vector has the required length before accessing these indices.
Apply this diff to add necessary checks:
+ if event.keys.len() >= 3 {
let mut hasher = DefaultHasher::new();
event.keys[1].hash(&mut hasher);
event.keys[2].hash(&mut hasher);
let hash = hasher.finish() & 0x00FFFFFFFFFFFFFF;
(2usize, hash) // Priority 2 (lower) for store operations
+ } else {
+ warn!(target: LOG_TARGET, "Insufficient event keys for hashing. Using default task identifier.");
+ (2usize, 0) // Handle with default or error as appropriate
+ }
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
let (task_priority, task_identifier) = match processor.event_key().as_str() { | |
"ModelRegistered" | "EventRegistered" => { | |
let mut hasher = DefaultHasher::new(); | |
event.keys.iter().for_each(|k| k.hash(&mut hasher)); | |
let hash = hasher.finish() & 0x00FFFFFFFFFFFFFF; | |
(0usize, hash) // Priority 0 (highest) for model/event registration | |
} | |
"StoreSetRecord" | "StoreUpdateRecord" | "StoreUpdateMember" | "StoreDelRecord" => { | |
let mut hasher = DefaultHasher::new(); | |
// model selector | |
event.keys[1].hash(&mut hasher); | |
// entity id | |
event.keys[2].hash(&mut hasher); | |
hasher.finish() | |
let hash = hasher.finish() & 0x00FFFFFFFFFFFFFF; | |
(2usize, hash) // Priority 2 (lower) for store operations | |
} | |
_ => 0, | |
_ => (0, 0) // No parallelization for other events | |
let (task_priority, task_identifier) = match processor.event_key().as_str() { | |
"ModelRegistered" | "EventRegistered" => { | |
let mut hasher = DefaultHasher::new(); | |
event.keys.iter().for_each(|k| k.hash(&mut hasher)); | |
let hash = hasher.finish() & 0x00FFFFFFFFFFFFFF; | |
(0usize, hash) // Priority 0 (highest) for model/event registration | |
} | |
"StoreSetRecord" | "StoreUpdateRecord" | "StoreUpdateMember" | "StoreDelRecord" => { | |
if event.keys.len() >= 3 { | |
let mut hasher = DefaultHasher::new(); | |
event.keys[1].hash(&mut hasher); | |
event.keys[2].hash(&mut hasher); | |
let hash = hasher.finish() & 0x00FFFFFFFFFFFFFF; | |
(2usize, hash) // Priority 2 (lower) for store operations | |
} else { | |
warn!(target: LOG_TARGET, "Insufficient event keys for hashing. Using default task identifier."); | |
(2usize, 0) // Handle with default or error as appropriate | |
} | |
} | |
_ => (0, 0) // No parallelization for other events |
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.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/torii/indexer/src/engine.rs (1)
895-901
:⚠️ Potential issueAdd safety check for event keys access.
Guard against potential index out-of-bounds when accessing
event.keys
."StoreSetRecord" | "StoreUpdateRecord" | "StoreUpdateMember" | "StoreDelRecord" => { + if event.keys.len() < 3 { + warn!( + target: LOG_TARGET, + "Insufficient event keys for hashing. Using default task identifier." + ); + return (2usize, 0); + } let mut hasher = DefaultHasher::new(); event.keys[1].hash(&mut hasher); event.keys[2].hash(&mut hasher); let hash = hasher.finish(); (2usize, hash) }🧰 Tools
🪛 GitHub Actions: ci
[warning] 900-900: Missing comma in match expression
🧹 Nitpick comments (6)
crates/torii/indexer/src/processors/store_del_record.rs (1)
66-72
: Excellent error message improvement, but there's a small formatting issue, sensei!The enhanced error message with hex-formatted selector will make debugging much easier. However, there's a formatting issue to fix.
Fix the extra closing parenthesis on line 68 as flagged by the CI:
- return Err(anyhow::anyhow!( - "Failed to retrieve model with selector {:#x}: {}", - event.selector, - e - )) + return Err(anyhow::anyhow!( + "Failed to retrieve model with selector {:#x}: {}", + event.selector, + e + ));🧰 Tools
🪛 GitHub Actions: ci
[warning] 68-68: Extra closing parenthesis, formatting issue
crates/torii/indexer/src/processors/store_update_record.rs (1)
71-75
: Ohayo! Small formatting fix needed, sensei!The CI pipeline indicates an extra closing parenthesis. Please fix the formatting to pass the pipeline checks.
- return Err(anyhow::anyhow!( - "Failed to retrieve model with selector {:#x}: {}", - event.selector, - e - )) + return Err(anyhow::anyhow!("Failed to retrieve model with selector {:#x}: {}", event.selector, e))🧰 Tools
🪛 GitHub Actions: ci
[warning] 72-72: Extra closing parenthesis, formatting issue
crates/torii/indexer/src/processors/store_set_record.rs (1)
67-73
: Fix formatting and enhance error context, sensei!The error handling provides good context, but there are two improvements to consider:
- Fix the formatting issue with the extra closing parenthesis.
- Consider including the model namespace/name in the error message for better debugging.
Apply this diff:
- Err(e) => { - return Err(anyhow::anyhow!( - "Failed to retrieve model with selector {:#x}: {}", - event.selector, - e - )) - } + Err(e) => return Err(anyhow::anyhow!( + "Failed to retrieve model with selector {:#x} (namespace: {}, name: {}): {}", + event.selector, + model.namespace, + model.name, + e + ))🧰 Tools
🪛 GitHub Actions: ci
[warning] 69-69: Extra closing parenthesis, formatting issue
crates/torii/indexer/src/processors/store_update_member.rs (1)
64-71
: Enhanced error handling with debug logging, sensei!The addition of debug logging provides better visibility into model absence cases.
Consider using string interpolation for a more idiomatic debug message:
- debug!( - target: LOG_TARGET, - selector = %model_selector, - "Model does not exist, skipping." - ); + debug!( + target: LOG_TARGET, + "Model with selector {model_selector:#x} does not exist, skipping." + );crates/torii/indexer/src/engine.rs (2)
600-662
: Clean up formatting issues.Please address the following formatting issues:
- Remove extra whitespace lines at 600, 607, and 616
- Split the chain method calls at lines 623-625 for better readability
- let processor = processors.iter() - .find(|p| p.validate(&event.event)) - .expect("Must find at least one processor for the event"); + let processor = processors + .iter() + .find(|p| p.validate(&event.event)) + .expect("Must find at least one processor for the event");🧰 Tools
🪛 GitHub Actions: ci
[warning] 600-600: Extra whitespace line found, formatting issue
[warning] 607-607: Extra whitespace line found, formatting issue
[warning] 616-616: Extra whitespace line found, formatting issue
[warning] 623-625: Chain method calls should be split into multiple lines for better readability
907-920
: Format method chain for better readability.Split the long chain of method calls across multiple lines.
- self.tasks - .entry(task_priority) - .or_default() - .entry(task_identifier) - .or_default() - .push(( - contract_type, - ParallelizedEvent { - event_id: event_id.to_string(), - event: event.clone(), - block_number, - block_timestamp, - }, - )); + self.tasks + .entry(task_priority) + .or_default() + .entry(task_identifier) + .or_default() + .push(( + contract_type, + ParallelizedEvent { + event_id: event_id.to_string(), + event: event.clone(), + block_number, + block_timestamp, + }, + ));🧰 Tools
🪛 GitHub Actions: ci
[warning] 907-917: Long chain of method calls should be formatted across multiple lines
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
crates/torii/indexer/src/engine.rs
(5 hunks)crates/torii/indexer/src/processors/store_del_record.rs
(2 hunks)crates/torii/indexer/src/processors/store_set_record.rs
(2 hunks)crates/torii/indexer/src/processors/store_update_member.rs
(3 hunks)crates/torii/indexer/src/processors/store_update_record.rs
(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
crates/torii/indexer/src/processors/store_set_record.rs
[warning] 69-69: Extra closing parenthesis, formatting issue
crates/torii/indexer/src/processors/store_update_member.rs
[warning] 74-74: Extra closing parenthesis, formatting issue
crates/torii/indexer/src/processors/store_del_record.rs
[warning] 68-68: Extra closing parenthesis, formatting issue
crates/torii/indexer/src/processors/store_update_record.rs
[warning] 72-72: Extra closing parenthesis, formatting issue
crates/torii/indexer/src/engine.rs
[warning] 600-600: Extra whitespace line found, formatting issue
[warning] 607-607: Extra whitespace line found, formatting issue
[warning] 616-616: Extra whitespace line found, formatting issue
[warning] 623-625: Chain method calls should be split into multiple lines for better readability
[warning] 900-900: Missing comma in match expression
[warning] 907-917: Long chain of method calls should be formatted across multiple lines
🔇 Additional comments (11)
crates/torii/indexer/src/processors/store_del_record.rs (2)
38-38
: Ohayo! Parameter renaming looks good, sensei!Removing the underscore prefix is appropriate since we're now using the config parameter.
58-65
: Nice error handling improvement, sensei!The additional check for non-empty namespaces prevents silently ignoring errors when no namespaces are configured, making the behavior more explicit and safer.
crates/torii/indexer/src/processors/store_update_record.rs (2)
39-39
: Ohayo! Parameter renaming looks good, sensei!The change from
_config
toconfig
properly reflects that this parameter is now being utilized in the implementation.
62-76
: Excellent error handling improvements, sensei!The changes enhance error handling in two valuable ways:
- The condition
!config.namespaces.is_empty()
ensures we only skip missing models when explicitly filtering by namespaces- The error message now includes the selector in hex format for better debugging
🧰 Tools
🪛 GitHub Actions: ci
[warning] 72-72: Extra closing parenthesis, formatting issue
crates/torii/indexer/src/processors/store_set_record.rs (2)
39-39
: Ohayo! Parameter rename looks good, sensei!The removal of the underscore prefix is appropriate since the
config
parameter is now actively used in the error handling logic.
59-66
: Ohayo! Nice enhancement to the namespace-specific indexing logic, sensei!The condition
!config.namespaces.is_empty()
effectively handles the case where only specific namespaces are being indexed.crates/torii/indexer/src/processors/store_update_member.rs (3)
10-10
: Ohayo! Import changes look good, sensei!The addition of
debug
to thetracing
import aligns well with the new debug logging capability.
40-40
: Parameter rename reflects its usage, sensei!The rename from
_config
toconfig
accurately reflects that the parameter is now actively used in the function.
73-77
:⚠️ Potential issueFix the formatting issue, sensei!
There's an extra closing parenthesis as indicated by the pipeline failure.
Apply this fix:
- return Err(anyhow::anyhow!( - "Failed to retrieve model with selector {:#x}: {}", - event.selector, - e - )) + return Err(anyhow::anyhow!( + "Failed to retrieve model with selector {:#x}: {}", + event.selector, + e + ))Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Actions: ci
[warning] 74-74: Extra closing parenthesis, formatting issue
crates/torii/indexer/src/engine.rs (2)
Line range hint
210-256
: Ohayo, sensei! LGTM - Clean type definitions and struct changes.The introduction of type aliases and the switch to
BTreeMap
for tasks improves code organization and enables priority-based processing.
603-666
: Ohayo, sensei! Excellent implementation of priority-based task processing.The sequential processing of priority levels while maintaining concurrency within each level is well-designed.
🧰 Tools
🪛 GitHub Actions: ci
[warning] 607-607: Extra whitespace line found, formatting issue
[warning] 616-616: Extra whitespace line found, formatting issue
[warning] 623-625: Chain method calls should be split into multiple lines for better readability
crates/torii/indexer/src/engine.rs
Outdated
let (task_priority, task_identifier) = match processor.event_key().as_str() { | ||
"ModelRegistered" | "EventRegistered" => { | ||
let mut hasher = DefaultHasher::new(); | ||
event.keys.iter().for_each(|k| k.hash(&mut hasher)); | ||
let hash = hasher.finish(); | ||
(0usize, hash) // Priority 0 (highest) for model/event registration | ||
} | ||
"StoreSetRecord" | "StoreUpdateRecord" | "StoreUpdateMember" | "StoreDelRecord" => { | ||
let mut hasher = DefaultHasher::new(); | ||
// model selector | ||
event.keys[1].hash(&mut hasher); | ||
// entity id | ||
event.keys[2].hash(&mut hasher); | ||
hasher.finish() | ||
let hash = hasher.finish(); | ||
(2usize, hash) // Priority 2 (lower) for store operations | ||
} | ||
_ => 0, | ||
_ => (0, 0) // No parallelization for other events |
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.
Fix match expression syntax.
Add missing comma in the match expression.
"StoreSetRecord" | "StoreUpdateRecord" | "StoreUpdateMember" | "StoreDelRecord" => {
let mut hasher = DefaultHasher::new();
event.keys[1].hash(&mut hasher);
event.keys[2].hash(&mut hasher);
let hash = hasher.finish();
- (2usize, hash) // Priority 2 (lower) for store operations
+ (2usize, hash), // Priority 2 (lower) for store operations
}
_ => (0, 0) // No parallelization for other events
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
let (task_priority, task_identifier) = match processor.event_key().as_str() { | |
"ModelRegistered" | "EventRegistered" => { | |
let mut hasher = DefaultHasher::new(); | |
event.keys.iter().for_each(|k| k.hash(&mut hasher)); | |
let hash = hasher.finish(); | |
(0usize, hash) // Priority 0 (highest) for model/event registration | |
} | |
"StoreSetRecord" | "StoreUpdateRecord" | "StoreUpdateMember" | "StoreDelRecord" => { | |
let mut hasher = DefaultHasher::new(); | |
// model selector | |
event.keys[1].hash(&mut hasher); | |
// entity id | |
event.keys[2].hash(&mut hasher); | |
hasher.finish() | |
let hash = hasher.finish(); | |
(2usize, hash) // Priority 2 (lower) for store operations | |
} | |
_ => 0, | |
_ => (0, 0) // No parallelization for other events | |
let (task_priority, task_identifier) = match processor.event_key().as_str() { | |
"ModelRegistered" | "EventRegistered" => { | |
let mut hasher = DefaultHasher::new(); | |
event.keys.iter().for_each(|k| k.hash(&mut hasher)); | |
let hash = hasher.finish(); | |
(0usize, hash) // Priority 0 (highest) for model/event registration | |
} | |
"StoreSetRecord" | "StoreUpdateRecord" | "StoreUpdateMember" | "StoreDelRecord" => { | |
let mut hasher = DefaultHasher::new(); | |
event.keys[1].hash(&mut hasher); | |
event.keys[2].hash(&mut hasher); | |
let hash = hasher.finish(); | |
(2usize, hash), // Priority 2 (lower) for store operations | |
} | |
_ => (0, 0) // No parallelization for other events |
🧰 Tools
🪛 GitHub Actions: ci
[warning] 900-900: Missing comma in match expression
Summary by CodeRabbit