solr
solr copied to clipboard
apache
SOLR-17321: Fix TestSolrCoreSnapshots.testBackupRestore
TestSolrCoreSnapshots.testBackupRestore test started failing on Windows after merging #2501.
Thank you @HoustonPutman for reporting this. This one specifically failed due to the backslash in the windows location path that passed to the URI ctor.
The solution for this issue is to encode all the passed params before passing it to the URI ctor.
Checklist
Please review the following and check all that apply:
- [x] I have reviewed the guidelines for How to Contribute 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.
- [x] I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
- [x] I have developed this patch against the
mainbranch. - [ ] I have run
./gradlew check. - [ ] I have added tests for my changes.
- [ ] I have added documentation for the Reference Guide
This will probably work, but using a string builder to build a URL just seems wrong (and this isn't the only place in the file that does it). It will probably be harder, but it would be great if we could build URL/URIs in a correct way in BackupRestoreUtil. I guess when the v2 APIs cover this, then a lot of this will not be useful anymore.
Replaced the StringBuilder with URIBuilder and also there is no need to explicitly encode the params as URIBuilder takes care of it.
Is there any way to test TestHdfsBackupRestoreCore?
LGTM. Is the URIBuilder something that we should be using in more places?
I certainly think so
LGTM. Is the URIBuilder something that we should be using in more places?
I certainly think so
How do we get these tasks actually tracked and moved forward? Worth opening a jira? We have a patten of "we should do" but then it's just in our heads!
URIBuilder is in the Apache HttpClient dependency, using it goes in the opposite direction we're trying to go in! Granted in tests, this isn't a priority, so this PR is acceptable. But be mindful please.
We have a patten of "we should do"
That could be quite a list; I'm not sure what tracking it accomplishes. If something is important enough, create a JIRA. Ideally when a "we should do" is identified, it can be done everywhere instead of just one spot.
