-
Notifications
You must be signed in to change notification settings - Fork 1.3k
NUTCH-2739 : Upgrade ES and migrate to REST client #484
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
Conversation
balashashanka
commented
Nov 7, 2019
- Updated the Transport Client to rest client.
- The test cases will only pass when there is a elastic server set up in local.
|
Thank you @balashashanka for this PR. |
|
Is anyone aware of any tradeoff's between indexer-elastic and indexer-elastic-rest? |
sebastian-nagel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @balashashanka, looks promising. I'll hope to be able to test it soon. The plugin.xml needs to be done first, best have a look how this is done in the plugin parse-tika.
src/plugin/indexer-elastic/ivy.xml
Outdated
| <dependencies> | ||
| <dependency org="org.elasticsearch" name="elasticsearch" rev="5.3.0" conf="*->default"/> | ||
| <dependency org="org.elasticsearch.client" name="transport" rev="5.3.0"/> | ||
| <dependency org="org.elasticsearch.client" name="elasticsearch-rest-high-level-client" rev="7.3.0"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The updated dependencies must be also "registered" in indexer-elastic/plugin.xml. The description hot to uprade ES is somewhat outdated, better have a look at parse-tika for both the description and build-ivy.xml.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure @sebastian-nagel will add the changes in plugin.xml and remove unnecessary dependencies.
| import java.util.UUID; | ||
|
|
||
| /*Test works only when there is a elastic client setup is already there. | ||
| * Because Rest Client requires a elastic server connection for it to function unlike Transport client*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood. But then we need to deactivate the tests by default. Otherwise the nightly builds will fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But maybe there is a way to mock the client, see CustomRestHighLevelClientTests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Sebastian, I went through the CustomRestHighLevelClientTests, and it seems it is to test Rest High Level client changes. This will mock the client itself. But we need to mock the server with requests and response. So can we go ahead and not do the tests at all?
Well, the previous non-REST test implemented a client which did not send anything to the server but just returned a successful response or (if But I'm ok to remove the Test class if it's too much work to rewrite it for the REST client. I've tested the PR but the initial rounds failed for about 50% of the pages/documents: I got it fixed by using XContentBuilder to pass document as JSON to ES client, you'll find the necessary changes in this branch. Also:
So far, I didn't run any tests at scale. Should be to make sure we are able to index millions of documents with the given settings. Please have a look at my changes. Can you integrate them into your branch? |
649fd12 to
51af6f6
Compare
|
Thanks, @balashashanka! I'll run another round of tests, and will merge soon. |
NUTCH-2739 indexer-elastic: Upgrade ES and migrate to REST client - upgrade to Elasticsearch 7.3.0 - use Java High Level REST Client instead of deprecated TransportClient
|
Rebased and merged into master in c23afa8. Thanks, @balashashanka! |