DanielCarter-stack commented on PR #10400:
URL: https://github.com/apache/seatunnel/pull/10400#issuecomment-3803408943
<!-- code-pr-reviewer -->
<!-- cpr:pr_reply_v2_parts {"group": "apache/seatunnel#10400", "part": 1,
"total": 1} -->
### Issue 1: Semantic Ambiguity of maxRetries=0
**Location**: `JdbcOutputFormat.java:137`
```java
for (int attempt = 0; attempt <= maxRetries; attempt++) {
```
**Problem Description**:
When `maxRetries=0` (default value), the loop executes once (attempt=0),
which contradicts the intuition that "maxRetries=0 means no retry".
**Related Context**:
- Configuration definition: `JdbcSinkOptions.java:70` - `defaultValue(0)`
- Old code: `for (int i = 0; i <= jdbcConnectionConfig.getMaxRetries();
i++)` - Same issue
- Callers: `JdbcSinkWriter`, `JdbcExactlyOnceSinkWriter` - Depend on
`maxRetries` configuration
**Potential Risks**:
- When users configure `max_retries=0`, they expect "stop on failure", but
it actually tries once
- If this is the design intent, the configuration name should be
`maxAttempts` instead of `maxRetries`
**Impact Scope**:
- **Direct Impact**: All users using JDBC Sink
- **Impact Area**: Entire JDBC Connector
**Severity**: MINOR (This is a legacy issue, not introduced by this PR)
**Improvement Suggestions**:
```java
// Solution 1: Change semantics (breaking change)
for (int attempt = 0; attempt < maxRetries; attempt++) { // Note < instead
of <=
// ...
}
// And change default value to 1
// Solution 2: Rename configuration item (recommended)
public static final Option<Integer> MAX_ATTEMPTS =
Options.key("max_attempts").intType().defaultValue(1);
```
**Rationale**:
- Fixes semantic ambiguity, matches user intuition
- `max_attempts=1` means "at most 1 attempt", `max_attempts=3` means "at
most 3 attempts (retry 2 times)"
---
### Issue 2: Incomplete SQLState Detection Coverage
**Location**: `JdbcOutputFormat.java:163`
```java
boolean sqlStateConnError = sqlState != null && sqlState.startsWith("08");
```
**Problem Description**:
Only detects SQLState "08xxx" (connection exception), but other databases
may use different error codes:
- Oracle: ORA-00028 (session has been killed)
- PostgreSQL: 57xxx (system error, possibly connection issue)
- SQL Server: May use custom error codes
**Related Context**:
- SQL Standard: 08xxx is indeed "Connection Exception"
- Real-world scenarios: Some drivers don't strictly follow the standard
**Potential Risks**:
- Certain connection errors may not be detected, leading to invalid retries
- Example: PostgreSQL network timeout may return non-08xxx error codes
**Impact Scope**:
- **Direct Impact**: Users of databases using non-standard SQLState
- **Indirect Impact**: Retry logic may fail, causing task failures
**Severity**: MAJOR
**Improvement Suggestions**:
```java
// Solution 1: Configurable SQLState pattern
private boolean isConnectionError(SQLException sqlEx) {
String sqlState = sqlEx.getSQLState();
if (sqlState == null) {
return false;
}
// Standard connection error
if (sqlState.startsWith("08")) {
return true;
}
// Database-specific connection error
String vendorCode = getVendorSpecificCode(sqlEx);
return CONNECTION_ERROR_CODES.contains(vendorCode);
}
// Solution 2: Extend configuration item
public static final Option<List<String>> CONNECTION_ERROR_SQLSTATES =
Options.key("connection_error_sqlstates")
.listType()
.defaultValue(Arrays.asList("08"))
.withDescription("SQLState patterns indicating connection errors");
```
**Rationale**:
- Improves compatibility with different databases
- Allows users to adjust based on actual conditions
---
### Issue 3: Missing Unit Tests
**Location**: The entire PR has no corresponding test files
**Problem Description**:
The newly added SQLState detection logic, reconnection logic, and exception
propagation logic have no unit test coverage.
**Related Context**:
- Existing tests: `JdbcOutputFormatBuilderTest.java` - Only tests format
building
- Test framework: Project uses JUnit 5 and Mockito
**Potential Risks**:
- Regression risk: Future modifications may break existing logic
- Edge cases not verified:
- `sqlState` is null
- `isConnectionValid()` throws exception
- Retry after reconnection failure
- `maxRetries=0` behavior
**Impact Scope**:
- **Direct Impact**: Code quality and maintainability
- **Impact Area**: JDBC Connector stability
**Severity**: MAJOR
**Improvement Suggestions**:
```java
// New file: JdbcOutputFormatRetryTest.java
public class JdbcOutputFormatRetryTest {
@Test
public void testFlushWithSqlConnectionError() throws Exception {
// Mock: SQLState=08003, isValid()=true
// Verify: call updateExecutor(true)
}
@Test
public void testFlushWithValidConnectionButSqlError() throws Exception {
// Mock: SQLState=23000 (unique key conflict), isValid()=true
// Verify: do not call updateExecutor, but retry
}
@Test
public void testFlushWithNullSqlState() throws Exception {
// Mock: SQLState=null, isValid()=false
// Verify: call updateExecutor(true)
}
@Test
public void testUpdateExecutorSuppressedException() throws Exception {
// Mock: closeStatements() throws exception, prepareStatements()
also throws exception
// Verify: second exception contains suppressed exception
}
}
```
**Rationale**:
- Ensures core logic correctness
- Prevents future regressions
- Provides usage examples
---
### Issue 4: Missing Exponential Backoff Configuration
**Location**: `JdbcOutputFormat.java:135-136`
```java
final long baseBackoffMs = 1000L;
final long maxBackoffMs = 10_000L;
```
**Problem Description**:
Backoff time is hardcoded and cannot be adjusted according to actual
scenarios.
**Related Context**:
- Old code: `sleepMs = 1000` - Also hardcoded
- Similar configurations: `JdbcConnectionConfig` already has
`socketTimeoutMs`, `connectTimeoutMs`
**Potential Risks**:
- In some scenarios, 1 second may be too short (frequent retries, increased
database load)
- In some scenarios, 10 seconds may be too long (excessive task delay)
**Impact Scope**:
- **Direct Impact**: Users who need to tune retry strategies
- **Impact Area**: JDBC Sink performance
**Severity**: MINOR
**Improvement Suggestions**:
```java
// JdbcSinkOptions.java
public static final Option<Long> RETRY_BACKOFF_BASE_MS =
Options.key("retry_backoff_base_ms")
.longType()
.defaultValue(1000L)
.withDescription("Base backoff time for retry (milliseconds)");
public static final Option<Long> RETRY_BACKOFF_MAX_MS =
Options.key("retry_backoff_max_ms")
.longType()
.defaultValue(10000L)
.withDescription("Max backoff time for retry (milliseconds)");
// JdbcOutputFormat.java
final long baseBackoffMs = jdbcConnectionConfig.getRetryBackoffBaseMs();
final long maxBackoffMs = jdbcConnectionConfig.getRetryBackoffMaxMs();
```
**Rationale**:
- Provides flexibility to adapt to different scenarios
- Consistent with existing timeout configurations
---
### Issue 5: Inconsistent Exception Handling in close() Method
**Location**: `JdbcOutputFormat.java:251-262`
```java
if (batchCount > 0) {
try {
flush();
} catch (Exception e) {
LOG.warn("Writing records to JDBC failed.", e);
flushException = new JdbcConnectorException(
CommonErrorCodeDeprecated.FLUSH_DATA_FAILED,
"Writing records to JDBC failed.",
e);
}
}
```
**Problem Description**:
When `flush()` fails in the `close()` method, the exception is saved to
`flushException`, but `connectionProvider.closeConnection()` is still executed
(line 272).
**Related Context**:
- `close()` method signature: `public synchronized void close()` - Does not
throw checked exceptions
- Caller: `JdbcSinkWriter.close()` - Depends on `flush()` throwing exception
**Potential Risks**:
- If `flush()` fails, `close()` won't throw exception (only saved to
`flushException`)
- The final `checkFlushException()` will throw exception, but log order may
be chaotic
- Inconsistent with `JdbcSinkWriter.close()` expectations (caller may expect
exception when `close()` fails)
**Impact Scope**:
- **Direct Impact**: `JdbcSinkWriter`, `JdbcExactlyOnceSinkWriter`
- **Impact Area**: JDBC Sink error handling semantics
**Severity**: MINOR (This is a legacy issue, PR introduces no new
inconsistencies)
**Improvement Suggestions**:
```java
// Solution 1: Throw exception in close()
public synchronized void close() throws IOException {
if (!closed) {
closed = true;
if (batchCount > 0) {
try {
flush();
} catch (Exception e) {
LOG.warn("Writing records to JDBC failed.", e);
throw new JdbcConnectorException( // Throw directly
CommonErrorCodeDeprecated.FLUSH_DATA_FAILED,
"Writing records to JDBC failed.",
e);
}
}
try {
if (jdbcStatementExecutor != null) {
jdbcStatementExecutor.closeStatements();
}
} catch (SQLException | JdbcConnectorException e) {
LOG.warn("Close JDBC writer failed.", e);
}
}
connectionProvider.closeConnection();
}
```
**Rationale**:
- `close()` should report critical errors
- More consistent with `java.io.Closeable` semantics
---
### Issue 6: Overly Strict Handling of Non-SQL Exceptions
**Location**: `JdbcOutputFormat.java:149-159`
```java
Throwable root = Throwables.getRootCause(e);
if (!(root instanceof SQLException)) {
LOG.error("Flush failed (non-SQL). batchCount={}, attempt={}/{}", ...);
throw new JdbcConnectorException(
CommonErrorCodeDeprecated.FLUSH_DATA_FAILED, e);
}
```
**Problem Description**:
Any non-SQL exception causes immediate failure without retry.
**Related Context**:
- Possible non-SQL exceptions:
- `NullPointerException` (code defect)
- `IOException` (network issue, possibly temporary)
- `RuntimeException` (various reasons)
**Potential Risks**:
- Some temporary non-SQL exceptions (e.g., IOException caused by network
jitter) cannot recover through retry
- But in most cases this is the **correct behavior** (non-SQL exceptions
usually indicate code defects)
**Impact Scope**:
- **Direct Impact**: Exception handling strategy
- **Impact Area**: Error recovery capability
**Severity**: MINOR (Current strategy is reasonable, but can be more
flexible)
**Improvement Suggestions**:
```java
// Configurable retry exception types
private boolean shouldRetry(Throwable t) {
if (t instanceof SQLException) {
return true;
}
// Configurable retryable exception types
for (Class<? extends Throwable> retryableType :
jdbcConnectionConfig.getRetryableExceptions()) {
if (retryableType.isInstance(t)) {
return true;
}
}
return false;
}
```
**Rationale**:
- Provides flexibility, allows certain non-SQL exceptions to retry
- But keeps strict by default (only retry SQL exceptions)
---
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
