Repository navigation
[GOBBLIN-2270] Add retained-state fallback for KILL actions - #4208
Merged
Blazer-007 merged 3 commits intoSep 24, 2026
Merged
Conversation
Keep active-DAG cancellation unchanged and allow missing-DAG KILL actions to delegate to an optional backend using retained job states. Preserve opaque execution metadata, scope it to the requested flow and job, and recheck for active DAGs during fallback. Add handler configuration and lifecycle hooks with focused unit coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Resolve the two moderate issues involving null state stores and defensive state copying.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Adds a configurable retained-state fallback for KILL actions when the active DAG is unavailable.
Changes:
- Introduces
ForceKillHandlerconfiguration and lifecycle management. - Adds raw retained-state retrieval and execution-scoped MySQL lookups.
- Adds fallback validation, race handling, and comprehensive tests.
- Outstanding issues: handle null
StateStoreinstances and defensively copy retained state before invoking handlers.
| File | Summary |
|---|---|
gobblin-service/src/test/java/org/apache/gobblin/service/monitoring/RawJobStatusRetrieverTest.java |
Tests raw metadata retrieval, scoping, and read failures. |
gobblin-service/src/test/java/org/apache/gobblin/service/modules/orchestration/proc/KillDagProcForceKillTest.java |
Tests fallback behavior, scoping, races, and failures. |
gobblin-service/src/test/java/org/apache/gobblin/service/modules/orchestration/DagProcFactoryForceKillTest.java |
Tests optional handler injection and legacy constructors. |
gobblin-service/src/test/java/org/apache/gobblin/service/modules/core/GobblinServiceForceKillLifecycleTest.java |
Tests handler lifecycle cleanup. |
gobblin-service/src/test/java/org/apache/gobblin/service/modules/core/GobblinServiceForceKillConfigurationTest.java |
Tests optional configuration and singleton binding. |
gobblin-service/src/main/java/org/apache/gobblin/service/monitoring/MysqlJobStatusRetriever.java |
Retrieves retained raw state from MySQL. |
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/proc/KillDagProc.java |
Implements retained-state force-kill fallback; state copying requires correction. |
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/ForceKillHandler.java |
Defines the backend force-kill contract. |
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagProcFactory.java |
Injects optional force-kill dependencies. |
gobblin-service/src/main/java/org/apache/gobblin/service/modules/core/GobblinServiceManager.java |
Manages handler lifecycle. |
gobblin-service/src/main/java/org/apache/gobblin/service/modules/core/GobblinServiceGuiceModule.java |
Configures optional singleton binding. |
gobblin-runtime/src/main/java/org/apache/gobblin/service/monitoring/JobStatusRetriever.java |
Adds raw-state retrieval; null StateStore handling requires correction. |
gobblin-metastore/src/test/java/org/apache/gobblin/metastore/MysqlJobStatusStateStoreFlowScopeTest.java |
Tests execution-ID delimiter lookup. |
gobblin-metastore/src/main/java/org/apache/gobblin/metastore/MysqlJobStatusStateStore.java |
Prevents execution-ID prefix collisions. |
gobblin-api/src/main/java/org/apache/gobblin/service/ServiceConfigKeys.java |
Adds the force-kill handler configuration key. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Siddhi-agg
marked this pull request as draft
September 22, 2026 08:03
Declare the metastore Mockito test dependency so the CI test and static-check jobs can compile their shared test fixtures. Reject raw-state reads with an IOException when the retriever has no state store. Snapshot merged properties before invoking the kill handler so neither common nor specific properties share mutable maps. Cover unsupported local-file retrieval and property isolation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
Pushed 9c72ce8 to address the CI failures and review feedback:
The new workflow is awaiting maintainer approval before any jobs can run: https://github.com/apache/gobblin/actions/runs/35818045073 . Please approve that run. Local targeted execution is blocked before compilation by a Gradle/Groovy ReflectionCache initialization error; no passing result is claimed. The previously identified standalone handler-lifetime and truncated-state decoding concerns remain outside this follow-up. |
Siddhi-agg
changed the title
Siddhi-agg
marked this pull request as ready for review
September 23, 2026 04:53
Blazer-007
reviewed
Sep 24, 2026
…N_SERVICE_FORCE_KILL_HANDLER_CLASS_KEY
Blazer-007
approved these changes
Sep 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Dear Gobblin maintainers,
Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!
JIRA
Description
The existing KILL path requires an active DAG to locate the executor and its submission handle. Once the DAG has been removed, KILL reports that the DAG was not found, even if an external execution or launcher is still running.
Retained job status can contain the execution metadata needed to address that remaining execution. However, the existing status retrieval APIs convert stored State objects into JobStatus objects, which do not preserve arbitrary backend-specific metadata.
Changes
gobblin.service.forceKillHandler.class, using an optional singleton Guice binding and service lifecycle hooks.1234does not also match12340.Tests
My PR adds the following unit tests:
KillDagProcForceKillTest: active-DAG compatibility, retained-state scoping, metadata preservation, DAG reappearance, and failure outcomes.DagProcFactoryForceKillTest: optional binding, lazy provider access, and legacy constructor behavior.RawJobStatusRetrieverTest: raw metadata retrieval, execution scoping, and read-error propagation.MysqlJobStatusStateStoreFlowScopeTest: execution-ID delimiter in the MySQL lookup.GobblinServiceForceKillConfigurationTest: optional configuration and singleton binding.GobblinServiceForceKillLifecycleTest: backend cleanup paths.Commits