Fix event drop by fetching 3 pages with ETag optimization - #317
Fix event drop by fetching 3 pages with ETag optimization#317emalaj-seq wants to merge 2 commits into
Conversation
Fixes igrigorik#310 The previous commit f1f4200 changed PAGE_LIMIT from 500 to 100 (per GitHub API requirements) but only fetched a single page, causing an ~80% drop in captured events (from ~500 to ~100 per cycle). This fix: - Fetches 5 pages in parallel (up to 500 events, matching original capacity) - Adds ETag-based conditional GET requests to minimize API usage - Skips polling cycles when page 1 returns 304 Not Modified - Automatically scales: only fetches pages that contain data - Updates Gemfile to use https:// instead of git:// protocol Results: - Restores original 500 event capture capacity - Efficient API usage through ETag optimization - No more missed events during normal GitHub activity levels Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
emalaj-seq
changed the title
pombredanne
left a comment
pombredanne
left a comment
There was a problem hiding this comment.
Hey :) Here are some quick review and suggestions from a bystander and nitpicker....
It has been a while since you submitted this and I think this can help. Are you still game to finish this?
Also an aside wrt. etags: given the volume of activities globally on the /events endpoint, when could there be a time where the etag would apply and avoid refetching? I fear that's never... but etags could be useful when adding support for focused per-org/user/repo event collection to backfill missing.
I am also wondering if we could use multiple keys and rotate them.
| # If page 1 hasn't changed (304 Not Modified), skip this cycle | ||
| if req1.response_header.status == 304 | ||
| @log.debug "Page 1 not modified, skipping" | ||
| EM.add_timer(0.2, &process) |
There was a problem hiding this comment.
What's the rationale for 0.2 seconds?
| @file.close | ||
| end | ||
| @file = File.new(archive, "a+") | ||
| # If page 1 hasn't changed (304 Not Modified), skip this cycle |
There was a problem hiding this comment.
Is this a fact that if the 1st page is not modified, then 2 and 3 are not either?
|
|
||
| @latest = [] | ||
| @latest_key = lambda { |e| "#{e['id']}" } | ||
| @etags = {} # Track ETags for each page |
There was a problem hiding this comment.
Since tracking for each page, why not just use etag_1, etag_2 and etag_3?
|
|
||
| StatHat.new.ez_count('Github Events', new_events.size) | ||
|
|
||
| EM.add_timer(0.2, &process) |
|
|
||
| gem 'eventmachine', :git => 'git://github.com/eventmachine/eventmachine.git' | ||
| gem 'em-http-request', :git => 'git://github.com/igrigorik/em-http-request.git' | ||
| gem 'eventmachine', :git => 'https://github.com/eventmachine/eventmachine.git' |
There was a problem hiding this comment.
I'd keep these changes in a separate PR.
3 participants
@igrigorik
Fixes #310
The previous commit f1f4200 changed PAGE_LIMIT from 500 to 100 (per GitHub API requirements) but only fetched a single page, causing an ~80% drop in captured events (from ~500 to ~100 per cycle).
This fix:
Results: