refactor: rework the download ingestion pipeline - #2132
Open
netomi wants to merge 1 commit into
Open
Conversation
This was referenced Sep 2, 2026
netomi
force-pushed
the
split/ingestion-rework
branch
from
September 2, 2026 16:09
0c8bab2 to
9ee95f2
Compare
Reworks the log-based download ingestion that PR #2027 needs, with no analytics behaviour attached, so that the restructuring of live download counting can be reviewed on its own. A log handler per storage backend becomes a DownloadRecordSource that yields RawDownloadRecords, and DownloadCountProcessor becomes DownloadIngestionProcessor, which resolves records to extensions (with a Caffeine cache), aggregates them and applies the download counts. Scheduling moves out of the handlers into DownloadIngestionRunner and the jobs package. AwsDownloadCountHandler splits into AwsDownloadRecordSource plus the CloudFront and Fastly parsers, and LogRecord becomes AccessLogRecord. StorageUtilService no longer asks a DownloadCountService whether a resource is covered; it asks the sources themselves. DownloadCountServiceAspect goes with the service it wrapped. DownloadCountProcessedItem is renamed to DownloadIngestion, keeping its table name, so there is no migration. Its repository gains countFailedDownloadIngestions. The analytics seam is present but inert: DownloadAnalyticsRepository is an interface with no implementation in this change, so the ObjectProvider the processor holds is never satisfied and only the download counts are written. The implementation, and the findSeries half of that interface, come with the analytics work. The test datasource pool is capped at 4. The new processor test adds another cached Spring context, and each one holds an idle pool of 10 against the single shared container, which exhausts its max_connections mid-suite - it surfaced as unrelated failures in RepositoryServiceSmokeTest and the recovery-lock test. Co-Authored-By: gnugomez <gomezbanaco@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
netomi
force-pushed
the
split/ingestion-rework
branch
from
September 3, 2026 14:46
d7e2400 to
3bb6f2d
Compare
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.
Stack: 3 of 6. Split out of #2027. Base: #2131 (
split/ingestion-move).This is the highest-risk part of #2027, isolated so it can be reviewed for what it actually is: a rewrite of live download counting.
Why this needed separating
#2027 presents itself as opt-in (
ovsx.analytics.enabled, default false). The analytics are. This rework is not — it replaces the download-count pipeline for every deployment regardless of that flag. That is not visible from the PR description, and it is the regression risk in the whole change.So this PR carries the restructuring with no analytics behaviour attached.
What changes
DownloadRecordSourceyieldingRawDownloadRecordsDownloadCountProcessorbecomesDownloadIngestionProcessor: resolves records to extensions (Caffeine-cached), aggregates, applies the countsDownloadIngestionRunnerand thejobspackageAwsDownloadCountHandlersplits intoAwsDownloadRecordSource+ the CloudFront and Fastly parsers;LogRecordbecomesAccessLogRecordStorageUtilServiceno longer asks aDownloadCountServicewhether a resource is covered, it asks the sources themselves;DownloadCountServiceAspectgoes with the service it wrappedDownloadCountProcessedItem->DownloadIngestion, keeping its table name, so there is no migration; its repository gainscountFailedDownloadIngestionsThe analytics seam is present but inert
DownloadAnalyticsRepositoryis here as an interface with no implementation, so theObjectProviderthe processor holds is never satisfied and only download counts are written. The single implementation anywhere in the tree is a test fake.That is what makes "did download counting regress?" an answerable question in this PR.
Two deliberate deviations from #2027 for this purpose:
save(List<DownloadEvent>). ItsfindSerieshalf drags in the whole query model (DownloadSeriesRequest,DownloadSeriesRow, and their enums), so it is added back with the Timescale implementation rather than carried here as five unused files.captureDownloadstays in the processor, guarded byanalyticsRepository.ifAvailable, rather than being removed and re-added. It does nothing until the implementation lands. Its call site on the download-serving path is worth a look when it becomes live — it is a synchronous write on the registry's hottest endpoint.One correction to an earlier concern
I had assumed the entity rename implied a table rename and therefore a data migration on the table that gates which log files have been processed. It does not:
@Table(name = "download_count_processed_item")is kept deliberately, with a comment saying so. There is no migration in #2027 at all, and that risk does not exist.The test pool cap
spring.datasource.hikari.maximum-pool-size: 4in the test config, taken from #2027's own35e5126e3.The new processor test adds another cached Spring context, and each context holds an idle pool of 10 against the one shared container, exhausting its
max_connectionsmid-suite. It surfaced as two failures with no apparent connection to this change —RepositoryServiceSmokeTest.testExecuteQueriesand the recovery-lock test — both of which pass in isolation.Worth noting for the Timescale PR that follows: this is a test-infra symptom of an unanswered production question. A second datasource means two pools per deployment, and the combined connection budget against the deployed
max_connectionshas not been stated anywhere.Verification
Full server suite: 1177 tests passing.
Refs #2027, #2025