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

Updating to/deleting from remote Solr on newer Tomcats. #4

Closed
wants to merge 8 commits into from

Conversation

adam-vessey
Copy link
Contributor

What does this Pull Request do?

A few things:

  • The main thing: Preventing errors with requests out to Solr containing characters deemed no longer valid in newer Tomcat implementations. See https://github.com/apache/tomcat70/commit/cdc0a935c2173aff60039a0b85e57a461381107c#diff-7ebec9f305c35d9e9486cbe25e39938fR113
  • Solr 5 compatibility: Rolled in equivalent code to @Rarian's fork (thought it might be related to the above... could revert if desired).
  • Prevent warning when generating warning outputs: Was reporting issues due to the presence of "&rows" in URLs, which were getting dumped into XML, and so reporting about the unknown "rows" entity. General XML escaping, to cover the ampersand and anything else which may pop up there.

How should this be tested?

Things should keep on working.

Background context:

Updating and deleting items from the index would fail on newer Tomcats.

Additional Notes:

  • Does this change require documentation to be updated? No.
  • Does this change add any new dependencies? Yes: commons-lang3-3.5.jar, included. Also, uses some Java 7-specific syntax (handling multiple exceptions in a single catch block).
  • Does this change require any other modifications to be made to the repository (ie. Regeneration activity, etc.)? No.
  • Could this change impact execution of existing code? No.

Adam Vessey
Developer
discoverygarden inc. | Managing Digital Content

@ddavis
Copy link
Member

ddavis commented Mar 1, 2017

I am curious if Gert is still the person for this PR since I am interested in having GSearch maintained for a while longer especially Solr 5.

@adam-vessey
Copy link
Contributor Author

Any idea how to move this forward, @ddavis, @gertsp?

We (@discoverygarden) would prefer to contribute these changes upstream; however, if there's nobody to accept/review the changes, we could close this PR and proceed with our fork?

@ddavis
Copy link
Member

ddavis commented Mar 13, 2017

I think we can get this reviewed (and accepted). I don't know if anyone will want put the time in to creating a new formal release of GSearch but folks who want to use this code can build from source if this is acceptable to you. I am interested in getting a version on Solr 5 but also testing the code in Tomcat with Fedora 3.8.1 and Java 8. I emailed with Gert.

@ddavis ddavis closed this Mar 13, 2017
@ddavis
Copy link
Member

ddavis commented Mar 13, 2017

Accidently closed

@ddavis ddavis reopened this Mar 13, 2017
@ddavis
Copy link
Member

ddavis commented Mar 15, 2017

Passing on one reviewer trivial comment. It is preferred that the .classpath files not be included. That IDE specific files generally should be left out.

@adam-vessey
Copy link
Contributor Author

@ddavis: I agree, they generally shouldn't be there; however, they were already present in the repo: Are you (/your reviewer) suggesting that they should be removed entirely? I wasn't quite comfortable taking that amount of initiative on my own; however, if desired, I can make it happen. On the other hand, if they're being left there, I think it's better to have them in sync with the rest of the code, as loading it up as it was results in a significant amount of errors reported.

Ideally, one could break away from using the "lib" directory as it is here presently, and move to use a build tool with dependency resolution (like Maven or Gradle or whichever)... This repo is rather large (~192MiB), largely due to having all the different versions of dependencies included directly; however, would require something of a rebase to resolve the size issue.

@willtp87
Copy link

willtp87 commented Apr 3, 2017

Hello,

This pull has been open for a month without appreciable action, and as with the rest of the F3 stack there won't be further releases.
We will therefore be closing this pull and proceeding with our fork for long term maintenance.
We invite any further collaboration to happen there.

Thanks,
William Panting

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