Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reader: tests / centralize RDY handling #8

Merged
merged 10 commits into from
Oct 21, 2013
Merged

Conversation

mreiferson
Copy link
Member

this adds better test coverage for backoff and recovery and refactors RDY handling to a single goroutine per reader rather than per connection. ping #1 cc @jehiah

@mreiferson
Copy link
Member Author

brace yourself... this one is RFR @jehiah

@jehiah
Copy link
Member

jehiah commented Sep 14, 2013

🚀 nice work. I've taken a first pass at reading through the diff. I'd like to sit on this a little as it's tough to remember all the intricacies of RDY handling right away.

@mreiferson
Copy link
Member Author

this is a lonely pull request

@mreiferson
Copy link
Member Author

ROUND 2

This was referenced Oct 18, 2013
* consolidate RDY handling to a single goroutine per Reader
  instead of per connection.  this simplifies the code paths
  and eliminates the need to synchronize across connections.
  (ie. when you encounter backoff you want all of the connections
  for that Reader to backoff).

* when exiting backoff send RDY 1 to single, random, connection.

* improve redistribution to better detect and handle cases
  where disconnections need their orphaned RDY count
  transferred to existing connections, such as when in backoff.
 * trim long lines
 * separate files for nsqConn and Reader
 * comment cleanup
jphines pushed a commit that referenced this pull request Oct 21, 2013
reader: tests / centralize RDY handling
@jphines jphines merged commit 836cf20 into nsqio:master Oct 21, 2013
amosir pushed a commit to amosir/go-nsq that referenced this pull request Jan 16, 2024
write to next file if  writePos == maxBytesPerFile
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants