Skip to content

Fix issues in ElasticSearch 7.14 and add log cases to ES7.14#7429

Merged
kezhenxu94 merged 1 commit intomasterfrom
es7.14
Aug 10, 2021
Merged

Fix issues in ElasticSearch 7.14 and add log cases to ES7.14#7429
kezhenxu94 merged 1 commit intomasterfrom
es7.14

Conversation

@kezhenxu94
Copy link
Member

@kezhenxu94 kezhenxu94 added bug Something isn't working and you are sure it's a bug! backend OAP backend related. labels Aug 10, 2021
@kezhenxu94 kezhenxu94 added this to the 8.8.0 milestone Aug 10, 2021
@kezhenxu94 kezhenxu94 requested a review from wu-sheng August 10, 2021 05:57
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.

Let's limit the not null in the certain scopes.

@kezhenxu94
Copy link
Member Author

Let's limit the not null in the certain scopes.

The nullability check is added according to whether they are annotated with org.apache.skywalking.oap.server.core.storage.annotation.Column#matchQuery that will add a copy_to field, which causes the problem. If we skip some of the cases mentioned in the comments, the root cause (serviceName being null) is hidden behind the ElasticSearch level message, although serviceName is expected to be non null,

@codecov
Copy link

codecov bot commented Aug 10, 2021

Codecov Report

Merging #7429 (c60bb4a) into master (cc66254) will decrease coverage by 0.84%.
The diff coverage is 25.00%.

❗ Current head c60bb4a differs from pull request most recent head 87b3e42. Consider uploading reports for the commit 87b3e42 to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##             master    #7429      +/-   ##
============================================
- Coverage     53.14%   52.30%   -0.85%     
+ Complexity     4335     4005     -330     
============================================
  Files          1899     1034     -865     
  Lines         41163    26449   -14714     
  Branches       4620     2620    -2000     
============================================
- Hits          21877    13833    -8044     
+ Misses        18152    11408    -6744     
- Partials       1134     1208      +74     
Impacted Files Coverage Δ
...er/core/analysis/manual/log/AbstractLogRecord.java 45.94% <0.00%> (-4.06%) ⬇️
...browser/manual/errorlog/BrowserErrorLogRecord.java 45.16% <50.00%> (-1.51%) ⬇️
...ing/oap/server/core/storage/IHistoryDeleteDAO.java 0.00% <0.00%> (-100.00%) ⬇️
...g/oap/server/core/cluster/ClusterHealthStatus.java 0.00% <0.00%> (-100.00%) ⬇️
...p/server/core/analysis/metrics/MinLongMetrics.java 0.00% <0.00%> (-100.00%) ⬇️
...er/exporter/provider/grpc/GRPCExporterSetting.java 0.00% <0.00%> (-100.00%) ⬇️
...log/analyzer/provider/log/ILogAnalyzerService.java 0.00% <0.00%> (-100.00%) ⬇️
...king/oap/server/configuration/api/ConfigTable.java 0.00% <0.00%> (-100.00%) ⬇️
...analyzer/provider/meter/process/SampleBuilder.java 0.00% <0.00%> (-100.00%) ⬇️
...skywalking/oap/server/receiver/envoy/als/Role.java 0.00% <0.00%> (-100.00%) ⬇️
... and 1378 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc66254...87b3e42. Read the comment docs.

@wu-sheng
Copy link
Member

Let's limit the not null in the certain scopes.

The nullability check is added according to whether they are annotated with org.apache.skywalking.oap.server.core.storage.annotation.Column#matchQuery that will add a copy_to field, which causes the problem. If we skip some of the cases mentioned in the comments, the root cause (serviceName being null) is hidden behind the ElasticSearch level message, although serviceName is expected to be non null,

The changes on the log side are reasonable, and should be done. But why we bother to check these should-not-null fields? If you want to check and expose the cause, check on the source#prepare stage rather than here.

@wu-sheng wu-sheng linked an issue Aug 10, 2021 that may be closed by this pull request
4 tasks
@kezhenxu94 kezhenxu94 merged commit 85e22f5 into master Aug 10, 2021
@kezhenxu94 kezhenxu94 deleted the es7.14 branch August 10, 2021 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend OAP backend related. bug Something isn't working and you are sure it's a bug!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Exception mapper_parsing_exception in ElasticSearch 7.14

2 participants

Comments