Repository navigation
Issue/2183/4 notice retention - #2189
Open
CarloMendola wants to merge 3 commits into
Open
CarloMendola wants to merge 3 commits into
CarloMendola wants to merge 3 commits into
Conversation
`ReportSummary` derived every number it shows from the list of notices the container retained, but `NoticeContainer` stops retaining notices of a type once `MAX_VALIDATION_NOTICES_TYPE_AND_SEVERITY` is reached while going on counting them. The HTML report then contradicted the JSON report generated from the same run: on a feed producing 200 000 `stop_too_far_from_shape` warnings, the page reported 100 000 of them where `report.json` reported 200 000, both in the total of the heading and in the per-code total of the table. The counts now come from the container, which counts every notice it is given whether or not it retains it. `NoticeContainer` exposes that number per notice code and severity, and the key those counts are stored under is built in one place instead of being spelled out twice. The template asks the summary for the count of a code instead of taking the size of the list of notices it renders. Adds the first test for `HtmlReportGenerator`, rendering a report from a container that dropped notices and asserting on the numbers the page shows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`NoticeContainer.addAll` copied every notice of the source container into the destination without applying the retention limits. `CsvFileLoader` creates one container per parsed row and merges it, so each of them held at most a handful of notices and the per-container limits never triggered: a feed where every row of stop_times.txt produces a warning (trailing whitespace or a non-ASCII character in an id, both common) retained one notice per row, growing the heap without bound. The limits are now enforced in a single place, `canRetain`, used both by `addValidationNoticeWithSeverity` and by `addAll`. Notice counts are still merged in full, so `totalNotices` in the validation report stays exact; a separate map tracks how many notices of each type and severity are actually retained. The first notice of a type and severity is retained even once the total limit is reached, because a notice type is described in the exported report only if one of its notices was retained: dropping them all would hide the type, and its exact count, from the report entirely. The defaults are lowered accordingly: at most 1000 notices per type and severity are ever exported and the HTML report renders 50, so retaining 100 000 of them was pure overhead. This is a behaviour change for callers that iterate `getValidationNotices()` expecting every notice of a pathological feed: `totalNotices` is unaffected, the retained sample is smaller. Callers needing different bounds can use the three-argument constructor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`ReportSummary` created a `NoticeView` for every retained notice, each holding the complete JSON tree of its notice, while the report lists at most 50 records per notice code. On a feed with many notices that is hundreds of megabytes built and thrown away, allocated at the end of validation when the heap is already at its peak. The views are now built only up to the limit the report lists, which is defined in one place (`ReportSummary.MAX_NOTICES_PER_CODE`) instead of being duplicated as a literal in the template. Notice counts are unaffected: the per-code total shown in the table comes from `getNoticeCountForCode`, which counts all notices, so the rendered report is unchanged. `HtmlReportGenerator.getUniqueFieldsForCodes` derives the columns of a notice table from the views of that code, so the column set is now derived from exactly the rows the report renders: a field only ever set on a record past the limit no longer gets a column of its own, which would have been empty on every rendered row anyway. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Summary:
Fourth of the six PRs #2183 is split into, in the grouping requested here. This is the group with the deliberate behaviour change, and the one you said you would want to discuss most.
Stacked on PR 1, which it needs to compile: the first commit of this diff is PR 1's and disappears from it once PR 1 merges. The two commits to review here are the last two.
1.
NoticeContainer.addAllbypassed the retention limits.addAllcopied every notice of the source container into the destination withvalidationNotices.addAll(...), applying no limits. That is not an edge case:CsvFileLoadercreates oneNoticeContainerper parsed row and merges it, so each source container holds a handful of notices and the per-container limits never trigger. A feed where every row of stop_times.txt produces a warning — trailing whitespace or a non-ASCII character in an id, both common — retains one notice per row and grows the heap without bound. TheaddAlljavadoc documented this explicitly ("the finalNoticeContainermay contain more than the maximum amount…"), so the behaviour was known; this PR makes the limits actually hold, and updates that javadoc.The limits now live in one place,
canRetain, used by bothaddValidationNoticeWithSeverityandaddAll. Two properties are preserved on purpose:totalNoticesinreport.json— and the counts the HTML report shows after PR 1 — stay exact. A second map tracks how many notices of each type and severity were actually retained.The defaults are lowered accordingly:
MAX_VALIDATION_NOTICES_TYPE_AND_SEVERITYMAX_TOTAL_VALIDATION_NOTICESMAX_EXPORTS_PER_NOTICE_TYPE_AND_SEVERITYAt most 1 000 notices per type and severity are ever exported and the HTML report lists 50, so retaining 100 000 of them was pure overhead. 2 000 keeps a margin over both.
2. Build only the notice views the report lists.
ReportSummarycreated aNoticeViewfor every retained notice, each holding the complete JSON tree of its notice, while the report lists at most 50 records per code. On a feed with many notices that is hundreds of megabytes built and thrown away, allocated at the very end of validation when the heap is already at its peak. Views are now built only up to the limit, defined once asReportSummary.MAX_NOTICES_PER_CODEand read by the template throughsummary.maxNoticesPerCodeinstead of being duplicated as the literal50in two places inreport.html.The consequence for
HtmlReportGenerator.getUniqueFieldsForCodes, as you asked. That method derives the column set of a notice table from the views of the code. With the views capped, the columns come from exactly the rows that get rendered: a field only ever set on a record past the 50th no longer gets a column — a column that, before this change, existed and wasN/Aon every rendered row. I think that is the better behaviour and the javadoc now states it, but it is a rendering difference on feeds with heterogeneous notices of one code, so it should be a conscious decision rather than a side effect. If you would rather keep the column set derived from all retained notices, that is a small change to that method and it does not affect the memory win — say the word.Implementation report for the whole series: https://github.com/CarloMendola/gtfs-validator/blob/9f409204bcefff7387f05a3f70118fb03134443b/prompts/memory-optimization/MEMORY_OPTIMIZATION_REPORT.md
Expected behavior:
The observable change: on a pathological feed, a caller that iterates
getValidationNotices()gets a smaller sample than before.totalNoticesis unaffected, the exportedsampleNoticesare unaffected (1 000 ≤ 2 000), and the HTML report is unaffected (50 listed). Callers needing different bounds can use the three-argument constructor, which is unchanged.On a normal feed nothing changes. Verified on the CTA Chicago feed (95 MB zip, 413 MB of CSV, 168 443 notices): the
noticesobject ofreport.jsonis identical to master's, because every notice type there is well below the new limits — the truncation only appears on feeds producing hundreds of thousands of notices of a single type.The rendered page is unchanged: same rows, same cells, same text (344
<tr>, 1 689<td>on both). The file does get much smaller — 6 587 188 B on master against 272 071 B on that feed — but that is whitespace, not content: for every notice skipped by the oldth:if="${iterStat.index < 50}", Thymeleaf still emitted the indentation of the iteration, 168 255 blank lines of it. Capping the list means there are no skipped iterations left.Tests:
addAll_shouldRespectMaxPerNoticeTypeAndSeverity,addAll_shouldRespectMaxTotalValidationNotices— the row-by-row merge pattern ofCsvFileLoader, reproduced.exportNoticesAfterAddAll_shouldReportExactTotalCountBeyondRetentionLimit—totalNoticesis still exact when notices were dropped, asserted on the exported JSON.addAll_shouldKeepValidationErrorAndWarningFlagsWhenNoticesAreDropped— the error/warning flags are not lost with the notices.addAll_totalLimitReached_stillRetainsTheFirstNoticeOfEachType— every type still appears in the exported report, with its exact count.ReportSummaryTest.noticesMapTest_isTruncatedButCountsAreNot— the map is capped, the counts are not.HtmlReportGeneratorTest.generateReport_listsAtMostMaxNoticesPerCodeButReportsExactTotalandgenerateReport_fewerNoticesThanTheLimit_listsThemAll— what the page shows, on both sides of the limit.Nothing under
docs/describes the retention limits, so no documentation change is needed; theaddAlljavadoc that did describe them is updated.Please make sure these boxes are checked before submitting your pull request - thanks!
gradle testto make sure you didn't break anything