YuriyKrasilnikov commented on PR #37516:
URL: https://github.com/apache/superset/pull/37516#issuecomment-4966701282
@rusackas, I pushed one more hardening pass after an independent
architectural
review of the execution and timing contracts.
The review found one real timing-composition bug in the new implementation.
When a source collected by one `SourceTimingCollector` ran inside an active
phase
owned by another collector, the outer phase continued measuring the nested
source.
Attaching the nested trace then counted the same duration twice. Besides
making the
public breakdown misleading, this could make a cache-serialized source tree
fail
its own additive-accounting validation when read back.
The fix is at the collector boundary rather than at individual call sites:
- collector activation is now an exception-safe context manager;
- a root source pauses active phases across the complete chain of outer
collectors,
including metadata collectors that do not own a source frame themselves;
- only the phase frames actually paused by the nested source are resumed on
exit,
including exceptional and trace-truncation paths;
- historical `origin: cache` traces remain top-level acquisition roots and
are not
attached below current-execution sources, because child durations are
additive;
- dataframe and metadata consumers use the same activation contract.
Regression coverage exercises separate collectors, a transparent intermediate
collector, exception restoration, trace truncation, cache-origin attachment,
and a
real metadata render followed by source serialization/deserialization. I
also added
the missing contribution-DAG case: if an explicit totals producer returns
only a
subset of the columns requested by the consumer, the consumer fails closed
and is
not executed.
The same pass tightens two internal contracts without changing the API: chart
request phases are now a typed `Literal` set, datasource providers use
`DatasourceType`, and the write-only `deferred` fields were removed while
retaining
the `defer_source_queries` execution path used to build executable previews.
I rechecked the latest CodeAnt and Bito comments against the resulting code.
The
useful findings are covered by the pushed commits. I did not apply the
suggestion to
treat every non-`SUCCEEDED` cache-write state as an async failure:
`cache_hit=True`
is a valid successful cache-only outcome and must not require another write.
The
latest Bito note about mock variable naming was not a Python mock
requirement, but
the test now uses the explicit `mock_execute` name and the assertion is
reachable.
Verification after the hardening pass:
- 98 focused timing/API/command tests passed;
- 231 processor, semantic-mapper, and streaming-export tests passed;
- both executable series-limit integration tests passed;
- the final timing regression rerun passed at 49 tests;
- repository MyPy, full TypeScript, Prettier, Oxlint, Stylelint, docs ESLint,
metadata validation, feature-flag sync, Zizmor, and direct Helm generation
passed;
- changed-file Ruff and Pylint passed, with Pylint at 10/10.
The repository-wide Ruff/Pylint hooks still report only the unrelated
existing
`execute_sql.py` C901 and migration W9001 findings.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
