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

Revert changes in AbstractPointGeometryFieldMapper #5250

Merged
merged 1 commit into from
Nov 15, 2022

Conversation

heemin32
Copy link
Contributor

The change made in AbstractPointGeometryFieldMapper class with commit 0503897 introduced a performace degradation during point data indexing. Reverting it therefore.

The change is about consolidating array format parsing code for point type in a single place to acheive following benefits.

  1. Allow plugins to override array parsing logic. Plugins can add its own parsing logic for point field by providing object parser. However, it cannot override array format. Therefore, plugin have to use whatever implemented in AbstractPointGeometryFieldMapper class.
  2. Enhanced code quality by removing duplicated code

There is no change in functionality because 1. There is no change in functionality in OpenSearch and 2. No plugins have its own parsing logic for point data in array format yet.

Signed-off-by: Heemin Kim heemin@amazon.com

Description

[Describe what this change achieves]

Issues Resolved

[List any issues this PR will resolve]

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

The change made in AbstractPointGeometryFieldMapper class
with commit 0503897 introduced
a performace degradation during point data indexing. Reverting it therefore.

The change is about consolidating array format parsing code for point type in a single place to acheive following benefits.
1. Allow plugins to override array parsing logic. Plugins can add its own parsing logic for point field by providing object parser. However, it cannot override array format. Therefore, plugin have to use whatever implemented in AbstractPointGeometryFieldMapper class.
2. Enhanced code quality by removing duplicated code

There is no change in functionality because 1. There is no change in functionality in OpenSearch and 2. No plugins have its own parsing logic for point data in array format yet.

Signed-off-by: Heemin Kim <heemin@amazon.com>
@heemin32 heemin32 requested review from a team and reta as code owners November 15, 2022 03:41
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@andrross
Copy link
Member

REPRODUCE WITH: ./gradlew ':plugins:repository-s3:yamlRestTest' --tests "org.opensearch.repositories.s3.RepositoryS3ClientYamlTestSuiteIT.test {yaml=repository_s3/20_repository_permanent_credentials/Snapshot and Restore with repository-s3 using permanent credentials}" -Dtests.seed=C17A7B3EB37A777E -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=es-GT -Dtests.timezone=Pacific/Johnston -Druntime.java=17 -Dtests.rest.denylist=repository_s3/30_repository_temporary_credentials/*,repository_s3/40_repository_ec2_credentials/*,repository_s3/50_repository_ecs_credentials/*,repository_s3/60_repository_eks_credentials/*

org.opensearch.repositories.s3.RepositoryS3ClientYamlTestSuiteIT > test {yaml=repository_s3/20_repository_permanent_credentials/Snapshot and Restore with repository-s3 using permanent credentials} FAILED
    java.lang.AssertionError: Failure at [repository_s3/20_repository_permanent_credentials:22]: expected [2xx] status code but api [snapshot.delete] returned [500 Internal Server Error]

Flaky test documented in #5219

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

Successfully merging this pull request may close these issues.

3 participants