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

Page size and paging state #283

Merged
merged 5 commits into from Mar 26, 2016
Merged

Page size and paging state #283

merged 5 commits into from Mar 26, 2016

Conversation

bettse
Copy link
Contributor

@bettse bettse commented Feb 26, 2016

The Cassandra driver supports pagination through a page_size and paging_state, and this exposes them on the record_set class, much like consistency is, so they can be used.

I haven't written tests for this or run the vagrant tests yet, I wanted to get some initial feedback on if I was going about this in right way beforehand.

@bettse bettse force-pushed the bettse/page_size branch 2 times, most recently from 1dbdae1 to 26481b6 Compare Feb 26, 2016
consistency: query_consistency,
page_size: query_page_size,
paging_state: query_paging_state
})
Copy link
Contributor

@pezra pezra Mar 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should query paging state be updated before returning the results?

Copy link
Contributor Author

@bettse bettse Mar 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could, and I think it would clean up (or make unnecessary) the next_paging_state if it did, but there is also the question of how the library user would know if the paging_state they're getting is the one they supplied vs the new one. To some extent, I think that's why these are all prefixed with 'query', they're parameters of the query, not the result.

This is my first foray into the code, so there is a lot of speculation. I can certainly refactor to have it update if that is the preferred style.

Copy link
Contributor

@pezra pezra Mar 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... I think i see your point. I would be helpful, for me, to see some examples of expected usage pattern. That would clarify the user experience this would create. A test or two might do the trick.

@pezra
Copy link
Contributor

@pezra pezra commented Mar 24, 2016

@bettse, I like the idea of using the native paging. If you rebase against master you should be able to get the specs passing. I'd be delighted to have this improvement once the tests are passing (and there are sometests for this new behavior).

@bettse
Copy link
Contributor Author

@bettse bettse commented Mar 24, 2016

Awesome. I was kinda concerned the project had gone dormant. I'll do that, and add a comment when I feel its passing the tests (has some new tests), and is ready to be looked at. I think there are some places in the batch classes that I also noticed after making the PR and need to be updated.

@pezra
Copy link
Contributor

@pezra pezra commented Mar 24, 2016

@bettse, there was a bit of a lull in this project but i think we are back in business now. I look forward to seeing the final PR.

@bettse
Copy link
Contributor Author

@bettse bettse commented Mar 24, 2016

I seem to be a bit stuck. Travis shows one of the new tests I wrote failing on all versions, but locally against ruby 2.2.2 it was passing, and I've tried dropping the cequel_schema_test keyspace to see if it was a caching issue, and installed ruby 2.1.5 to try to more closely reproduce one of the travis runs. I just bundle exec rake test:all and got success for all non-missing combinations:

Results:
MISSING gemfile: gemfiles/rails_4.0.gemfile rvm: 2.2.0        
MISSING gemfile: gemfiles/rails_4.0.gemfile rvm: jruby-9.0.0.0
MISSING gemfile: gemfiles/rails_4.1.gemfile rvm: 2.2.0        
MISSING gemfile: gemfiles/rails_4.0.gemfile rvm: jruby-1.7.22 
MISSING gemfile: gemfiles/rails_4.1.gemfile rvm: 2.3.0        
MISSING gemfile: gemfiles/rails_4.0.gemfile rvm: 2.3.0        
MISSING gemfile: gemfiles/rails_4.1.gemfile rvm: jruby-1.7.22 
MISSING gemfile: gemfiles/rails_4.1.gemfile rvm: jruby-9.0.0.0
MISSING gemfile: gemfiles/rails_4.2.gemfile rvm: 2.2.0        
MISSING gemfile: gemfiles/rails_4.2.gemfile rvm: 2.3.0        
MISSING gemfile: gemfiles/rails_4.2.gemfile rvm: jruby-1.7.22 
MISSING gemfile: gemfiles/rails_4.2.gemfile rvm: jruby-9.0.0.0
SUCCESS gemfile: gemfiles/rails_4.1.gemfile rvm: 2.1.5        
SUCCESS gemfile: gemfiles/rails_4.0.gemfile rvm: 2.1.5        
SUCCESS gemfile: gemfiles/rails_4.2.gemfile rvm: 2.1.5

I'm open to any ideas about how I might reproduce the failure locally, or if there are nuances to travis that could be playing a part (I have no experience with it).

@bettse
Copy link
Contributor Author

@bettse bettse commented Mar 24, 2016

reproduced! I had been testing against cassandra 2.0.4 since that was the example in the contributing file, but after bringing up a vagrant image for 2.1.3 (arbitrarily chosen), the test now fails and I can get to fixing it :)

@bettse
Copy link
Contributor Author

@bettse bettse commented Mar 24, 2016

Green and ready for review!


def next_paging_state
execute_cql(*cql).paging_state
end
Copy link
Contributor

@pezra pezra Mar 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that the query is run twice? Ie, once to figure the paging state and once to iterate over the results? We should extract execute_cql(*cql) in to a #results method that memoizes that operation so we only do it once per data set instance.

Copy link
Contributor Author

@bettse bettse Mar 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct, the query would be called a second time. I can take a whack at the memoization.

@pezra
Copy link
Contributor

@pezra pezra commented Mar 25, 2016

This is looking real close. A bit more polish and we'll get it merged.

@bettse bettse force-pushed the bettse/page_size branch 2 times, most recently from f848aae to 1cc8643 Compare Mar 25, 2016
@bettse
Copy link
Contributor Author

@bettse bettse commented Mar 25, 2016

There were some intermittent test failures on travis, but now its all green. Let me know what you think 🔬

@pezra pezra merged commit 155ab15 into cequel:master Mar 26, 2016
1 check passed
@pezra
Copy link
Contributor

@pezra pezra commented Mar 26, 2016

Merged. Thanks for putting that together.

@bettse
Copy link
Contributor Author

@bettse bettse commented Mar 26, 2016

Thank you for your guidance!

@bettse bettse deleted the bettse/page_size branch Mar 26, 2016
@orenmazor
Copy link

@orenmazor orenmazor commented Aug 25, 2016

Finding this PR literally made my day.

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

Successfully merging this pull request may close these issues.

None yet

3 participants