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

[Feature] Support ElasticSearch 7 as backend storage #3870

Merged
merged 22 commits into from
Nov 21, 2019
Merged

[Feature] Support ElasticSearch 7 as backend storage #3870

merged 22 commits into from
Nov 21, 2019

Conversation

kezhenxu94
Copy link
Member

Add storage-elasticsearch7-plugin module

Motivation:

Support ES7 as storage

Modification:

Add storage-elasticsearch7-plugin, make the compilation passed, add maven profiles to distinguish different version of ES

Result:

ES7 storage plugin can co-exist with ES6 storage plugin

Most of the codes come from the existed ES6 plugin, and the credits should belong to @peng-yongsheng

@kezhenxu94 kezhenxu94 added core feature Core and important feature. Sometimes, break backwards compatibility. backend OAP backend related. plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor. labels Nov 16, 2019
@kezhenxu94 kezhenxu94 added this to the 6.6.0 milestone Nov 16, 2019
@wu-sheng
Copy link
Member

Before all codes related discussion, a question, as this profile based solution, how should developers run the local test for both ES6 and ES7? I mean in the IDE.

@kezhenxu94
Copy link
Member Author

Before all codes related discussion, a question, as this profile based solution, how should developers run the local test for both ES6 and ES7? I mean in the IDE.

Simply switch the profile in maven tab

image

and I'm not sure if it's possible to achieve the same goal without using profiles, we have to build two distribution package, and have two dependency hierarchies

@wu-sheng
Copy link
Member

and I'm not sure if it's possible to achieve the same goal without using profiles, we have to build two distribution package, and have two dependency hierarchies

That is why I proposed two starter module. After you have that, you don't need profile anymore.
From my understanding, our project repo has been too complex already, if need profile to package different dist, it is very hard for a new developer.

@wu-sheng
Copy link
Member

Another thing, if you have the exclude and dependency right in new starter and storage, you could use most of existing es6 storage codes, rather than copy them again. It increases the payload to maintain codes. You should just need to implement the ES7 DAO, if ES API changed.

@kezhenxu94 kezhenxu94 closed this Nov 16, 2019
@kezhenxu94 kezhenxu94 reopened this Nov 17, 2019
@kezhenxu94
Copy link
Member Author

@wu-sheng Review comments are addressed, please recheck when you have got some time

Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two comments. The most concerns are duplicated config files. Suggest to optimize.

@wu-sheng
Copy link
Member

@kezhenxu94 You need to sync and resolve the new YAML file. Facing conflicts.

@dmsolr
Copy link
Member

dmsolr commented Nov 20, 2019

Hi @kezhenxu94
I found thread_pool.index.queue_size is deprecated in es7.4.2. Do we need to update the FAQ of the document?

@kezhenxu94
Copy link
Member Author

Hi @kezhenxu94
I found thread_pool.index.queue_size is deprecated in es7.4.2. Do we need to update the FAQ of the document?

The FAQ is for users who are facing the exceptions, if the thread_pool.index.queue_size is not set by default in ES7, which I thing is the case, the users won't reach the FAQ

@wu-sheng
Copy link
Member

What we recommend for es7? Is write queue size still there? Add these two in document directly?

@wu-sheng
Copy link
Member

@dmsolr I think we also need to update the document, another PR for that?

@dmsolr
Copy link
Member

dmsolr commented Nov 20, 2019

@dmsolr I think we also need to update the document, another PR for that?

Yes, I will do that.

docs/en/setup/backend/backend-storage.md Outdated Show resolved Hide resolved
docs/en/setup/backend/backend-storage.md Outdated Show resolved Hide resolved
Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. @dmsolr @arugal Please recheck. And as @arugal told me, he is very busy these days, we could merge this after @dmsolr finished the local test, and he will test later.

@wu-sheng
Copy link
Member

/run agent-plugin-test-4

@wu-sheng
Copy link
Member

Merging.

@wu-sheng wu-sheng merged commit 9b408e8 into apache:master Nov 21, 2019
@kezhenxu94 kezhenxu94 deleted the feature/es7-storage branch November 21, 2019 03:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend OAP backend related. core feature Core and important feature. Sometimes, break backwards compatibility. plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants