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

Migrated Data Prepper to use the opensearch-java client for bulk requests #1381

Merged
merged 1 commit into from
May 13, 2022

Conversation

dlvenable
Copy link
Member

@dlvenable dlvenable commented May 12, 2022

Description

This PR updates Data Prepper to use the opensearch-java client for _bulk requests to OpenSearch. It does not update all interactions to the opensearch-java client. Others can come in subsequent PRs.

With this change, Data Prepper also runs against OpenSearch 2.0.0-rc1.

Related changes included in this PR include:

  • Updated the OpenSearchSinkIT tests to test against both the Record<String> and Record<Event> models. Previously these tests only ran against Record<Sink>.
  • Updated the OpenSearch client to 1.3.2, which is the current latest for the 1.x series.

Issues Resolved

Resolves #1347

Check List

  • New functionality includes testing.
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed with a real name per the DCO

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.

…ests rather than the REST High Level Client. opensearch-project#1347

Signed-off-by: David Venable <[email protected]>
@dlvenable dlvenable requested a review from a team as a code owner May 12, 2022 15:20
@codecov-commenter
Copy link

codecov-commenter commented May 12, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.08%. Comparing base (d0ccedd) to head (d57315b).
Report is 1673 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #1381   +/-   ##
=========================================
  Coverage     94.08%   94.08%           
  Complexity     1156     1156           
=========================================
  Files           162      162           
  Lines          3248     3248           
  Branches        263      263           
=========================================
  Hits           3056     3056           
  Misses          138      138           
  Partials         54       54           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@asifsmohammed asifsmohammed left a comment

Choose a reason for hiding this comment

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

LGTM

JsonProvider jsonProvider = jsonpMapper.jsonProvider();
final JsonData jsonData = JsonData.from(jsonProvider.createParser(new StringReader(jsonString)), jsonpMapper);

final String serializedJsonLength = jsonData.toJson().toString();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Variable name should be serializedJson or should have length() method in this line.

Copy link
Collaborator

@chenqi0805 chenqi0805 left a comment

Choose a reason for hiding this comment

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

LGTM!

import com.fasterxml.jackson.databind.ObjectMapper;
import io.micrometer.core.instrument.Measurement;
import org.apache.commons.io.FileUtils;
import org.apache.http.util.EntityUtils;
import org.hamcrest.MatcherAssert;
import org.junit.After;
import org.junit.Assert;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since now we are using JUnit 5, this could be replaced. Could be in follow up PRs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I agree. This change is already significant, and so I wanted to minimize the number of unnecessary changes. I noticed this when doing it, but didn't want to add even more change.

@dlvenable dlvenable merged commit 8b7d39c into opensearch-project:main May 13, 2022
@dlvenable dlvenable deleted the opensearch-java-client branch May 17, 2022 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update to use the opensearch-java client for _bulk requests
4 participants