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

Forgotten line #126

Closed
tdeo opened this issue Sep 9, 2015 · 2 comments · Fixed by #127
Closed

Forgotten line #126

tdeo opened this issue Sep 9, 2015 · 2 comments · Fixed by #127

Comments

@tdeo
Copy link
Contributor

tdeo commented Sep 9, 2015

Hi,

I was writing some migration on a table, and came across a weird issue while testing in development environment. When I have a single line in a table, this line is lost during the migration process. It works well with 2, 5 or more.

I believe this comes from https://github.com/soundcloud/lhm/blob/master/lib/lhm/chunker.rb#L30.
If I understand that line well, the || (@next_to_insert == 1 && @start == 1) part was added in case @next_to_insert == 0 && @start == 1 && @limit == 1 (if @limit != 1, the second part of the or statement implies the first one).

Shouldn't that line be changed from while @next_to_insert < @limit || (@next_to_insert == 1 && @start == 1) to while @next_to_insert <= @limit?

Moreover, I had a feeling that if @limit - @start % stride == 1, the last line would be lost. I tested that it is actually the case.

@tdeo
Copy link
Contributor Author

tdeo commented Aug 25, 2017

@grobie @sunny Does this issue makes sense and have a chance of being addressed?

I have two PR opened for this:

Should I rebase them?

@grobie
Copy link
Contributor

grobie commented Aug 25, 2017

@tdeo Please go ahead! While I'm not actively using or maintaining LHM anymore, I'd be more than happy to merge a well tested bug fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants