Add stopped state support for pipeline status and stop/cancel AI agents#27098
Add stopped state support for pipeline status and stop/cancel AI agents#27098Vishnuujain wants to merge 14 commits intomainfrom
Conversation
Vishnuujain
added
safe to test
|
The Java checkstyle failed. Please run You can install the pre-commit hooks with |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
openmetadata-service/src/main/java/org/openmetadata/service/resources/apps/AppResource.java
Outdated
Show resolved
Hide resolved
✅ TypeScript Types Auto-UpdatedThe generated TypeScript types have been automatically updated based on JSON schema changes in this PR. |
Jest test CoverageUI tests summary
|
openmetadata-service/src/main/java/org/openmetadata/service/resources/apps/AppResource.java
Outdated
Show resolved
Hide resolved
openmetadata-service/src/main/java/org/openmetadata/service/resources/apps/AppResource.java
Outdated
Show resolved
Hide resolved
🔴 Playwright Results — 2 failure(s), 29 flaky✅ 3590 passed · ❌ 2 failed · 🟡 29 flaky · ⏭️ 207 skipped
Genuine Failures (failed on all attempts)❌
|
Vishnuujain
force-pushed
the
feature/support-stop-cancel-ai-agents
branch
from
3bda980 to
d562cc4
Compare
...tadata-service/src/main/java/org/openmetadata/service/jdbi3/IngestionPipelineRepository.java
Show resolved
Hide resolved
| {record.status !== Status.Success && | ||
| record.status !== Status.Failed && | ||
| record.status !== Status.Stopped && | ||
| record.status !== Status.Completed && | ||
| record.status !== Status.StopInProgress && |
There was a problem hiding this comment.
What's the reason for changing the assertions to negative conditions?
There was a problem hiding this comment.
actually initially I had positive one only but I missed some of them to include like when in queue so stop button was not appearing, therefore I used negative assertion to ensure the button appears in all cases except these.
openmetadata-spec/src/main/java/org/openmetadata/sdk/PipelineServiceClientInterface.java
Show resolved
Hide resolved
| default PipelineServiceClientResponse killIngestionRun( | ||
| IngestionPipeline ingestionPipeline, String runId) { | ||
| return new PipelineServiceClientResponse().withCode(200).withPlatform(getPlatform()); | ||
| } |
There was a problem hiding this comment.
⚠️ Bug: killIngestionRun default no-op marks DB STOPPED but never kills
The killIngestionRun default method in PipelineServiceClientInterface returns a 200 success response without actually stopping anything. None of the concrete pipeline service clients (AirflowRESTClient, K8sPipelineClient, etc.) override this method. Combined with the fact that stopSpecificRun marks the DB status as STOPPED before calling killIngestionRun, this means:
- The pipeline status will show STOPPED in the database
- The actual workflow continues running in the pipeline service
- The API returns 200 success to the user
This creates a silent inconsistency where the UI shows the run as stopped but it's still executing. The same issue applies to getIngestionLogs which silently ignores the runId parameter and returns latest logs instead of per-run logs.
At minimum, killIngestionRun should throw UnsupportedOperationException or return a non-200 response so callers know the feature isn't supported, rather than silently pretending it succeeded. Alternatively, the concrete clients should implement these methods.
Suggested fix:
// Option A: Make it clear the operation is unsupported
default PipelineServiceClientResponse killIngestionRun(
IngestionPipeline ingestionPipeline, String runId) {
throw new UnsupportedOperationException(
"Per-run stop is not supported by " + getPlatform());
}
// Option B: Return a distinguishable response
default PipelineServiceClientResponse killIngestionRun(
IngestionPipeline ingestionPipeline, String runId) {
return new PipelineServiceClientResponse()
.withCode(501).withPlatform(getPlatform())
.withReason("Per-run stop not supported");
}
Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion
openmetadata-service/src/main/java/org/openmetadata/service/resources/apps/AppResource.java
Outdated
Show resolved
Hide resolved
openmetadata-service/src/main/java/org/openmetadata/service/resources/apps/AppResource.java
Outdated
Show resolved
Hide resolved
| uriInfo, pipelineRef.getId(), ingestionPipelineRepository.getFields(FIELD_OWNERS)); | ||
| return Response.ok( | ||
| pipelineServiceClient.getLastIngestionLogs(ingestionPipeline, after), | ||
| pipelineServiceClient.getIngestionLogs(ingestionPipeline, after, runId), |
There was a problem hiding this comment.
is this backward compatible? would it pick the last logs if runId is null?
There was a problem hiding this comment.
yes, it's backward comatible
| } | ||
| } | ||
|
|
||
| private void markLatestPipelineStatusAsStopped( |
There was a problem hiding this comment.
shouldnt this implementation be based on markPipelineStatusAsStopped but passing the last runId? Otherwise seems we're duplicating logic
There was a problem hiding this comment.
refactored it, thanks
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Gitar
|
|
Describe your changes:
Fixes: https://github.com/open-metadata/openmetadata-collate/issues/3456
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>Summary by Gitar
stoppedstate support to pipeline status enum iningestionPipeline.tsThis will update automatically on new commits.