refactor(sink): refactor es and opensearch to support async #17746

xxhZs · 2024-07-18T10:17:39Z

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

What's changed and what's your intention?

refactor es and opensearch with https://docs.rs/opensearch/latest/opensearch/ and https://docs.rs/elasticsearch/latest/elasticsearch/

they support async, and their sink_debuple defaults to true

#17073

resolve #17443

The new sink is exactly the same as the original creation parameters. For now, we keep two copies of the code (elasticsearch and elasticsearch_rust), after which we can delete the old one.

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
I have added test labels as necessary. See details.
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

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

wenym1

After we change to use the rust implementation by default, we can also run the es e2e so that the new logic implemented in this PR can be covered in the test.

wenym1 · 2024-09-06T10:59:52Z

src/connector/src/sink/opensearch.rs

+use super::{DummySinkCommitCoordinator, Sink, SinkError, SinkParam, SinkWriterParam};
+use crate::sink::Result;
+
+pub const OPENSEARCH_SINK: &str = "opensearch_rust";


In this PR we can just deprecate the original java implementation and always use the rust implementation when the connector is opensearch or elastic-search. As long as the with option is the same as the java implementation, we can switch to the rust implementation without any compatibility issue.

Already made the parameters consistent， but not sure about migrating in this pr

Calling it _rust doesn't look good because users shouldn't care about the implementation language.

We should either

Call it opensearch_v2 (Then replace like Deprecate the s3 source connector and rename s3_v2 to s3 #16234)

Directly replace it and deprecate old version like @wenym1 said. (Actually I also prefer this if possible)

Discussed with yim and decided to replace the previous implementation in this pr, so rust's will be renamed opensearch, and java's will remain, or is it called opensearch_v1?

If the new impl is called opensearch, and replaces the java impl, I think java impl will become dead code and we can directly remove it?

src/connector/src/sink/elasticsearch_opensearch_common.rs

src/connector/src/sink/opensearch.rs

remove print

wenym1

Do we have any test or CI to validate whether the current implementation works?

src/connector/src/sink/remote.rs

xxhZs · 2024-09-29T06:26:47Z

Do we have any test or CI to validate whether the current implementation works?

Yes, since we've changed our name to elasticsearch, the original tests can be used on the new implementation

fix ci fix ci fix ci fix ci

wenym1

Rest LGTM.

src/connector/src/sink/elasticsearch_opensearch/elasticsearch_converter.rs

wenym1 · 2024-09-29T08:37:10Z

e2e_test/sink/elasticsearch/elasticsearch_with_pk_sink.result

What causes the change in the result? Is the changes expected?

Yes, I noticed a missing column in the previous test

src/connector/src/sink/elasticsearch_opensearch/elasticsearch_opensearch_common.rs

xxchan · 2024-09-29T09:10:50Z

src/connector/src/sink/remote.rs

+            { ElasticSearchJava, ElasticSearchJavaSink, "elasticsearch_v1" }
+            { OpensearchJava, OpenSearchJavaSink, "opensearch_v1"}


May I ask why to keep _v1?

Since there are some users using java es, I'd like to be able to keep him for a while, in case something goes wrong with the migration to v2, so that they can use v1 first. Then delete v1 when v2 is more stable.

To clarify,

For users already using connector = elasticsearch, when they upgrade, they will switch to v2 immediately.

If they meet problems, they can manually create new sinks with elasticsearch_v1 as a temporary workaround.

Is this what you want?

The main concern for me is that when we offer elasticsearch_v1, it might be hard to delete the legacy implementation in the future. (It might be possible if we only make it an undocumented feature and tell only some users about it...)

Yes yes, I'm leaning towards,Only problems that are difficult to solve with v2 will make users try v1, and no updates will be made after v1, he may just exist for a version or two as a transition

For existing users, if they encounter any problem in this new rust implementation, instead of letting user recreate a sink with connector as _v1, I think we can make a patch and build a separate image to change back to use the java implementation for that users, and meanwhile investigate the fix the rust implementation.

If this is feasible, we can exclude the _v1 from the sink list.

wenym1

LGTM. Thanks for the PR.

wenym1 · 2024-09-29T10:23:30Z

src/connector/src/sink/remote.rs

+            { ElasticSearchJava, ElasticSearchJavaSink, "elasticsearch_v1" }
+            { OpensearchJava, OpenSearchJavaSink, "opensearch_v1"}


For existing users, if they encounter any problem in this new rust implementation, instead of letting user recreate a sink with connector as _v1, I think we can make a patch and build a separate image to change back to use the java implementation for that users, and meanwhile investigate the fix the rust implementation.

If this is feasible, we can exclude the _v1 from the sink list.

hzxa21

LGTM for Cargo.lock

xxhZs added 4 commits July 17, 2024 12:17

save

e8ec240

add all

464bd19

save

82ee305

save

a5cf6bd

xxhZs requested a review from a team as a code owner July 18, 2024 10:17

xxhZs requested a review from lmatz July 18, 2024 10:17

github-actions bot added the type/refactor label Jul 18, 2024

xxhZs added 5 commits July 18, 2024 18:23

fix cargo lock

92db3d4

save

5ae7a0d

fix cargo lock

caf1a80

fix cargo lock

94d4de1

fix ut

a146ff7

xxhZs requested a review from wenym1 July 22, 2024 08:21

xxhZs mentioned this pull request Aug 14, 2024

Tracking: enable sink_decouple by default for all types of sink #17095

Closed

31 tasks

wenym1 reviewed Sep 6, 2024

View reviewed changes

xxhZs added 3 commits September 19, 2024 15:58

Merge branch 'main' into xxh/es-opensearch-sink

30a908e

add

535b0b6

fix comm

88264e5

remove print

xxhZs force-pushed the xxh/es-opensearch-sink branch from a663b69 to 88264e5 Compare September 23, 2024 07:18

graphite-app bot requested a review from a team September 23, 2024 07:40

xxhZs added 3 commits September 26, 2024 14:11

Merge branch 'main' into xxh/es-opensearch-sink

0487361

add file

172ae02

fix comm

fa833d3

xxhZs requested a review from wenym1 September 29, 2024 05:00

wenym1 reviewed Sep 29, 2024

View reviewed changes

src/connector/src/sink/remote.rs Outdated Show resolved Hide resolved

fix ci

5beea04

Merge branch 'main' into xxh/es-opensearch-sink

e24676c

xxhZs force-pushed the xxh/es-opensearch-sink branch 2 times, most recently from 5787b79 to 0086ea9 Compare September 29, 2024 07:58

xxhZs force-pushed the xxh/es-opensearch-sink branch from 0086ea9 to 66e902d Compare September 29, 2024 08:07

fix ut

202f0e4

fix ci fix ci fix ci fix ci

xxhZs force-pushed the xxh/es-opensearch-sink branch from 66e902d to 202f0e4 Compare September 29, 2024 08:41

wenym1 reviewed Sep 29, 2024

View reviewed changes

graphite-app bot requested a review from a team September 29, 2024 09:10

xxchan reviewed Sep 29, 2024

View reviewed changes

xxhZs added 2 commits September 29, 2024 18:08

fix comm

36cdab5

fix ut

f2ce065

wenym1 approved these changes Sep 29, 2024

View reviewed changes

fix comm

67659d5

xxhZs requested a review from hzxa21 October 9, 2024 03:13

fix ci

02560db

hzxa21 approved these changes Oct 9, 2024

View reviewed changes

xxhZs added this pull request to the merge queue Oct 10, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 10, 2024

xxhZs added this pull request to the merge queue Oct 10, 2024

Merged via the queue into main with commit 840c37b Oct 10, 2024
29 of 30 checks passed

xxhZs deleted the xxh/es-opensearch-sink branch October 10, 2024 09:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(sink): refactor es and opensearch to support async #17746

refactor(sink): refactor es and opensearch to support async #17746

xxhZs commented Jul 18, 2024 •

edited by wenym1

Loading

wenym1 left a comment

wenym1 Sep 6, 2024

xxhZs Sep 25, 2024

xxchan Sep 25, 2024

xxhZs Sep 26, 2024

xxchan Sep 26, 2024

wenym1 left a comment

xxhZs commented Sep 29, 2024

wenym1 left a comment

wenym1 Sep 29, 2024

xxhZs Sep 29, 2024 •

edited

Loading

xxchan Sep 29, 2024

xxhZs Sep 29, 2024 •

edited

Loading

xxchan Sep 29, 2024

xxhZs Sep 29, 2024

wenym1 Sep 29, 2024

wenym1 left a comment

wenym1 Sep 29, 2024

hzxa21 left a comment

		{ ElasticSearchJava, ElasticSearchJavaSink, "elasticsearch_v1" }
		{ OpensearchJava, OpenSearchJavaSink, "opensearch_v1"}

refactor(sink): refactor es and opensearch to support async #17746

refactor(sink): refactor es and opensearch to support async #17746

Conversation

xxhZs commented Jul 18, 2024 • edited by wenym1 Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

wenym1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenym1 left a comment

Choose a reason for hiding this comment

xxhZs commented Sep 29, 2024

wenym1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xxhZs Sep 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xxhZs Sep 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenym1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzxa21 left a comment

Choose a reason for hiding this comment

xxhZs commented Jul 18, 2024 •

edited by wenym1

Loading

xxhZs Sep 29, 2024 •

edited

Loading

xxhZs Sep 29, 2024 •

edited

Loading