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

Fix copy index command #83

Merged
merged 3 commits into from
Jan 31, 2019
Merged

Fix copy index command #83

merged 3 commits into from
Jan 31, 2019

Conversation

willtj
Copy link
Contributor

@willtj willtj commented Jan 31, 2019

The cursor method was defaulting to 100,000 records and failing to allow it to be set to zero, so copying the index was stopping after hitting that limit.

@ghost ghost assigned willtj Jan 31, 2019
@ghost ghost added the review label Jan 31, 2019
Copy link
Contributor

@JasparGupta JasparGupta left a comment

Choose a reason for hiding this comment

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

Also, in Connection.php line 209, that variable is no longer being used.

if ($limit > $numResults) {
foreach ($this->scroll($scrollId, $scrollTimeout, $limit - $numResults) as $result) {
if (! $limit || $limit > $numResults) {
$limit = $limit ? $limit - $numResults : $limit;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'm being an idiot, but I'm not sure I understand this line, I mean I get that you're setting the limit, but I can figure out what $limit - $numResults is doing. So I have a limit of, say, 100, and the number of results is 25, set the limit to 75?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry yeah, it's confusing. The default limit will be zero so all results will be returned, but if you want a scroll cursor with 1000 results and the first query (on line 205) returns 100 results, then you want to scroll through 900 additional results, so 900 gets passed to scroll() and at line 255 it will break out of the scroll once that's been hit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ohhh I see, ok I'm with you!

@willtj
Copy link
Contributor Author

willtj commented Jan 31, 2019

Good one on line 209, thanks - I'll remove.

Copy link
Contributor

@JasparGupta JasparGupta left a comment

Choose a reason for hiding this comment

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

LGTM

@willtj willtj merged commit f82324d into master Jan 31, 2019
@ghost ghost removed the review label Jan 31, 2019
@willtj willtj deleted the copy-index-command branch January 31, 2019 11:59
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.

2 participants