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

Rebuilt ElasticSearch client on top of their REST API #7634

Merged
merged 2 commits into from
Sep 6, 2021

Conversation

kezhenxu94
Copy link
Member

@kezhenxu94 kezhenxu94 commented Sep 1, 2021

@kezhenxu94 kezhenxu94 added backend OAP backend related. feature New feature labels Sep 1, 2021
@kezhenxu94 kezhenxu94 added this to the 8.8.0 milestone Sep 1, 2021
@kezhenxu94 kezhenxu94 force-pushed the feature/elasticsearch-client branch 6 times, most recently from d49df7f to 9df20c3 Compare September 1, 2021 14:07
wu-sheng
wu-sheng previously approved these changes Sep 6, 2021
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. Wait for others' review.

@JaredTan95
Copy link
Member

I tested locally, all looks great to me except EmptyEndpointGroupException as follows when es is shutdown and connected failures:

2021-09-06 11:14:10,020 org.apache.skywalking.library.elasticsearch.ElasticSearch 149 [armeria-eventloop-nio-4-2] DEBUG [] - [creqId=3fe1891d, preqId=8179d41f][http://UNKNOWN/#GET] Request: {startTime=2021-09-06T03:14:10.010Z(1630898050010987), length=0B, duration=4538µs(4538520ns), cause=com.linecorp.armeria.client.UnprocessedRequestException: com.linecorp.armeria.client.endpoint.EmptyEndpointGroupException, scheme=none+http, name=GET, headers=[:method=GET, :path=/, :scheme=http, :authority=UNKNOWN]}
2021-09-06 11:14:10,021 org.apache.skywalking.library.elasticsearch.ElasticSearch 186 [armeria-eventloop-nio-4-2] WARN  [] - [creqId=3fe1891d, preqId=8179d41f][http://UNKNOWN/#GET] Response: {startTime=2021-09-06T03:14:10.020Z(1630898050020718), length=0B, duration=0ns, totalDuration=9749µs(9749393ns), cause=com.linecorp.armeria.client.UnprocessedRequestException: com.linecorp.armeria.client.endpoint.EmptyEndpointGroupException, headers=[:status=0]}
com.linecorp.armeria.client.UnprocessedRequestException: com.linecorp.armeria.client.endpoint.EmptyEndpointGroupException

I think EmptyEndpointGroupException is not enough clear to point out the cause of the error?

@kezhenxu94
Copy link
Member Author

I tested locally, all looks great to me except EmptyEndpointGroupException as follows when es is shutdown and connected failures:

2021-09-06 11:14:10,020 org.apache.skywalking.library.elasticsearch.ElasticSearch 149 [armeria-eventloop-nio-4-2] DEBUG [] - [creqId=3fe1891d, preqId=8179d41f][http://UNKNOWN/#GET] Request: {startTime=2021-09-06T03:14:10.010Z(1630898050010987), length=0B, duration=4538µs(4538520ns), cause=com.linecorp.armeria.client.UnprocessedRequestException: com.linecorp.armeria.client.endpoint.EmptyEndpointGroupException, scheme=none+http, name=GET, headers=[:method=GET, :path=/, :scheme=http, :authority=UNKNOWN]}
2021-09-06 11:14:10,021 org.apache.skywalking.library.elasticsearch.ElasticSearch 186 [armeria-eventloop-nio-4-2] WARN  [] - [creqId=3fe1891d, preqId=8179d41f][http://UNKNOWN/#GET] Response: {startTime=2021-09-06T03:14:10.020Z(1630898050020718), length=0B, duration=0ns, totalDuration=9749µs(9749393ns), cause=com.linecorp.armeria.client.UnprocessedRequestException: com.linecorp.armeria.client.endpoint.EmptyEndpointGroupException, headers=[:status=0]}
com.linecorp.armeria.client.UnprocessedRequestException: com.linecorp.armeria.client.endpoint.EmptyEndpointGroupException

I think EmptyEndpointGroupException is not enough clear to point out the cause of the error?

There is warning / error message before what you posted, also, it has health check registered .

2021-09-06 12:52:21,329 org.apache.skywalking.oap.server.telemetry.api.HealthCheckMetrics 50 [armeria-common-worker-nio-2-1] WARN  [] - Health check fails. reason: No healthy endpoint
2021-09-06 12:52:21,330 org.apache.skywalking.oap.server.library.client.elasticsearch.ElasticSearchClient 146 [armeria-common-worker-nio-2-1] ERROR [] - Failed to recreate ElasticSearch client based on new config

JaredTan95
JaredTan95 previously approved these changes Sep 6, 2021
Copy link
Member

@JaredTan95 JaredTan95 left a comment

Choose a reason for hiding this comment

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

LGTM

wu-sheng
wu-sheng previously approved these changes Sep 6, 2021
@kezhenxu94
Copy link
Member Author

Thank you @wu-sheng @JaredTan95 for reviewing and checking, I'm going to merge this

@wu-sheng
Copy link
Member

wu-sheng commented Sep 6, 2021

@wankai123 is confirming too.

Copy link
Contributor

@hanahmily hanahmily left a comment

Choose a reason for hiding this comment

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

docker/oap/Dockerfile.oap will be fixed in follow-up PRs?

@hanahmily
Copy link
Contributor

docker/oap/Dockerfile.oap will be fixed in follow-up PRs?

ignore this

Copy link
Contributor

@hanahmily hanahmily left a comment

Choose a reason for hiding this comment

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

hanahmily
hanahmily previously approved these changes Sep 6, 2021
Copy link
Contributor

@hanahmily hanahmily left a comment

Choose a reason for hiding this comment

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

LGTM

some nits regarding docker/.env

wankai123
wankai123 previously approved these changes Sep 6, 2021
Copy link
Member

@wankai123 wankai123 left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend OAP backend related. complexity:high Relate to multiple(>4) components of SkyWalking core feature Core and important feature. Sometimes, break backwards compatibility.
Projects
None yet
6 participants