-
Notifications
You must be signed in to change notification settings - Fork 15
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
refactor: job isolation done #204
base: main
Are you sure you want to change the base?
Changes from 4 commits
e703bff
ff84440
decc1ad
d11c246
a35594a
7336d08
37cff7a
d0f2e34
11d3be9
b1ba517
9c2c64f
e3dcaea
04e9324
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 | ||||
---|---|---|---|---|---|---|
|
@@ -216,14 +216,29 @@ impl Database for MongoDb { | |||||
|
||||||
tracing::debug!(job_type = ?job_type, category = "db_call", "Fetching latest job by type"); | ||||||
|
||||||
// Get the first (and only) result if it exists | ||||||
match cursor.try_next().await? { | ||||||
Some(doc) => { | ||||||
let job: JobItem = mongodb::bson::from_document(doc)?; | ||||||
let attributes = [KeyValue::new("db_operation_name", "get_latest_job_by_type")]; | ||||||
let duration = start.elapsed(); | ||||||
ORCHESTRATOR_METRICS.db_calls_response_time.record(duration.as_secs_f64(), &attributes); | ||||||
Ok(Some(job)) | ||||||
// Add debug logging to see the raw document | ||||||
tracing::info!(raw_document = ?doc, "Raw document from MongoDB"); | ||||||
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.
Suggested change
|
||||||
|
||||||
// Try to deserialize and log any errors | ||||||
match mongodb::bson::from_document::<JobItem>(doc.clone()) { | ||||||
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. we're cloning here so that we can log in error later? if yes, should we explain that's why we're cloning it? |
||||||
Ok(job) => { | ||||||
tracing::info!(deserialized_job = ?job, "Successfully deserialized job"); | ||||||
let attributes = [KeyValue::new("db_operation_name", "get_latest_job_by_type")]; | ||||||
let duration = start.elapsed(); | ||||||
ORCHESTRATOR_METRICS.db_calls_response_time.record(duration.as_secs_f64(), &attributes); | ||||||
Ok(Some(job)) | ||||||
} | ||||||
Err(e) => { | ||||||
tracing::error!( | ||||||
error = %e, | ||||||
document = ?doc, | ||||||
"Failed to deserialize document into JobItem" | ||||||
); | ||||||
Err(eyre!("Failed to deserialize document: {}", e)) | ||||||
} | ||||||
} | ||||||
} | ||||||
None => Ok(None), | ||||||
} | ||||||
|
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. this is also types to be precise. maybe we should add this to types.rs OR create a types folder with jobs.rs and metadata.rs? |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,86 @@ | ||||||
use chrono::{DateTime, Utc}; | ||||||
use serde::{Deserialize, Serialize}; | ||||||
|
||||||
#[derive(Debug, Clone, Serialize, Deserialize, Default, PartialEq, Eq)] | ||||||
pub struct CommonMetadata { | ||||||
pub process_attempt_no: u64, | ||||||
pub process_retry_attempt_no: u64, | ||||||
pub verification_attempt_no: u64, | ||||||
pub verification_retry_attempt_no: u64, | ||||||
#[serde(with = "chrono::serde::ts_seconds_option")] | ||||||
pub process_completed_at: Option<DateTime<Utc>>, | ||||||
#[serde(with = "chrono::serde::ts_seconds_option")] | ||||||
pub verification_completed_at: Option<DateTime<Utc>>, | ||||||
pub failure_reason: Option<String>, | ||||||
} | ||||||
|
||||||
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] | ||||||
pub struct SnosMetadata { | ||||||
// Required fields | ||||||
pub block_number: u64, | ||||||
pub full_output: bool, | ||||||
|
||||||
// Optional fields populated during processing | ||||||
pub cairo_pie_path: Option<String>, | ||||||
pub snos_output_path: Option<String>, | ||||||
pub program_output_path: Option<String>, | ||||||
pub snos_fact: Option<String>, | ||||||
} | ||||||
|
||||||
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] | ||||||
pub struct StateUpdateMetadata { | ||||||
// Required fields | ||||||
pub blocks_to_settle: Vec<u64>, | ||||||
// Paths for data | ||||||
pub snos_output_paths: Vec<String>, | ||||||
pub program_output_paths: Vec<String>, | ||||||
pub blob_data_paths: Vec<String>, | ||||||
|
||||||
// State tracking | ||||||
pub last_failed_block_no: Option<u64>, | ||||||
pub tx_hashes: Vec<String>, // key: attempt_no, value: comma-separated tx hashes | ||||||
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.
Suggested change
|
||||||
} | ||||||
|
||||||
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] | ||||||
pub struct ProvingMetadata { | ||||||
// Required fields | ||||||
pub block_number: u64, | ||||||
pub cairo_pie_path: Option<String>, | ||||||
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. if it's required it should not be optional right? 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. make an input field which is enum Input { 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. can remove 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. also add comments to explain what are inputs in metadata and what is added by jobs on the fly |
||||||
|
||||||
pub cross_verify: bool, | ||||||
pub download_proof: bool, | ||||||
|
||||||
pub snos_fact: String, | ||||||
|
||||||
// Proof related fields | ||||||
pub proof_path: Option<String>, | ||||||
pub verification_key_path: Option<String>, | ||||||
} | ||||||
|
||||||
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] | ||||||
pub struct DaMetadata { | ||||||
// Required fields | ||||||
pub block_number: u64, | ||||||
|
||||||
// DA specific fields | ||||||
pub blob_data_path: Option<String>, | ||||||
pub blob_commitment: Option<String>, | ||||||
pub blob_proof: Option<String>, | ||||||
pub tx_hash: Option<String>, | ||||||
pub blob_versioned_hash: Option<String>, | ||||||
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. let's keep what's needed for now. |
||||||
} | ||||||
|
||||||
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] | ||||||
#[serde(tag = "type")] | ||||||
pub enum JobSpecificMetadata { | ||||||
Snos(SnosMetadata), | ||||||
StateUpdate(StateUpdateMetadata), | ||||||
Proving(ProvingMetadata), | ||||||
Da(DaMetadata), | ||||||
} | ||||||
|
||||||
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] | ||||||
pub struct JobMetadata { | ||||||
pub common: CommonMetadata, | ||||||
pub specific: JobSpecificMetadata, | ||||||
} |
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.
we've a lot of metadata constants here. should we reuse this?