You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2021/06/02 14:36:53 UTC
[GitHub] [solr] tboeghk opened a new pull request #159: [SOLR-10059] Handle appends params in distributed requests
tboeghk opened a new pull request #159:
URL: https://github.com/apache/solr/pull/159
# Description
In distributed requests, the `appends` section of a `RequestHandler` is evaluated not only on the non-distributed coordinator requests but also during the distributed requests. This might lead to parameters (especially harmful for `fq` params) being attached multiple times.
```xml
<requestHandler name="/myHandler" class="solr.SearchHandler">
<lst name="appends">
<str name="fq">{!collapse field=routingKey hint=top_fc}</str>
</lst>
</requestHandler>
```
The issue exists for some time in the bug tracker: [SOLR-10059](https://issues.apache.org/jira/browse/SOLR-10059)
# Solution
The fix ignores the `appends` section of the handler definition during distributed requests.
# Tests
I added a simple unit test, no cloud setup test though
# Checklist
Please review the following and check all that apply:
- [x] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [x] I have created a Jira issue and added the issue ID to my pull request title.
- [ ] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [x] I have developed this patch against the `main` branch.
- [x] I have run `./gradlew check`.
- [x] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #159: [SOLR-10059] Handle appends params in distributed requests
Posted by GitBox <gi...@apache.org>.
epugh commented on pull request #159:
URL: https://github.com/apache/solr/pull/159#issuecomment-892617936
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] tboeghk commented on pull request #159: [SOLR-10059] Handle appends params in distributed requests
Posted by GitBox <gi...@apache.org>.
tboeghk commented on pull request #159:
URL: https://github.com/apache/solr/pull/159#issuecomment-891769483
Hey @epugh any chance to merge this? Or has anybody else directions for improvement?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] tboeghk commented on pull request #159: [SOLR-10059] Handle appends params in distributed requests
Posted by GitBox <gi...@apache.org>.
tboeghk commented on pull request #159:
URL: https://github.com/apache/solr/pull/159#issuecomment-853605016
Whoopsie, my bad, ran the `gradlew check` with `-Pvalidation.git.failOnModified=false`. Added license header in the last commit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on pull request #159: [SOLR-10059] Handle appends params in distributed requests
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on pull request #159:
URL: https://github.com/apache/solr/pull/159#issuecomment-974716554
> ... no cloud setup test though ...
#426 is about adding a `SearchHandlerAppendsCloudTest` test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] tboeghk commented on pull request #159: [SOLR-10059] Handle appends params in distributed requests
Posted by GitBox <gi...@apache.org>.
tboeghk commented on pull request #159:
URL: https://github.com/apache/solr/pull/159#issuecomment-892623271
Yes (I'm 95% sure): The macros don't get expanded on the shard. With the fix above the configuration is not picked up on the shard and the macros should stay expanded. Do we need another test for that?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #159: [SOLR-10059] Handle appends params in distributed requests
Posted by GitBox <gi...@apache.org>.
epugh commented on pull request #159:
URL: https://github.com/apache/solr/pull/159#issuecomment-853111093
Did you see the precommit failure on missing license header @tboeghk ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a change in pull request #159: [SOLR-10059] Handle appends params in distributed requests
Posted by GitBox <gi...@apache.org>.
epugh commented on a change in pull request #159:
URL: https://github.com/apache/solr/pull/159#discussion_r681963742
##########
File path: .gitignore
##########
@@ -25,3 +25,8 @@ __pycache__
# Emacs backup
*~
+
+# VSCode
Review comment:
Then, I think it's good for merging.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #159: [SOLR-10059] Handle appends params in distributed requests
Posted by GitBox <gi...@apache.org>.
epugh commented on pull request #159:
URL: https://github.com/apache/solr/pull/159#issuecomment-892617936
One more request @tboeghk, it apears from JIRA that your patch would fix https://issues.apache.org/jira/browse/SOLR-14931 ???
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] tboeghk commented on pull request #159: [SOLR-10059] Handle appends params in distributed requests
Posted by GitBox <gi...@apache.org>.
tboeghk commented on pull request #159:
URL: https://github.com/apache/solr/pull/159#issuecomment-853605016
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on pull request #159: [SOLR-10059] Handle appends params in distributed requests
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on pull request #159:
URL: https://github.com/apache/solr/pull/159#issuecomment-974716554
> ... no cloud setup test though ...
#426 is about adding a `SearchHandlerAppendsCloudTest` test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #159: [SOLR-10059] Handle appends params in distributed requests
Posted by GitBox <gi...@apache.org>.
epugh commented on pull request #159:
URL: https://github.com/apache/solr/pull/159#issuecomment-965069597
@cpoerschke I wanted to get this sorted, any chance you could be another set of eyes????
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] tboeghk commented on pull request #159: [SOLR-10059] Handle appends params in distributed requests
Posted by GitBox <gi...@apache.org>.
tboeghk commented on pull request #159:
URL: https://github.com/apache/solr/pull/159#issuecomment-892623271
Yes (I'm 95% sure): The macros don't get expanded on the shard. With the fix above the configuration is not picked up on the shard and the macros should stay expanded. Do we need another test for that?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a change in pull request #159: [SOLR-10059] Handle appends params in distributed requests
Posted by GitBox <gi...@apache.org>.
epugh commented on a change in pull request #159:
URL: https://github.com/apache/solr/pull/159#discussion_r681751836
##########
File path: .gitignore
##########
@@ -25,3 +25,8 @@ __pycache__
# Emacs backup
*~
+
+# VSCode
Review comment:
Could we pull this out into it's own ticket? DOn't want to confuse things by having the VSCode default mixed in...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] tboeghk commented on pull request #159: [SOLR-10059] Handle appends params in distributed requests
Posted by GitBox <gi...@apache.org>.
tboeghk commented on pull request #159:
URL: https://github.com/apache/solr/pull/159#issuecomment-853617796
I came across an edge case that I want to consider: In the distributed environment we should only ignore the `appends` section if we do not change the handler during the distributed requests. I'll add that as well and write some tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #159: [SOLR-10059] Handle appends params in distributed requests
Posted by GitBox <gi...@apache.org>.
epugh commented on pull request #159:
URL: https://github.com/apache/solr/pull/159#issuecomment-892673867
Does anyone ever say "no, we don't need another test" ;-) I'd love one, partly because this whole space is kind of new for me, so it might help me feel confident of your fix!!!! unit tests are super helpful in understanding the logic of the fix!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on pull request #159: [SOLR-10059] Handle appends params in distributed requests
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on pull request #159:
URL: https://github.com/apache/solr/pull/159#issuecomment-969144721
> @cpoerschke I wanted to get this sorted, any chance you could be another set of eyes????
From looking at the changes I'm wondering if [RequestUtil](https://github.com/apache/lucene-solr/blob/releases/lucene-solr/8.11.0/solr/core/src/java/org/apache/solr/request/json/RequestUtil.java#L51-L52) might be an alternative place for the logic since the _"don't expand macros on the shard"_ logic already happens there.
From reading about the context of the changes on https://issues.apache.org/jira/browse/SOLR-10059 on 2017-03-01 there's a mention of multi-collection requests, I don't yet fully understand how that fits into the bigger picture.
From running the new tests locally, hmm, they don't pass here at the moment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org