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

feat(sink): support elasticsearch 8 sink #12269

Merged
merged 2 commits into from
Sep 14, 2023
Merged

feat(sink): support elasticsearch 8 sink #12269

merged 2 commits into from
Sep 14, 2023

Conversation

hzxa21
Copy link
Collaborator

@hzxa21 hzxa21 commented Sep 13, 2023

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Fix #12234

This PR adds setApiCompatibilityMode(true) for the RestHighLevelClient in the es 7 client library to make it compatible with es8 server. This is a simple workaround suggested in the es doc.

I have manually tested this PR with es 7.17 and es 8.10 server. Both work.

TODO:

  • A bunch of renames
  • Add integration tests for es7 and es8 sink.

Checklist

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
  • My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
  • All checks passed in ./risedev check (or alias, ./risedev c)
  • My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)
  • My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

  • My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

Support elasticsearch 7 and elasticsearch 8 sink. SQL Example:

CREATE SINK es_sink
FROM test_mv WITH (
    connector = 'elasticsearch',
    index = 'test',
    url = 'http://elasticsearch8:9200',
    username = 'elastic',
    password = 'risingwave'
    delimiter = ','
);

WITH options:

  • connector: required. Previously we use elasticsearch-7 as the connector name and now we switch to elasticsearch. elastic-search-7 is no longer a valid connector name, which is a potential breaking change. Given that es sink is not officially released, I don't think we need to mention this breaking change in the doc.
  • index: required. The target index name in elasticsearch.
  • url: required. The elasticsearch REST endpoint url.
  • username: optional. User name for the elasticsearch credential. It must be used with password.
  • password: optional. Password for the elasticsearch credential. It must be used with username.
  • delimiter: optional. Delimiter to construct elasticsearch id if the sink's primary key is composed of multiple columns.

Notes on ES id:

  • If sink has a primary key (normally it is inherited from the streaming job), we will use the primary key as the ES id.
  • If sink doesn't have a primary key (normally it is because the streaming job is append only), we will use the first column in the sink definition as the ES id.

@neverchanje
Copy link
Contributor

thanks Patrick! We may also need an additional integration test for Es8 in the future

@hzxa21 hzxa21 marked this pull request as ready for review September 14, 2023 03:39
@hzxa21
Copy link
Collaborator Author

hzxa21 commented Sep 14, 2023

thanks Patrick! We may also need an additional integration test for Es8 in the future

Added integration tests for es7 and es8 sink in the latest commit.

@hzxa21 hzxa21 added breaking-change user-facing-changes Contains changes that are visible to users labels Sep 14, 2023
@codecov
Copy link

codecov bot commented Sep 14, 2023

Codecov Report

Merging #12269 (d9f9194) into main (43852dc) will increase coverage by 0.00%.
Report is 25 commits behind head on main.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main   #12269   +/-   ##
=======================================
  Coverage   69.73%   69.73%           
=======================================
  Files        1409     1409           
  Lines      235961   235961           
=======================================
+ Hits       164553   164559    +6     
+ Misses      71408    71402    -6     
Flag Coverage Δ
rust 69.73% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
src/connector/src/sink/remote.rs 53.97% <ø> (ø)

... and 5 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@hzxa21 hzxa21 added this pull request to the merge queue Sep 14, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 14, 2023
@hzxa21 hzxa21 added this pull request to the merge queue Sep 14, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 14, 2023
@hzxa21 hzxa21 added this pull request to the merge queue Sep 14, 2023
Merged via the queue into main with commit f25c625 Sep 14, 2023
34 of 35 checks passed
@hzxa21 hzxa21 deleted the patrick/es8 branch September 14, 2023 06:34
Li0k pushed a commit that referenced this pull request Sep 15, 2023
@hzxa21 hzxa21 mentioned this pull request Sep 21, 2023
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change type/feature user-facing-changes Contains changes that are visible to users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracking: Support sink to elastic search 8
3 participants