Skip to content

Conversation

@hailin0
Copy link
Member

@hailin0 hailin0 commented Apr 23, 2025

Purpose of this pull request

Does this PR introduce any user-facing change?

How was this patch tested?

Check list

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request is aimed at improving error handling for duplicate labels in the StarRocks connector by catching and skipping batches when a label reuse is detected.

  • Injects a custom StarRocksStreamLoadVisitor via an overloaded constructor to facilitate testing.
  • Adds exception handling in flush() to catch an error indicating that a label has already been used.
  • Updates the tests to verify that a reused label skips the batch while other errors trigger retries.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
seatunnel-connectors-v2/connector-starrocks/src/main/java/org/apache/seatunnel/connectors/seatunnel/starrocks/client/StarRocksSinkManager.java Implements error handling for duplicate labels by checking exception messages and breaking out of the retry loop.
seatunnel-connectors-v2/connector-starrocks/src/test/java/org/apache/seatunnel/connectors/seatunnel/starrocks/client/StarRocksSinkManagerTest.java Adds tests to verify correct handling of duplicate label errors and propagation of other errors with retry behavior.
Comments suppressed due to low confidence (2)

seatunnel-connectors-v2/connector-starrocks/src/main/java/org/apache/seatunnel/connectors/seatunnel/starrocks/client/StarRocksSinkManager.java:102

  • Relying on string matching to identify an already-used label exception may be error-prone if exception messages change; consider using a dedicated exception type or error code for a more robust check.
String labelAlreadyMessage = String.format("Label [%s] has already been used", label);

seatunnel-connectors-v2/connector-starrocks/src/test/java/org/apache/seatunnel/connectors/seatunnel/starrocks/client/StarRocksSinkManagerTest.java:73

  • [nitpick] Consider renaming the test method to more clearly indicate that it verifies propagation of non-label exceptions (e.g., testNonLabelExceptionPropagation) to improve readability.
void testLabelAlreadyMessageNotHandled() throws Exception {

@hailin0 hailin0 marked this pull request as ready for review April 23, 2025 10:16
@hailin0 hailin0 force-pushed the dev-fix-sr branch 2 times, most recently from 3ca2240 to ce708b0 Compare April 23, 2025 12:30
@corgy-w corgy-w merged commit b6fc222 into apache:dev Apr 30, 2025
5 checks passed
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