feat: Support non-error reconcile status#9031
Conversation
k-anshul
force-pushed
the
non_error_reconcile_status
branch
from
eaadd74 to
1fb42b0
Compare
| var testWarnings []string | ||
| if model.State.TestHash != testHash { | ||
| testErrs, err := r.runModelTests(ctx, self) | ||
| var testErrs []string | ||
| var err error | ||
| testErrs, testWarnings, err = r.runModelTests(ctx, self) |
There was a problem hiding this comment.
Since we have model.State.TestErrors, I think it might be cleaner to also keep model.State.TestWarnings. The reason we added TestErrors was so it could return the same errors even after another reconcile (e.g. a restart) without re-running the tests. I think that problem may also apply here if we don't add TestWarnings to the state?
There was a problem hiding this comment.
Actually, as I think about this, I would probably keep it simple and just output warnings as part of testErrs (i.e. not return two different slices here). Tests are a rarely used feature, so I think it's okay to have strict enforcement on them (i.e. to treat any warning as a test error), especially given you can configure whether test results become errors or warnings.
There was a problem hiding this comment.
I ended up storing TestWarnings. This is to ensure that test runs on non-triggers do not duplicate same warnings.
| // Surface partition and test failures as warnings or errors based on instance config | ||
| var warnings []string | ||
| warnings = append(warnings, testWarnings...) | ||
| if model.State.PartitionsHaveErrors { |
There was a problem hiding this comment.
A related problem is that the some partitions have errors error is not very useful. It would be nice if we could return Partition <id> failed with error: %w, and if multiple partitions failed, return such an error for the first, and return e.g. 9 other partitions also failed with errors.
Would that be easy to fit in here? Warnings might make it easier to return such lists.
Anyway, it's probably better to do this in a separate PR.
There was a problem hiding this comment.
Sure. Will do it separately.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| // We have continuously updated prevResult with new partition results, so we complete and return it here | ||
| prevResult.ExecDuration = time.Duration(totalExecDuration.Load()) | ||
| prevResult.Warnings = append(prevResult.Warnings, syncWarnings...) |
There was a problem hiding this comment.
Isn't prevResult is re-used from previous runs? Worried this might cause it to grow infinitely.
There was a problem hiding this comment.
The prevResult is manually constructed.
prevResult = &drivers.ModelResult{
Connector: model.State.ResultConnector,
Properties: model.State.ResultProperties.AsMap(),
Table: model.State.ResultTable,
}
I added warnings: nil to be explicit.
|
|
||
| // We have continuously updated prevResult with new partition results, so we complete and return it here | ||
| prevResult.ExecDuration = time.Duration(totalExecDuration.Load()) | ||
| prevResult.Warnings = append(prevResult.Warnings, syncWarnings...) |
There was a problem hiding this comment.
So if the warnings are not persisted, and not handled by drivers.ModelManager, then isn't it a little weird to pass the warnings as part of drivers.ModelResult? Should they perhaps be returned separately then?
There was a problem hiding this comment.
The godoc on ModelResult says:
// ModelResult contains metadata about the result of a model execution.
Since warnings is also a metadata so can be added here. We also pass ExecDuration as part of Result which is also not peristed.
But if you prefer this can be returned separately as well.
Co-authored-by: Benjamin Egelund-Müller <b@egelund-muller.com>
3 participants
closes https://linear.app/rilldata/issue/PLAT-130/support-non-error-reconcile-status-info-warning
Disclaimer: The frontend code is vibe coded and not reviewed fully.
Checklist: