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 starrocks sink #12681

Merged
merged 11 commits into from
Nov 8, 2023
Merged

feat(sink): support starrocks sink #12681

merged 11 commits into from
Nov 8, 2023

Conversation

xxhZs
Copy link
Contributor

@xxhZs xxhZs commented Oct 8, 2023

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

What's changed and what's your intention?

support starrocks sink. Can't support struct and json. because starrocks's stream load don't support it.

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

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.

@xxhZs xxhZs force-pushed the xxh/starrock-sink branch from 8d799b4 to b5b982d Compare October 9, 2023 05:49
@xxhZs xxhZs marked this pull request as ready for review October 9, 2023 05:51
@xxhZs xxhZs requested review from wenym1, tabVersion and hzxa21 October 9, 2023 05:52
@xxhZs xxhZs force-pushed the xxh/starrock-sink branch from b5b982d to aed8ef7 Compare October 9, 2023 06:39
Copy link
Contributor

Choose a reason for hiding this comment

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

what does doris_connector mean here? we have separate doris and starrocks

Copy link
Contributor Author

@xxhZs xxhZs Oct 9, 2023

Choose a reason for hiding this comment

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

Starrocks and Doris both use HTTP for data import, and here is where their logic is identical (calling the HTTP interface).

src/connector/src/common.rs Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 9, 2023

Codecov Report

Merging #12681 (5260a2d) into main (e3c8649) will decrease coverage by 0.12%.
Report is 11 commits behind head on main.
The diff coverage is 0.28%.

@@            Coverage Diff             @@
##             main   #12681      +/-   ##
==========================================
- Coverage   68.00%   67.89%   -0.12%     
==========================================
  Files        1521     1522       +1     
  Lines      257858   258289     +431     
==========================================
+ Hits       175364   175365       +1     
- Misses      82494    82924     +430     
Flag Coverage Δ
rust 67.89% <0.28%> (-0.12%) ⬇️

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

Files Coverage Δ
src/connector/src/common.rs 1.85% <ø> (ø)
src/connector/src/sink/mod.rs 60.71% <ø> (ø)
src/connector/src/sink/encoder/json.rs 87.18% <50.00%> (+0.02%) ⬆️
src/connector/src/sink/doris.rs 0.00% <0.00%> (ø)
...rc/connector/src/sink/doris_starrocks_connector.rs 0.00% <0.00%> (ø)
src/connector/src/sink/starrocks.rs 0.00% <0.00%> (ø)

... and 2 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

pub(crate) const STARROCKS_DELETE_SIGN: &str = "__op";
const STARROCK_MYSQL_PREFER_SOCKET: &str = "false";
const STARROCK_MYSQL_MAX_ALLOWED_PACKET: usize = 1024;
const STARROCK_MYSQL_WAIT_TIMEOUT: usize = 31536000;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this ms or s? Why 31536000? Any special consideration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is because initially we were directly querying in MySQL. Since Starrock doesn't have this table, it required us to set up bug prevention. We set the default value, but the value 31536000 was incorrect due to an error in the location of the query. After checking the official website, it should be 28800, and it has been updated.

src/connector/src/sink/doris_starrocks_connector.rs Outdated Show resolved Hide resolved
src/connector/src/sink/doris_starrocks_connector.rs Outdated Show resolved Hide resolved
@xxhZs xxhZs requested a review from hzxa21 October 23, 2023 09:00
@fuyufjh fuyufjh requested review from tabVersion and fuyufjh October 25, 2023 03:39
Copy link
Collaborator

@hzxa21 hzxa21 left a comment

Choose a reason for hiding this comment

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

Rest LGTM

src/connector/src/sink/doris_starrocks_connector.rs Outdated Show resolved Hide resolved
@@ -388,10 +390,12 @@ pub enum SinkError {
Redis(String),
#[error("Nats error: {0}")]
Nats(anyhow::Error),
#[error("Doris http error: {0}")]
#[error("Doris/Starrocks connect error: {0}")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Better name it http error?

@xxhZs xxhZs enabled auto-merge November 8, 2023 05:57
@xxhZs xxhZs added this pull request to the merge queue Nov 8, 2023
Merged via the queue into main with commit f42a612 Nov 8, 2023
6 of 8 checks passed
@xxhZs xxhZs deleted the xxh/starrock-sink branch November 8, 2023 06:44
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