Repository navigation
Conversation
xiangfu0
added
release-notes
Jackie-Jiang
left a comment
Jackie-Jiang
left a comment
There was a problem hiding this comment.
Reviewed all 11 changed files at fa60242. Leaving two findings as a non-blocking review: one P1 for empty consistent REFRESH behavior and one P2 for nullable output-directory handling. Static review only; no local tests run.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #19947 +/- ##
=========================================
Coverage 69.82% 69.83%
Complexity 1519 1519
=========================================
Files 3558 3558
Lines 235843 235859 +16
Branches 37848 37851 +3
=========================================
+ Hits 164678 164705 +27
+ Misses 58464 58449 -15
- Partials 12701 12705 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
fa60242 to
c411786
Compare
xiangfu0
left a comment
xiangfu0
left a comment
There was a problem hiding this comment.
Review: Reject duplicate segment names before batch push
I reviewed the duplicate-detection logic, the empty-batch / consistent-push paths, error messaging, and push back-compat across the Hadoop, Spark, and SegmentPushUtils changes. Overall this looks solid and well tested: the duplicate check runs on the full un-deduped path list (so same-named archives in different directories are caught even though the URI map would otherwise hide one), empty REFRESH snapshots still replace, and the data-provider / expectThrows tests cover the key cases.
A couple of minor, non-blocking observations are inline. Decision: this is an approve on the merits -- the only remaining items are minor. (Submitting as a review comment rather than a formal Approve because GitHub blocks approving from this account, which it treats as the PR owner.)
c411786 to
14be3d6
Compare
xiangfu0
left a comment
xiangfu0
left a comment
There was a problem hiding this comment.
Reviewed correctness, back-compat, and coverage; built and ran the touched unit tests (SegmentPushUtilsTest, 18/18 green). The change looks good — rejecting duplicate segment names before any HTTP/replacement starts is a real improvement, and routing the Hadoop/Spark runners through getSegmentUriToTarPathMap is a clean consolidation that aligns them with the standard push runner. Only minor/nit observations below, nothing blocking.
(Posting as a comment rather than an approval since GitHub blocks approving your own PR.)
14be3d6 to
a8dbb60
Compare
Fail before upload or replacement when selected archives share a segment name. Apply the push pattern first so excluded archives do not cause false conflicts.
Empty consistent REFRESH snapshots still complete replacement, and METADATA uploads accept an omitted output directory with a URI prefix. Reproduce with empty REFRESH output or a local METADATA push using only push.segmentUriPrefix.
Reproduce by mapping an archive without an output directory or URI prefix; the missing directory caused an NPE. Keep the input URI unchanged and cover mixed-codec duplicate rejection.
Reproduce by querying WindowFunnelTest while a newly uploaded segment is still optional: MSE can return 36 instead of 40 groups. Wait for all fixture segments in the broker required-routing view before running assertions.
a8dbb60 to
6208e67
Compare
|
Closing to keep this work focused on loading performance. Duplicate-name rejection is a separate correctness change and is not required by the performance improvements. |
Backward incompatible: Batch pushes reject selected archives with the same logical segment name before replacement or upload, preventing arbitrary overwrites from recursive output directories. This applies across gzip, Zstd, and LZ4. Selected names also enforce the existing illegal-character validation (
\ / : * ? " < > |).Reproduce with
segment.tar.gzand2022/segment.tar.gzunder one output directory: the push reports the first conflicting pair. Remove stale output or select one cohort withpushFileNamePattern: "glob:**/2022/*.tar.gz". After rebasing onto master with #19898, Hadoop/Spark use the shared selection rules: honorpushFileNamePatternand skip.metadata.tar.gzsidecars.Spark 3 metadata pushes preserve empty consistent REFRESH replacement without dispatching uploads. An omitted output directory now preserves the supplied segment URI when no prefix/suffix is configured; prefix rewriting remains supported. Shared offline fixtures now wait for every generated segment in the broker required-routing view: SSE row-count readiness can pass while MSE still omits optional segments, matching the recurring WindowFunnelTest 36/40-group failure. Validation: 74 targeted JDK 25 reactor tests (including all 38 WindowFunnelTest cases), affected-module formatting/license/style checks, and independent code review passed.
PR flow
Batch push now lists output files, maps them to segment URIs/tar paths with duplicate name validation, then pushes tar, metadata, or URIs accordingly.
AI-generated · Green: added · Yellow: modified · Red: removed · Gray: existing
Diff evidence
Current hosted validation
Current head
6208e67e9d2epassed all 16 hosted checks. Completed Unit workflow 40053061761 and Integration workflow 40053061800 logs confirm 74 relevant invocations with no failures, errors or skips: 29 segment-push, three duplicate-output Spark, four Spark3 empty-output and 38 WindowFunnel ingestion cases. The tested checkout tree matches this PR head exactly.Independent review covered all eight changed files, including URI handling and selected archive-name identity across common/Spark/minion push paths. All seven review threads are answered and resolved. No new source change was needed for this readiness check; formal maintainer approval remains separate.