Repository navigation
feat: backfill download events from access logs - #2228
Merged
Merged
Conversation
gnugomez
force-pushed
the
gnugomez/main/backfill-analytics-data
branch
from
September 21, 2026 07:09
3b456f8 to
d536998
Compare
gnugomez
force-pushed
the
gnugomez/main/backfill-analytics-data
branch
from
September 21, 2026 10:44
ba62b7e to
8ecdf6b
Compare
gnugomez
force-pushed
the
gnugomez/main/backfill-analytics-data
branch
from
September 21, 2026 13:39
8ecdf6b to
1f6f80f
Compare
gnugomez
marked this pull request as ready for review
September 21, 2026 13:43
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical idempotency, resource-lifetime, and unbounded-input issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 4
Open (4)
What changed in this PR
Adds an admin API and processing pipeline to backfill download analytics from access logs.
Changes:
- Adds log parsing, gzip support, and analytics backfill processing.
- Adds continuous aggregate refresh and repository wiring.
- Adds comprehensive endpoint, parser, processor, and integration tests.
| File | Description |
|---|---|
server/src/test/java/org/eclipse/openvsx/repositories/RepositoryServiceSmokeTest.java |
Repository smoke-test coverage |
server/src/test/java/org/eclipse/openvsx/analytics/ingestion/jobs/IngestionJobsTest.java |
Ingestion job wiring tests |
server/src/test/java/org/eclipse/openvsx/analytics/ingestion/DownloadIngestionProcessorTest.java |
Backfill processor tests |
server/src/test/java/org/eclipse/openvsx/analytics/ingestion/DownloadBackfillAPITest.java |
Backfill API tests |
server/src/test/java/org/eclipse/openvsx/analytics/ingestion/aws/DownloadLogParserTest.java |
Access-log parser tests |
server/src/test/java/org/eclipse/openvsx/analytics/ContinuousAggregateRefresherTest.java |
Aggregate refresh tests |
server/src/main/java/org/eclipse/openvsx/repositories/RepositoryService.java |
Repository query delegation |
server/src/main/java/org/eclipse/openvsx/repositories/DownloadIngestionRepository.java |
Processed-ingestion query |
server/src/main/java/org/eclipse/openvsx/analytics/ingestion/DownloadIngestionProcessor.java |
Analytics backfill persistence |
server/src/main/java/org/eclipse/openvsx/analytics/ingestion/DownloadBackfillAPI.java |
Admin backfill endpoint |
server/src/main/java/org/eclipse/openvsx/analytics/ingestion/BackfillResultJson.java |
Backfill response DTO |
server/src/main/java/org/eclipse/openvsx/analytics/ingestion/aws/DownloadLogParser.java |
Access-log parsing and gzip handling |
server/src/main/java/org/eclipse/openvsx/analytics/ingestion/aws/AwsDownloadRecordSource.java |
AWS log record source |
server/src/main/java/org/eclipse/openvsx/analytics/DownloadAnalyticsConfiguration.java |
Analytics bean configuration |
server/src/main/java/org/eclipse/openvsx/analytics/ContinuousAggregateSet.java |
Aggregate registration contract |
server/src/main/java/org/eclipse/openvsx/analytics/ContinuousAggregateRefresher.java |
Continuous aggregate refresh logic |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Copilot flagged four issues on the backfill feature: - Re-uploading or retrying a backfill wrote a second set of download_event rows, since nothing tracked which files had already been backfilled. Adds a dedicated download_backfill ledger (deliberately separate from download_ingestion, which backfill still does not touch) and rejects a repeat of the same file with 409, both from the API and at the DB level via a unique index. - The S3 response stream stayed open while the temp file was parsed, defeating the connection-release comment right above it and risking pool exhaustion. Closes it before the parse starts. - An admin upload had no cap on decompressed size, so a small but highly-compressible gzip could grow the in-memory record list without bound. Wraps the decompressed stream in the existing SizeLimitInputStream, configurable via ovsx.analytics.ingestion.max-decompressed-bytes. Also removes findAllDownloadIngestionsProcessedSince, which was added for the same double-counting problem but never wired into the backfill path - dead code superseded by the ledger above. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Complements generate-download-logs.sh --upload, which only exercises the scheduled AWS ingestion path. This one reuses the same generator to build the log, then POSTs it straight to the new backfill endpoint, so the API can be exercised without a Silo/S3 bucket or waiting on the cron schedule. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The prior script (77c96c5) came out of a research task that overstepped its scope and pushed on its own; replaces it with a version that also supports a dry-run preview (--out without --backfill, matching generate-download-logs.sh's own --out/--upload split) and skips the unnecessary gzip step, since the parser already auto-detects it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The download_backfill table/entity added to fix Copilot's double-counting findings was reviewed and judged not worth its footprint - a new table, entity, repository and migration for what boils down to a boolean check. Reusing download_ingestion instead would have needed either a storageType hack or a new column plus filtering the scheduled job's own queries, to avoid it wrongly skipping a file an admin had backfilled under the same name; both were judged more complexity than the problem warrants. Reverts to the original behavior: backfill trusts the caller not to re-upload the same file, with no persisted guard against it. The S3 stream-lifetime and decompressed-size-cap fixes from 65f2d0d are unaffected. Updates generate-download-logs-backfill.sh to match (drops --file-name, no longer needed). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Completes ae4b2c0 - that commit only removed the new entity/repository/ migration files; this one reverts the code that referenced them (DownloadBackfillAPI, DownloadIngestionProcessor, RepositoryService and their tests) back to the pre-ledger behavior, and drops --file-name from generate-download-logs-backfill.sh since the endpoint no longer takes it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
generate-download-logs-backfill.sh only ever wrapped this script and added a curl call, so the two had near-identical flags and structure. Folds that into a --backfill mode alongside the existing --upload, sharing all the generation logic, argument parsing and usage text directly instead of through a delegating sibling script. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
netomi
approved these changes
Sep 22, 2026
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.
No description provided.