#742 Add 'number_of_tasks_completed' field to the 'executions' table.#743
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThe PR adds an optional ChangesExecution completion tracking
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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.
🧹 Nitpick comments (2)
pramen/core/src/main/scala/za/co/absa/pramen/core/journal/JournalDynamoDB.scala (1)
401-401: ⚡ Quick winMove constant to the executions table attributes section.
ATTR_NUMBER_OF_TASKS_COMPLETEDis defined in the journal table attributes section (lines 385-408) but is actually used for the executions table at line 151. For consistency with the code organization, this constant should be moved to the executions table section (lines 358-383), likely afterATTR_EXEC_FAILURE_REASONat line 379.♻️ Suggested placement
Move line 401 to appear after line 379 in the executions attributes section:
val ATTR_EXEC_FAILURE_REASON = "failure_reason" +val ATTR_NUMBER_OF_TASKS_COMPLETED = "number_of_tasks_completed" val ATTR_EXEC_NUMBER_OR_RECORDS_INGESTED = "number_of_records_ingested"And remove from line 401 in the journal attributes section.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pramen/core/src/main/scala/za/co/absa/pramen/core/journal/JournalDynamoDB.scala` at line 401, ATTR_NUMBER_OF_TASKS_COMPLETED is declared in the journal table attributes block but is used by the executions table; move the constant declaration to the executions table attributes section and remove it from the journal section — place the declaration after ATTR_EXEC_FAILURE_REASON in the executions attributes block so it's grouped with other execution-related attribute constants.pramen/core/src/main/scala/za/co/absa/pramen/core/state/PipelineStateImpl.scala (1)
354-361: 💤 Low valueConsider simplifying the counting logic.
The double-negation pattern (
isSkippedthen!isSkipped) makes this code harder to follow.♻️ Simpler alternative
- val notSkippedTasks = taskResults.count { task => - val isSkipped = task.runStatus match { - case _: RunStatus.Succeeded => false - case _: RunStatus.Failed => false - case _ => true - } - !isSkipped - } + val notSkippedTasks = taskResults.count { task => + task.runStatus match { + case _: RunStatus.Succeeded | _: RunStatus.Failed => true + case _ => false + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pramen/core/src/main/scala/za/co/absa/pramen/core/state/PipelineStateImpl.scala` around lines 354 - 361, The counting logic for notSkippedTasks uses a confusing double-negation; replace it by directly counting tasks whose runStatus is a RunStatus.Succeeded or RunStatus.Failed. In other words, update the expression that computes notSkippedTasks (which currently inspects taskResults and matches on task.runStatus) to match only the two concrete cases (RunStatus.Succeeded and RunStatus.Failed) and increment the count for those, removing the intermediate isSkipped boolean and the negation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@pramen/core/src/main/scala/za/co/absa/pramen/core/journal/JournalDynamoDB.scala`:
- Line 401: ATTR_NUMBER_OF_TASKS_COMPLETED is declared in the journal table
attributes block but is used by the executions table; move the constant
declaration to the executions table attributes section and remove it from the
journal section — place the declaration after ATTR_EXEC_FAILURE_REASON in the
executions attributes block so it's grouped with other execution-related
attribute constants.
In
`@pramen/core/src/main/scala/za/co/absa/pramen/core/state/PipelineStateImpl.scala`:
- Around line 354-361: The counting logic for notSkippedTasks uses a confusing
double-negation; replace it by directly counting tasks whose runStatus is a
RunStatus.Succeeded or RunStatus.Failed. In other words, update the expression
that computes notSkippedTasks (which currently inspects taskResults and matches
on task.runStatus) to match only the two concrete cases (RunStatus.Succeeded and
RunStatus.Failed) and increment the count for those, removing the intermediate
isSkipped boolean and the negation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2ade901f-29fd-4f48-b28d-e890ea6c96d2
📒 Files selected for processing (5)
pramen/core/src/main/scala/za/co/absa/pramen/core/journal/JournalDynamoDB.scalapramen/core/src/main/scala/za/co/absa/pramen/core/journal/model/Execution.scalapramen/core/src/main/scala/za/co/absa/pramen/core/journal/model/ExecutionsTable.scalapramen/core/src/main/scala/za/co/absa/pramen/core/rdb/PramenDb.scalapramen/core/src/main/scala/za/co/absa/pramen/core/state/PipelineStateImpl.scala
Unit Test Coverage
Files
|
1 participant
Closes #742
Summary by CodeRabbit
New Features