[Improve][Starrocks] Catch lable already exception#9222
Merged
Conversation
Contributor
There was a problem hiding this comment.
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 {
3ca2240 to
ce708b0
Compare
Hisoka-X
approved these changes
Apr 29, 2025
corgy-w
approved these changes
Apr 30, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose of this pull request
Does this PR introduce any user-facing change?
How was this patch tested?
Check list
New License Guide
release-note.