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

DATAES-407 - Support for HighLevelRestClient #216

Merged
merged 1 commit into from Sep 28, 2018

Conversation

Projects
None yet
8 participants
@donrw
Contributor

donrw commented Jul 7, 2018

Added an ElasticsearchRestTemplate and support for XML configuration of HighLevelRestClient.

ElasticSearchOperations no longer has getClient(). So, that will be a breaking change if anyone uses it.

Inclusive of changes for DATAES-468, since that pull request isn't accepted and I neglected to baseline DATAES-407 work on that branch and my git history go screwed up, so it is all squashed into here.

I decided to set the infrastructure.xml node client to http-enabled=true and import it for rest client tests. I am a little worried that we may need to do something to check for an unused port and assign appropriately for 100% reliability, and propagate that to the rest client configuration. I don't know anything about the build server to know if there is a chance of port collision. Two branches of spring-data-elasticsearch on the same build agent? Maybe for another day...

I altered CustomMethodRepositoryTest to do a test with both the old ElasticsearchTemplate and ElasticsearchRestTemplate to double check compatibility of the ElasticsearchRestTemplate. I didn't think doubling up all the repository tests was worth the effort.

  • You have read the Spring Data contribution guidelines.
  • There is a ticket in the bug tracker for the project in our JIRA.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.
  • You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).
DATAES-407 - Support for HighLevelRestClient via
ElasticsearchRestTemplate

Removed getClient() from EsOperations
Made CustomMethodRepositoryTest for both TranportClient and RestClient
to make sure the new Template worked with a Repository.
@xhaggi

This comment has been minimized.

Collaborator

xhaggi commented Jul 12, 2018

Thank you for your contribution. We will need some time to review your changes.

@akonczak

This comment has been minimized.

Contributor

akonczak commented Jul 20, 2018

Awesome job with rest client @donrw.

All changes are looking good - both transport and rest clients are working alongside - this is a good base to start gradually removing support for transport client and give people time to adapt to the changes.
I had to ignore 2 tests due to problem with accessing final private fields over reflection "java.lang.UnsupportedOperationException: No accessor to set property" - @olivergierke can you help with this - I believe that you have spend already some time trying to fix it - DATACMNS-1322

@akonczak

This comment has been minimized.

Contributor

akonczak commented Jul 20, 2018

@dllz

This comment has been minimized.

dllz commented Aug 8, 2018

Waiting for this to be merged.

@mp911de

This comment has been minimized.

Member

mp911de commented Aug 14, 2018

We missed merging this pull request for the last release candidate. We're now in the RC phase of the Lovelace release train and merging such a significant change, even though the change introduces primarily a new Template API implementation, we want to reduce the risk by including the merge for the next, Moore, release train.

@fbaligand

This comment has been minimized.

fbaligand commented Aug 23, 2018

Really sad to hear this @mp911de :(

@akonczak

This comment has been minimized.

Contributor

akonczak commented Aug 31, 2018

GA for Lovelace is scheduled for 19 of September - after that date we should be able to merge changes into master

https://github.com/spring-projects/spring-data-commons/wiki/Release-Train-Moore

@akonczak

This comment has been minimized.

Contributor

akonczak commented Sep 24, 2018

Hi @mp911de - I should be able to merge this PR to master this week

@fbaligand

This comment has been minimized.

fbaligand commented Sep 24, 2018

Great !

http\://www.springframework.org/schema/data/elasticsearch/spring-elasticsearch-1.0.xsd=org/springframework/data/elasticsearch/config/spring-elasticsearch-1.0.xsd
http\://www.springframework.org/schema/data/elasticsearch/spring-elasticsearch.xsd=org/springframework/data/elasticsearch/config/spring-elasticsearch-1.0.xsd
http\://www.springframework.org/schema/data/elasticsearch/spring-elasticsearch-1.0.xsd=org/springframework/data/elasticsearch/config/spring-elasticsearch-1.0.xsd
http\://www.springframework.org/schema/data/elasticsearch/spring-elasticsearch-1.1.xsd=org/springframework/data/elasticsearch/config/spring-elasticsearch-1.1.xsd

This comment has been minimized.

@mp911de

mp911de Sep 24, 2018

Member

We should rather upgrade schema versions to 3.2 to reflect the relation between the schema and the artifact version.

*/
@RunWith(SpringJUnit4ClassRunner.class)
@ContextConfiguration("classpath:elasticsearch-rest-template-test.xml")
public class ElasticsearchRestTemplateTests {

This comment has been minimized.

@mp911de

mp911de Sep 24, 2018

Member

How about parametrizing the test so we can run the exact same tests against two different implementations? Would reduce test code and removes the possibility of deviations as both implementations are expected to behave the same way.

This comment has been minimized.

@akonczak

akonczak Sep 27, 2018

Contributor

yes - that will be a nice option to do that in that way - I'm not sure if new client cover all old cases but we can try to go with this idea, My plan is to merge PR and them reduce duplication in test between transport client and http client.

@akonczak akonczak merged commit e782dd1 into spring-projects:master Sep 28, 2018

@fbaligand

This comment has been minimized.

fbaligand commented Sep 28, 2018

Great to see this PR is merged !

@odrotbohm

This comment has been minimized.

Member

odrotbohm commented Sep 29, 2018

@akonczak – Thanks for the merge! Would you mind fixing up the merge to avoid the merge commit and make sure that the commit messages follow the usual template? We can do that ourselves as well, I just wanted to avoid we step on each other's toes 🙃.

@akonczak

This comment has been minimized.

Contributor

akonczak commented Sep 29, 2018

@olivergierke - it has been a while sine I was playing with SDES project and I have forget a lot of procedures, my mistake - I need just confirmation from you that you are happy that I will alter/override history of master

@odrotbohm

This comment has been minimized.

Member

odrotbohm commented Sep 29, 2018

Please go ahead with the cleanups. We can add polishing commits on top. It’s just hard to keep track of what’s actually included with that merge commit.

akonczak added a commit that referenced this pull request Sep 29, 2018

@harshahst

This comment has been minimized.

harshahst commented Nov 28, 2018

Hi, We have started using AWS Elastic Cluster and it, in turn, depends on Rest Client instead of Transport Client... 3.1.3.RELEASE does not include these changes... Please let me know when these changes will be included in the official Release...

@mp911de

This comment has been minimized.

Member

mp911de commented Nov 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment