Add result to jobs - #1502
Conversation
for more information, see https://pre-commit.ci
📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
participant Worker as Worker
participant Manager as JobMgr
participant DB as Database
Worker->>Worker: capture exception (exc_info)
Worker->>Worker: _persist_job_status(..., exc_info)
Worker->>Manager: finish_job(job, status, delete_job, exc_info)
Manager->>Manager: stringify exc_info if present
Manager->>DB: procrastinate_finish_job_v1(job_id, end_status, delete_job, job_result)
DB->>DB: UPDATE procrastinate_jobs SET result = job_result
DB-->>Manager: ack
Manager-->>Worker: persistence complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@procrastinate/sql/migrations/03.08.00_01_pre_result_to_job_procedure.sql`:
- Line 1: The migration adds the procrastinate_jobs.result column but the
procrastinate_finish_job_v1 procedure isn’t updated to accept and persist a
job_result parameter; update procrastinate_finish_job_v1 to accept job_result
text default null and modify its UPDATE to set result = job_result (or add a new
post-migration file 03.08.00_50_post_result_to_job_procedure.sql that performs
this change). Ensure the function signature includes job_result and the UPDATE
targeting procrastinate_jobs sets result = job_result so the new column is
populated.
In `@README.md`:
- Line 15: Fix the grammatical typo in the README line referencing the `result`
column on the `jobs` table: change "it's" to the possessive "its" so the
sentence reads that a failed job can store its failure reason; update the string
where the text mentions `result` and `jobs` to correct the possessive form.
- Around line 13-18: Change the "Fork changes" heading from h3 to h2 by
replacing "### Fork changes" with "## Fork changes" and fix the malformed bold
markdown by combining the split "**Procrastinate is looking for" and "additional
maintainers!**" into a single bolded phrase such as "**Procrastinate is looking
for additional maintainers!**" so the bold markers are on the same line around
the full text.
🧹 Nitpick comments (3)
procrastinate/manager.py (1)
289-323: Consider storing traceback information for better debugging.The current implementation uses
str(exc_info)which only captures the exception message. For failed jobs, having the full traceback would be more useful for debugging.Consider using
traceback.format_exception()to capture the full traceback:♻️ Suggested enhancement
+import traceback + async def finish_job_by_id_async( self, job_id: int, status: jobs_module.Status, delete_job: bool, exc_info: bool | BaseException = False, ) -> None: + result = None + if exc_info and isinstance(exc_info, BaseException): + result = "".join(traceback.format_exception(type(exc_info), exc_info, exc_info.__traceback__)) + elif exc_info: + result = str(exc_info) await self.connector.execute_query_async( query=sql.queries["finish_job"], job_id=job_id, status=status.value, delete_job=delete_job, - exc_info=str(exc_info) if exc_info else None, + exc_info=result, )procrastinate/testing.py (1)
293-308: Unusedexc_infoparameter - consider storing it for test verification.The
exc_infoparameter is added for API consistency but is not used. For testing purposes, storing it in the job row would allow tests to verify that exception information is correctly passed through the system.♻️ Suggested fix to store exc_info
async def finish_job_run( self, job_id: int, status: str, delete_job: bool, exc_info: bool | BaseException = False, ) -> None: if delete_job: self.jobs.pop(job_id) return job_row = self.jobs[job_id] job_row["status"] = status job_row["attempts"] += 1 job_row["abort_requested"] = False + job_row["result"] = str(exc_info) if exc_info else None self.events[job_id].append({"type": status, "at": utils.utcnow()})tests/unit/test_manager.py (1)
285-307: Tests updated correctly, but consider adding test for non-None exc_info.The test expectations are correctly updated to include
exc_info: None. However, there's no test coverage for whenexc_infocontains an actual exception value.🧪 Suggested additional test
async def test_finish_job_with_exception(job_manager, job_factory, connector): job = job_factory(id=1) await job_manager.defer_job_async(job=job) exc = ValueError("test error") await job_manager.finish_job( job=job, status=jobs.Status.FAILED, delete_job=False, exc_info=exc ) assert connector.queries[-1] == ( "finish_job", {"job_id": 1, "status": "failed", "delete_job": False, "exc_info": "test error"}, )
| ### Fork changes | ||
|
|
||
| 1. Added `result` column to `jobs` table so failed job can store it's failure reason | ||
|
|
||
| **Procrastinate is looking for | ||
| ** [additional maintainers!](https://github.com/procrastinate-org/procrastinate/discussions/748) |
There was a problem hiding this comment.
Fix heading level and malformed bold markdown.
Two issues:
- The heading should be
##(h2) instead of###(h3) to follow proper heading hierarchy after h1. - The bold markdown on line 17-18 is malformed - the
**markers are split across lines.
📝 Suggested fix
-### Fork changes
+## Fork changes
1. Added `result` column to `jobs` table so failed job can store it's failure reason
-**Procrastinate is looking for
-** [additional maintainers!](https://github.com/procrastinate-org/procrastinate/discussions/748)
+**Procrastinate is looking for [additional maintainers!](https://github.com/procrastinate-org/procrastinate/discussions/748)**📝 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.
| ### Fork changes | |
| 1. Added `result` column to `jobs` table so failed job can store it's failure reason | |
| **Procrastinate is looking for | |
| ** [additional maintainers!](https://github.com/procrastinate-org/procrastinate/discussions/748) | |
| ## Fork changes | |
| 1. Added `result` column to `jobs` table so failed job can store it's failure reason | |
| **Procrastinate is looking for [additional maintainers!](https://github.com/procrastinate-org/procrastinate/discussions/748)** |
🧰 Tools
🪛 LanguageTool
[style] ~18-~18: Using many exclamation marks might seem excessive (in this case: 8 exclamation marks for a text that’s 3189 characters long)
Context: ...s looking for ** [additional maintainers!](https://github.com/procrastinate-org/p...
(EN_EXCESSIVE_EXCLAMATION)
🪛 markdownlint-cli2 (0.20.0)
[warning] 13-13: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 Prompt for AI Agents
In `@README.md` around lines 13 - 18, Change the "Fork changes" heading from h3 to
h2 by replacing "### Fork changes" with "## Fork changes" and fix the malformed
bold markdown by combining the split "**Procrastinate is looking for" and
"additional maintainers!**" into a single bolded phrase such as "**Procrastinate
is looking for additional maintainers!**" so the bold markers are on the same
line around the full text.
| **Procrastinate is looking for** [additional maintainers!](https://github.com/procrastinate-org/procrastinate/discussions/748) | ||
| ### Fork changes | ||
|
|
||
| 1. Added `result` column to `jobs` table so failed job can store it's failure reason |
There was a problem hiding this comment.
Minor grammatical issue: "it's" should be "its".
"it's" is a contraction of "it is", while "its" is the possessive form needed here.
📝 Suggested fix
-1. Added `result` column to `jobs` table so failed job can store it's failure reason
+1. Added `result` column to `jobs` table so a failed job can store its failure reason📝 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.
| 1. Added `result` column to `jobs` table so failed job can store it's failure reason | |
| 1. Added `result` column to `jobs` table so a failed job can store its failure reason |
🤖 Prompt for AI Agents
In `@README.md` at line 15, Fix the grammatical typo in the README line
referencing the `result` column on the `jobs` table: change "it's" to the
possessive "its" so the sentence reads that a failed job can store its failure
reason; update the string where the text mentions `result` and `jobs` to correct
the possessive form.
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.