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

SOLR-14036: Remove explicit distrib=false from /terms handler #1900

Merged
merged 6 commits into from
Sep 24, 2020

Conversation

munendrasn
Copy link
Contributor

  • This removes shards, shards.qt, and shard whitelisting checks from TermsComponent. Similar to other components, this check will be done in HttpShardHandler

@munendrasn
Copy link
Contributor Author

@tflobbe I have removed CustomTermsComponentTest as none of the components currently have custom handling of shardWhitelist. Let me know if that should be added back

@munendrasn munendrasn changed the title [SSK-14036]: Remove explicit distrib=false from /terms handler SSK-14036: Remove explicit distrib=false from /terms handler Sep 21, 2020
@munendrasn munendrasn changed the title SSK-14036: Remove explicit distrib=false from /terms handler SOLR-14036: Remove explicit distrib=false from /terms handler Sep 21, 2020
@dsmiley
Copy link
Contributor

dsmiley commented Sep 21, 2020

I'm glad to see this :-)

I think the change of default behavior for users using /terms should be master-only and have a note in solr-upgrade-notes.adoc. If you like, you could backport the underlying fix but not the change in default.

@munendrasn
Copy link
Contributor Author

@joel-bernstein Please review(not able to tags as a reviewer so the ping)

@munendrasn
Copy link
Contributor Author

I think the change of default behavior for users using /terms should be master-only and have a note in solr-upgrade-notes.adoc. If you like, you could backport the underlying fix but not the change in default.

At this point, I'm planning to keep this master only but if there is a need will backport the necessary changes to 8x

@arafalov
Copy link
Contributor

I can't comment on the code, as I did not understand the shard part. I am happy if the extra parameter (distrib=false) is not needed. I did feel that maybe a test was missing somehow. Is there one that tests that distributed terms work? Especially, since terms handler is explicitly defining the Component Chain to be terms only and nothing else (not even sure if that's relevant, though).

@munendrasn
Copy link
Contributor Author

@arafalov
There is one already which I have modified(DistributedTermsComponentTest) - https://github.com/apache/lucene-solr/pull/1900/files#diff-9b9ccbcf271c6320902d92479615f4fbR73

@arafalov
Copy link
Contributor

@arafalov
There is one already which I have modified(DistributedTermsComponentTest) - https://github.com/apache/lucene-solr/pull/1900/files#diff-9b9ccbcf271c6320902d92479615f4fbR73

Thank you for clarification. I saw that code change, but did not realize it answered my question.

Copy link
Contributor

@dsmiley dsmiley left a comment

Choose a reason for hiding this comment

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

Once I see the solr-upgrade-notes.adoc change, I'll approve.

@munendrasn
Copy link
Contributor Author

I have included the changes and upgrade entry. Instead of adding upgrade entry to solr-upgrade-notes.adoc, I have added to major-changes-in-solr-9.adoc as mentioned the former doc

Co-authored-by: David Smiley <david.w.smiley@gmail.com>
@dsmiley dsmiley self-requested a review September 24, 2020 03:24
@munendrasn munendrasn merged commit ac58472 into apache:master Sep 24, 2020
@munendrasn munendrasn deleted the terms-component branch September 24, 2020 16:42
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.

3 participants