TEZ-4733: Fix flaky TestHistoryParser.testParserWithSuccessfulJob - #519
Open
maheshrajus wants to merge 3 commits into
Open
TEZ-4733: Fix flaky TestHistoryParser.testParserWithSuccessfulJob#519maheshrajus wants to merge 3 commits into
maheshrajus wants to merge 3 commits into
Conversation
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
Author
|
@abstractdog Could you please review and approve the PR at your convenience? |
abstractdog
requested changes
Jul 29, 2026
Comment on lines
+109
to
+110
| private record ZipContent(String name, String payload) { | ||
| } |
Contributor
There was a problem hiding this comment.
what is the record for?
Contributor
Author
There was a problem hiding this comment.
@abstractdog
It's just a lightweight data holder for writeZip(...) that lets each test declare its entries inline as (name, payload) pairs.
I used a record since it's pure data with no behavior, but I'm happy to switch to a per-entry writeEntry(zos, name, payload) helper if we'd prefer not to introduce a new type.
|
🎊 +1 overall
This message was automatically generated. |
Contributor
Author
|
@abstractdog Fixed your review comments. Could you please check and approve the PR at your convenience? |
Contributor
Author
|
@abstractdog Could you please check and approve the PR at your convenience? Thank you ! |
There was a problem hiding this comment.
Pull request overview
Improves history parser resilience against incomplete asynchronous history writes.
Changes:
- Skips blank ATS zip entries and improves malformed JSON diagnostics.
- Adds ATS retry and SimpleHistory readiness polling.
- Adds ATS parser regression tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
TestHistoryParser.java |
Adds retry and readiness helpers. |
TestATSFileParser.java |
Tests blank and malformed entries. |
ATSFileParser.java |
Handles blank entries and enriches errors. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
tez-plugins/tez-history-parser/src/test/java/org/apache/tez/history/TestHistoryParser.java:296
- This can stop retrying on the same partial ATS snapshot the change is meant to avoid. ATS creates vertex, task, and attempt entities from their STARTED events, so all of these collections can already be non-empty while the FINISHED updates (status, finish times, counters, and successful-attempt data used by the assertions below) are still queued. Those assertions run after this method returns and therefore fail immediately rather than triggering another attempt. Require the successful terminal state and finish data for the DAG and every nested entity before accepting the snapshot.
&& info.getVertices().size() >= expectedNumOfVertices
&& info.getVertices().stream().allMatch(v ->
!v.getTasks().isEmpty()
&& v.getTasks().stream().allMatch(t -> !t.getTaskAttempts().isEmpty()));
|
🎊 +1 overall
This message was automatically generated. |
Contributor
Author
|
@abstractdog I fixed all review comments. |
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.
Problem
org.apache.tez.history.TestHistoryParser.testParserWithSuccessfulJob fails intermittently with:
Root cause
After the DAG client returns, ATSHistoryLoggingService still has history events in an async queue that must be
flushed to the timeline server. The test previously called ATSImportTool.process(...) immediately, so under load the
download could race the timeline write path — producing a zip whose entries were empty/whitespace, which then failed JSON parsing with the misleading "must begin with {" error.
A fixed Thread.sleep(10000) was already present before the SimpleHistory parse path (as a workaround for the same class of race), but there was no equivalent guard for the ATS parse path.
Changes
- skip empty/whitespace zip entries with a WARN; enrich JSON parse errors with the offending entry name + payload snippet.
-replaced the unguarded ATS export+parse with a retry loop that only accepts a DagInfo with ≥2
vertices, tasks, and attempts.
-replaced the fixed Thread.sleep(10000) before SimpleHistory parse with a
poll-until-file-size-stable.
-New tests added that checks (empty entry skipped, malformed entry names itself in the error).