Mediawiki changes #481
Mediawiki changes #481
Conversation
WalkthroughThe changes simplify workflow management for MediaWiki platforms. Workflow execution now uses a standardized ID format and always runs when the platform is activated, without storing workflow IDs. Workflow termination is also streamlined, using the platform ID to construct the workflow ID internally. Logging and document updates were removed. Changes
Sequence Diagram(s)sequenceDiagram
participant ModuleService
participant TemporalMediaWikiService
alt Platform Activated
ModuleService->>TemporalMediaWikiService: executeWorkflow(platformId)
else Platform Deactivated
ModuleService->>TemporalMediaWikiService: terminateWorkflow(platformId)
end
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/services/temporal/mediaWiki.service.ts (1)
2-2: Remove unused import.The
uuidimport is no longer needed since the workflow ID format was changed to deterministic.Apply this diff to remove the unused import:
-import { v4 as uuidv4 } from 'uuid';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/services/module.service.ts(1 hunks)src/services/temporal/mediaWiki.service.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/services/module.service.ts (2)
Learnt from: Behzad-rabiei
PR: TogetherCrew/api#350
File: src/services/module.service.ts:55-71
Timestamp: 2024-10-09T08:57:20.287Z
Learning: Error handling for the `updateModule` function in `src/services/module.service.ts` is managed by the parent function, as indicated by user Behzad-rabiei.
Learnt from: Behzad-rabiei
PR: TogetherCrew/api#350
File: src/services/module.service.ts:55-71
Timestamp: 2024-07-27T19:38:44.885Z
Learning: Error handling for the `updateModule` function in `src/services/module.service.ts` is managed by the parent function, as indicated by user Behzad-rabiei.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci / build-push / Build + Push Image
🔇 Additional comments (2)
src/services/temporal/mediaWiki.service.ts (1)
20-20: Verify deterministic workflow ID won’t block concurrent runsUsing a fixed ID format
api:mediawikietl:${platformId}means only one workflow per platform can run at a time. If your system ever needs to start multiple MediaWiki ETL workflows for the sameplatformIdin parallel (e.g., retries, backfills, overlapping triggers), they will collide.Please confirm whether you ever need concurrent workflows per platform. If so, consider one of the following:
- Append a unique suffix (UUID, timestamp) to the ID:
workflowId: `api:mediawikietl:${platformId}:${uuid()}`,- Revert to letting Temporal generate a random ID (omit
workflowId)- Use tags or memo fields to identify workflows by platform while keeping unique IDs
Location needing attention:
• src/services/temporal/mediaWiki.service.ts — line definingworkflowIdsrc/services/module.service.ts (1)
149-153: LGTM! Simplified workflow management.The simplified logic that directly calls the temporal service methods based on activation status is much cleaner than the previous implementation. The removal of workflow ID management aligns well with the standardized ID format in the temporal service.
| if (description.status.name !== 'TERMINATED' && description.status.name !== 'COMPLETED') { | ||
| await handle.terminate('Terminated due to schedule deletion'); | ||
| try { | ||
| client.workflow.getHandle(`api:mediawikietl:${platformId}`).terminate(); |
There was a problem hiding this comment.
Missing await for workflow termination.
The workflow termination is not awaited, which means the method will return before the termination completes. This could lead to inconsistent state or ineffective error handling.
Apply this diff to fix the async operation:
- client.workflow.getHandle(`api:mediawikietl:${platformId}`).terminate();
+ await client.workflow.getHandle(`api:mediawikietl:${platformId}`).terminate();📝 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.
| client.workflow.getHandle(`api:mediawikietl:${platformId}`).terminate(); | |
| await client.workflow.getHandle(`api:mediawikietl:${platformId}`).terminate(); |
🤖 Prompt for AI Agents
In src/services/temporal/mediaWiki.service.ts at line 31, the call to terminate
the workflow handle is missing an await, causing the method to return before the
termination completes. Add the await keyword before
client.workflow.getHandle(...).terminate() to ensure the termination completes
before proceeding, enabling proper error handling and consistent state.
| client.workflow.execute('MediaWikiETLWorkflow', { | ||
| taskQueue: queues.TEMPORAL_QUEUE_PYTHON_HEAVY, | ||
| args: [payload], | ||
| workflowId: `mediawiki/${platformId}/${uuidv4()}`, | ||
| workflowId: `api:mediawikietl:${platformId}`, | ||
| }); |
There was a problem hiding this comment.
Missing await for workflow execution.
The workflow execution is not awaited, which means the method will return immediately without waiting for the workflow to start. This could lead to race conditions or make error handling ineffective.
Apply this diff to fix the async operation:
- client.workflow.execute('MediaWikiETLWorkflow', {
+ await client.workflow.execute('MediaWikiETLWorkflow', {📝 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.
| client.workflow.execute('MediaWikiETLWorkflow', { | |
| taskQueue: queues.TEMPORAL_QUEUE_PYTHON_HEAVY, | |
| args: [payload], | |
| workflowId: `mediawiki/${platformId}/${uuidv4()}`, | |
| workflowId: `api:mediawikietl:${platformId}`, | |
| }); | |
| await client.workflow.execute('MediaWikiETLWorkflow', { | |
| taskQueue: queues.TEMPORAL_QUEUE_PYTHON_HEAVY, | |
| args: [payload], | |
| workflowId: `api:mediawikietl:${platformId}`, | |
| }); |
🤖 Prompt for AI Agents
In src/services/temporal/mediaWiki.service.ts around lines 17 to 21, the call to
client.workflow.execute is missing an await, causing the method to return
immediately without waiting for the workflow to start. To fix this, add the
await keyword before client.workflow.execute to ensure the workflow execution is
properly awaited and errors can be handled effectively.
| } | ||
| temporalMediaWiki.executeWorkflow(platformDoc.id); | ||
| } else if (isActivated === false) { | ||
| temporalMediaWiki.terminateWorkflow(platformDoc.id); |
There was a problem hiding this comment.
Missing await for workflow termination.
The workflow termination call is not awaited, which could lead to the function returning before the workflow is actually terminated. This may cause issues if the platform state depends on the workflow being stopped.
Apply this diff to fix the async operation:
- temporalMediaWiki.terminateWorkflow(platformDoc.id);
+ await temporalMediaWiki.terminateWorkflow(platformDoc.id);📝 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.
| temporalMediaWiki.terminateWorkflow(platformDoc.id); | |
| await temporalMediaWiki.terminateWorkflow(platformDoc.id); |
🤖 Prompt for AI Agents
In src/services/module.service.ts at line 152, the call to
temporalMediaWiki.terminateWorkflow(platformDoc.id) is missing an await, causing
the function to potentially return before the workflow termination completes.
Add the await keyword before the call to ensure the workflow termination
finishes before proceeding.
| platformDoc.set('metadata.workflowId', workflowId); | ||
| await platformDoc.save(); | ||
| } | ||
| temporalMediaWiki.executeWorkflow(platformDoc.id); |
There was a problem hiding this comment.
Missing await for workflow execution.
The workflow execution call is not awaited, which could lead to the function returning before the workflow starts. This may cause issues if subsequent operations depend on the workflow being active.
Apply this diff to fix the async operation:
- temporalMediaWiki.executeWorkflow(platformDoc.id);
+ await temporalMediaWiki.executeWorkflow(platformDoc.id);📝 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.
| temporalMediaWiki.executeWorkflow(platformDoc.id); | |
| await temporalMediaWiki.executeWorkflow(platformDoc.id); |
🤖 Prompt for AI Agents
In src/services/module.service.ts at line 150, the call to
temporalMediaWiki.executeWorkflow(platformDoc.id) is missing an await, causing
the function to potentially return before the workflow starts. Add the await
keyword before temporalMediaWiki.executeWorkflow to ensure the workflow
execution completes or starts before proceeding.
Summary by CodeRabbit