Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
📝 WalkthroughWalkthroughThis PR introduces a configurable default execution mode system across Dagu. Users can set a server-wide Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 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: 2
🤖 Fix all issues with AI agents
In `@internal/core/spec/dag.go`:
- Around line 916-973: buildWorkerSelector currently rejects scalar YAML values
in the map[string]any and map[any]any cases instead of coercing them to strings;
update buildWorkerSelector to convert non-string scalar values using fmt.Sprint
before trimming (for the case map[string]any, if val is not a string call
fmt.Sprint(val) and proceed, and for map[any]any after validating/obtaining
strKey similarly convert non-string val with fmt.Sprint), preserving existing
error handling for non-string keys in map[any]any and keeping the "local" string
behavior unchanged; reference function name buildWorkerSelector and the
map[string]any / map[any]any switch cases when making the change.
In `@rfcs/010-immediate-execution-worker-selector.md`:
- Around line 55-57: The fenced code block containing
"DAGU_DEFAULT_EXECUTION_MODE=distributed" is missing a language identifier;
update the markdown block around that line to include a language hint (e.g.,
"sh" or "bash") so the block becomes a shell snippet (add the identifier after
the opening backticks) to satisfy MD040 linting.
🧹 Nitpick comments (4)
internal/service/worker/handler_test.go (1)
7-7: Consider a clearer alias for the standardruntimepackage.
osrtis not immediately recognizable as the Go standard libraryruntimepackage. A more descriptive alias likegoruntimeorstdruntimewould improve readability at the single usage site (Line 201).♻️ Suggested rename
- osrt "runtime" + goruntime "runtime"And at line 201:
- if osrt.GOOS == "windows" { + if goruntime.GOOS == "windows" {internal/core/dispatch_test.go (1)
10-61: Well-structured table-driven test covering key dispatch scenarios.Follows the project's testing conventions. Consider adding a case for an empty/unknown
ExecutionModestring to verify the function defaults safely (even if config loading prevents it, defense-in-depth is useful for a public function).Optional: additional edge case
+ { + name: "unknown defaultMode — treated as local", + dag: &DAG{}, + hasCoordinator: true, + defaultMode: config.ExecutionMode("unknown"), + want: false, + },internal/service/frontend/api/v1/dags.go (2)
919-962: Duplicated timeout computation in local vs. distributed paths.Lines 922–925 and 948–951 repeat the identical GOOS-based timeout logic. Consider extracting a small helper to reduce duplication and ensure both paths stay in sync.
♻️ Suggested helper
+func startTimeout() time.Duration { + if osrt.GOOS == "windows" { + return 10 * time.Second + } + return 5 * time.Second +} + func (a *API) startDAGRunWithOptions(ctx context.Context, dag *core.DAG, opts startDAGRunOptions) error { // Check if this DAG should be dispatched to the coordinator for distributed execution if core.ShouldDispatchToCoordinator(dag, a.coordinatorCli != nil, a.defaultExecMode) { - timeout := 5 * time.Second - if osrt.GOOS == "windows" { - timeout = 10 * time.Second - } - return a.dispatchStartToCoordinator(ctx, dag, opts.dagRunID, timeout) + return a.dispatchStartToCoordinator(ctx, dag, opts.dagRunID, startTimeout()) } ... - timeout := 5 * time.Second - if osrt.GOOS == "windows" { - timeout = 10 * time.Second - } + timeout := startTimeout()
888-917: Coordinator dispatch path looks correct, but consider the timeout adequacy.The
dispatchStartToCoordinatormethod dispatches to the coordinator and waits up to 5s (10s on Windows) for the DAG status to transition fromNotStarted. In a distributed setup, the coordinator must select a worker, forward the task, and the worker must begin execution — all within that window. If the coordinator or worker pool is under load, this timeout may be too aggressive, resulting in a spurious HTTP 500 even though the DAG will eventually start.If this is acceptable for now (e.g., coordinator dispatch is expected to be fast), a short comment documenting the assumption would help future readers.
|
Should we hide the distributed execution related UI elements based on this? |
|
@ghansham No, I don't think so. The setting is just default, so distributed execution can be used even |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1642 +/- ##
==========================================
- Coverage 69.83% 69.46% -0.38%
==========================================
Files 332 333 +1
Lines 37327 37409 +82
==========================================
- Hits 26068 25986 -82
- Misses 9188 9223 +35
- Partials 2071 2200 +129
... and 9 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Yeah Later I realized. It is default mode |
2 participants
Summary by CodeRabbit
workerSelector: localin DAG specifications