Feat: Retry endpoint added #198

Mohiiit · 2024-12-26T07:43:32Z

Created a new REST endpoint /jobs/:id/retry following RESTful conventions
- Implemented retry logic that:
  - Verifies the job is in Failed state before allowing retry
  - Updates job status to RetryAttempt
  - Triggers job processing automatically after status update
Additional Improvements:
- Enhanced verification status handling:
  - Added support for VerificationTimeout status in the verify endpoint
  - Jobs in either PendingVerification or VerificationTimeout states can now be verified

…status

coveralls · 2024-12-26T08:20:47Z

coverage: 67.321% (-0.1%) from 67.449%
when pulling d34594c on feat/retry-job-status
into a008642 on main.

heemankv · 2024-12-27T08:33:32Z

crates/orchestrator/src/jobs/mod.rs

-        JobStatus::Created | JobStatus::VerificationFailed => {
-            tracing::info!(job_id = ?id, status = ?job.status, "Job status is Created or VerificationFailed, proceeding with processing");
+        JobStatus::Created | JobStatus::VerificationFailed | JobStatus::RetryAttempt => {
+            tracing::info!(job_id = ?id, status = ?job.status, "Job status is Created or VerificationFailed or RetryAttempt, proceeding with processing");


I think the message here is a lil too much, since we are already logging the status so we can change the message to something more useful.
WDYT @Mohiiit ?

heemankv · 2024-12-27T08:34:04Z

crates/orchestrator/src/jobs/mod.rs

@@ -209,8 +209,8 @@ pub async fn process_job(id: Uuid, config: Arc<Config>) -> Result<(), JobError>
    match job.status {
        // we only want to process jobs that are in the created or verification failed state.
        // verification failed state means that the previous processing failed and we want to retry


IMO we need to update this comment to match the new implementation.

heemankv · 2024-12-27T08:35:07Z

crates/orchestrator/src/jobs/mod.rs

@@ -333,8 +333,8 @@ pub async fn verify_job(id: Uuid, config: Arc<Config>) -> Result<(), JobError> {
    tracing::Span::current().record("internal_id", job.internal_id.clone());

    match job.status {
-        JobStatus::PendingVerification => {
-            tracing::debug!(job_id = ?id, "Job status is PendingVerification, proceeding with verification");
+        JobStatus::PendingVerification | JobStatus::VerificationTimeout => {


As discussed, let's add an IMP! comment here explaining why doing this is fine,
referring that we don't add the jobs that timeout to the queue back again

heemankv · 2024-12-27T08:36:49Z

crates/orchestrator/src/jobs/mod.rs

-        JobStatus::PendingVerification => {
-            tracing::debug!(job_id = ?id, "Job status is PendingVerification, proceeding with verification");
+        JobStatus::PendingVerification | JobStatus::VerificationTimeout => {
+            tracing::info!(job_id = ?id, status = ?job.status, "Job status is PendingVerification or VerificationTimeout, proceeding with verification");


Same as previous:
I think the message here is a lil too much, since we are already logging the status so we can change the message to something more useful.
WDYT @Mohiiit ?

heemankv · 2024-12-27T08:40:39Z

crates/orchestrator/src/jobs/types.rs

@@ -103,6 +103,8 @@ pub enum JobStatus {
    VerificationFailed,
    /// The job failed completing
    Failed,
+    /// The job is being retried
+    RetryAttempt,


Nit-pick

RetryAttempt could be Retrying to match the progressive verb form used in other statuses

Suggestions from Claude :

Let me review the enum names carefully and suggest only meaningful improvements:

Created could be Pending or Queued - both more precisely describe that the job is waiting to be processed

LockedForProcessing is quite verbose - could be simplified to Processing since the locking is an implementation detail

RetryAttempt could be Retrying to match the progressive verb form used in other statuses

The rest of the names (PendingVerification, Completed, VerificationTimeout, VerificationFailed, Failed) are clear and appropriate as they are.

So the simplified version would be:

pub enum JobStatus { /// An acknowledgement that the job has been received by the /// orchestrator and is waiting to be processed Pending, /// Some system has taken a lock over the job for processing and no /// other system to process the job Processing, /// The job has been processed and is pending verification PendingVerification, /// The job has been processed and verified. No other actions needs to be taken Completed, /// The job was processed but the was unable to be verified under the given time VerificationTimeout, /// The job failed processing VerificationFailed, /// The job failed completing Failed, /// The job is being retried Retrying, }

These changes make the enum more consistent while maintaining clarity about each state's purpose. Let me know if you'd like me to explain any of these suggestions in more detail.

Ya agreed that RetryAttempt isn't in line with other enums. Although Retrying is a verb and others aren't a verb right? Does PendingRetry sound better? it goes in line with PendingVerification.

heemankv · 2024-12-27T08:49:26Z

crates/orchestrator/src/routes/job_routes.rs

+        }
+    };
+
+    // Get the job and verify it's in a failed state


Nit-pick

Remove and verify it's in a failed state as it's being written below.

heemankv · 2024-12-27T08:52:07Z

crates/orchestrator/src/routes/job_routes.rs

+                Err(e) => {
+                    ORCHESTRATOR_METRICS
+                        .failed_job_operations
+                        .add(1.0, &[KeyValue::new("operation_type", "retry_job")]);


operation_type should strictly be process_job since that's the function being called.
you can add another attribute like operation_info for registering retry_job

heemankv · 2024-12-27T08:53:21Z

crates/orchestrator/src/routes/job_routes.rs

+            }
+        }
+        Err(e) => {
+            ORCHESTRATOR_METRICS.failed_job_operations.add(1.0, &[KeyValue::new("operation_type", "retry_job")]);


Same as above

apoorvsadana · 2024-12-28T05:38:27Z

crates/orchestrator/src/jobs/types.rs

@@ -103,6 +103,8 @@ pub enum JobStatus {
    VerificationFailed,
    /// The job failed completing
    Failed,
+    /// The job is being retried
+    RetryAttempt,


Ya agreed that RetryAttempt isn't in line with other enums. Although Retrying is a verb and others aren't a verb right? Does PendingRetry sound better? it goes in line with PendingVerification.

apoorvsadana · 2024-12-28T05:55:08Z

crates/orchestrator/src/routes/job_routes.rs

+                .into_response();
+        }
+        Err(e) => {
+            return ApiResponse::<JobApiResponse>::error(e.to_string()).into_response();


i see a lot of ApiResponse::<JobApiResponse>::error, any way to abstract this? maybe a macro, maybe a TryFrom implementation

apoorvsadana · 2024-12-28T05:56:37Z

crates/orchestrator/src/routes/job_routes.rs

+    }
+
+    // Update the job status to RetryAttempt
+    match config.database().update_job(&job, JobItemUpdates::new().update_status(JobStatus::RetryAttempt).build()).await


the routes usually don't have the business logic, they just have basic process of taking the request, calling the function, taking the response and returning. i would suggest moving this inside the jobs mod.rs

apoorvsadana · 2024-12-28T06:01:26Z

crates/orchestrator/src/tests/server/job_routes.rs

@@ -119,6 +119,44 @@ async fn test_trigger_verify_job(#[future] setup_trigger: (SocketAddr, Arc<Confi
    }
 }

+#[tokio::test]
+#[rstest]
+async fn test_trigger_retry_job(#[future] setup_trigger: (SocketAddr, Arc<Config>)) {


should we also add tests that you can't retry is not in failed?

mohiiit added 3 commits December 26, 2024 11:38

feat: added retry endpoint for failed jobs

1eb726a

refactor: verify job will allow verification of the job with timeout …

db81c35

…status

chore: changelog updated

d34594c

Mohiiit requested review from heemankv and apoorvsadana December 26, 2024 09:07

Mohiiit self-assigned this Dec 26, 2024

Mohiiit requested a review from ocdbytes December 26, 2024 10:23

heemankv reviewed Dec 27, 2024

View reviewed changes

apoorvsadana reviewed Dec 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Retry endpoint added #198

Feat: Retry endpoint added #198

Mohiiit commented Dec 26, 2024

coveralls commented Dec 26, 2024

heemankv Dec 27, 2024

heemankv Dec 27, 2024

heemankv Dec 27, 2024

heemankv Dec 27, 2024

heemankv Dec 27, 2024

apoorvsadana Dec 28, 2024

heemankv Dec 27, 2024

heemankv Dec 27, 2024

heemankv Dec 27, 2024

apoorvsadana Dec 28, 2024

apoorvsadana Dec 28, 2024

apoorvsadana Dec 28, 2024

apoorvsadana Dec 28, 2024

Feat: Retry endpoint added #198

Are you sure you want to change the base?

Feat: Retry endpoint added #198

Conversation

Mohiiit commented Dec 26, 2024

coveralls commented Dec 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nit-pick

Suggestions from Claude :

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nit-pick

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment