DanielCarter-stack commented on PR #10501:
URL: https://github.com/apache/seatunnel/pull/10501#issuecomment-3935620459
<!-- code-pr-reviewer -->
<!-- cpr:pr_reply_v2_parts {"group": "apache/seatunnel#10501", "part": 1,
"total": 1} -->
### Issue 1: CI timeout configuration lacks reasonable justification
**Location**: `.github/workflows/backend.yml:1412`
**Related code**:
```yaml
connector-file-local-it:
timeout-minutes: 150 # 从 120 改为 150
```
**Issue description**:
The PR increases the timeout of the `connector-file-local-it` job from 120
minutes to 150 minutes without providing justification. This job tests the
local file connector, which theoretically should not require such a long
timeout.
**Potential risks**:
- Risk 1: May mask issues with test execution efficiency degradation
- Risk 2: Indiscriminate timeout increase may lead to CI resource waste
**Impact scope**:
- Direct impact: `connector-file-local-it` job
- Indirect impact: CI execution time and resource consumption
- Impact surface: Single E2E job
**Severity**: MINOR
**Improvement suggestions**:
```yaml
# Suggest adding timeout only to jobs that require long execution time
# Timeout for connector-file-local-it should remain 120 minutes
# If increasing is truly necessary, explain the reason in the commit message
```
**Rationale**:
Timeout should be based on actual test execution time, not indiscriminate
increases. Local file connector tests should typically complete within 30
minutes.
---
### Issue 2: K8s prebuild step lacks failure handling
**Location**: `.github/workflows/backend.yml:702-720`
**Related code**:
```yaml
- name: run seatunnel zeta on k8s test
run: |
./mvnw -B install -DskipTests -am
TARGET_PATH="seatunnel-e2e/seatunnel-engine-e2e/seatunnel-engine-k8s-e2e/src/test/resources"
mkdir -p $TARGET_PATH/jars $TARGET_PATH/bin $TARGET_PATH/connectors
$TARGET_PATH/config
cp -r config/* $TARGET_PATH/config/
# ... large number of cp commands
docker build -t seatunnel:latest -f $TARGET_PATH/seatunnel_dockerfile
$TARGET_PATH
docker save seatunnel:latest | sudo k3s ctr images import -
```
**Issue description**:
The newly added prebuild step contains multiple commands that may fail
(`cp`, `docker build`, `k3s ctr`), but there is no error handling. If an
intermediate step fails, it will be difficult to debug.
**Potential risks**:
- Risk 1: When the `cp` command fails, it will silently continue, causing
test failures but making it difficult to locate the root cause
- Risk 2: Docker build failures lack error logs
- Risk 3: `k3s ctr images import` failure may cause k3s to use old images
**Impact scope**:
- Direct impact: `k8s-e2e-test` job
- Indirect impact: K8s-related CI stability
- Impact surface: Single CI job
**Severity**: MAJOR
**Improvement suggestions**:
```yaml
- name: run seatunnel zeta on k8s test
run: |
set -e # 遇到错误立即退出
set -u # 使用未定义变量时报错
set -o pipefail # 管道命令中任一失败则整体失败
./mvnw -B install -DskipTests -am
TARGET_PATH="seatunnel-e2e/seatunnel-engine-e2e/seatunnel-engine-k8s-e2e/src/test/resources"
mkdir -p $TARGET_PATH/jars $TARGET_PATH/bin $TARGET_PATH/connectors
$TARGET_PATH/config
echo "Copying config files..."
cp -r config/* $TARGET_PATH/config/ || { echo "Failed to copy config";
exit 1; }
cp $TARGET_PATH/custom_config/hazelcast-kubernetes-discovery.yaml
$TARGET_PATH/config/hazelcast.yaml || { echo "Failed to copy hazelcast config";
exit 1; }
# ... other cp commands
echo "Building Docker image..."
docker build -t seatunnel:latest -f $TARGET_PATH/seatunnel_dockerfile
$TARGET_PATH || { echo "Docker build failed"; exit 1; }
echo "Importing image to k3s..."
docker save seatunnel:latest | sudo k3s ctr images import - || { echo
"Failed to import image to k3s"; exit 1; }
echo "Pre-build setup completed successfully"
```
**Rationale**:
Error handling in CI configuration is critical. Adding `set -e` and explicit
error checks can quickly identify issues.
---
### Issue 3: Testcontainers version跨越过大 lacks compatibility verification
**Location**: `pom.xml:141`
**Related code**:
```xml
<testcontainer.version>1.17.6</testcontainer.version> → 1.21.4
```
**Issue description**:
Testcontainers upgrade from 1.17.6 to 1.21.4 spans 4 minor versions (1.18,
1.19, 1.20, 1.21), and the PR lacks verification and explanation of breaking
changes between versions.
**Potential risks**:
- Risk 1: Some modules may depend on deprecated APIs
- Risk 2: New versions may introduce new container startup restrictions
- Risk 3: May affect local development environment (developers' local Docker
versions may be older)
**Impact scope**:
- Direct impact: All E2E tests using Testcontainers (~35 modules)
- Indirect impact: Local development environment
- Impact surface: Multiple Connector E2E
**Severity**: MAJOR
**Improvement suggestions**:
1. Add Testcontainers version change release notes link in PR description
2. List major breaking changes between 1.17.6 → 1.21.4
3. Explain the verified compatibility testing scope
**Example supplement**:
```markdown
### Compatibility Notes
Testcontainers version upgrade path:
- 1.17.6 → 1.21.4 (跨越 4 个小版本)
Breaking changes to be aware of:
- [参考官方 changelog]
Verified scenarios:
- ✅ All E2E tests passed in CI
- ⚠️ Local developers need Docker API ≥1.44
```
**Rationale**:
Large version spans require more careful verification, and documented risk
explanations help developers understand and respond.
---
### Issue 4: connector-redis Testcontainers dependency lacks version
declaration
**Location**: `seatunnel-connectors-v2/connector-redis/pom.xml:70-73`
**Related code**:
```xml
<!-- 修改前 -->
<dependency>
<groupId>org.testcontainers</groupId>
<artifactId>testcontainers</artifactId>
<scope>test</scope>
</dependency>
<!-- 修改后 -->
<dependency>
<groupId>org.testcontainers</groupId>
<artifactId>testcontainers</artifactId>
<version>${testcontainer.version}</version> <!-- 新增 -->
<scope>test</scope>
</dependency>
```
**Issue description**:
Before the modification, `connector-redis`'s testcontainers dependency did
not explicitly declare a version, relying on the parent POM's
dependencyManagement. After the modification, the version is explicitly
declared, which is the correct behavior, but the **inconsistency** lies in:
- Other modules (such as `connector-cdc-mysql`) also do not explicitly
declare versions
- The PR only fixes `connector-redis` without checking whether other modules
have the same issue
**Potential risks**:
- Risk 1: Other modules may also have implicit version dependency issues
- Risk 2: If dependencyManagement configuration changes in the future,
version inconsistencies may result
**Impact scope**:
- Direct impact: `connector-redis` module
- Indirect impact: Other unchecked modules
- Impact surface: Single Connector
**Severity**: MINOR
**Improvement suggestions**:
1. Globally check all testcontainers dependencies to ensure explicit version
declarations
2. Or uniformly manage in parent POM's dependencyManagement to avoid
explicit declarations in child modules
**Rationale**:
Consistent dependency version management is very important—either all
explicitly declared or all managed through dependencyManagement.
---
### Issue 5: QdrantIT test modification introduces type mismatch risk
**Location**:
`seatunnel-e2e/seatunnel-connector-v2-e2e/connector-qdrant-e2e/src/test/java/org/apache/seatunnel/e2e/connector/v2/qdrant/QdrantIT.java:146`
**Related code**:
```java
// Before modification
Assertions.assertEquals(qdrantClient.countAsync(SINK_COLLECTION).get(), 10);
// After modification
Assertions.assertEquals(10L, qdrantClient.countAsync(SINK_COLLECTION).get());
```
**Issue description**:
After the modification, `10L` (long type) is used, but the return type of
`countAsync()` is uncertain. If it returns `Integer`, this will cause a type
mismatch.
**Potential risks**:
- Risk 1: If `countAsync()` returns `Integer`, compilation errors will result
- Risk 2: If the return type changes in future versions, test failures may
occur
**Impact scope**:
- Direct impact: `QdrantIT.testQdrant()` method
- Indirect impact: Qdrant connector test coverage
- Impact surface: Single E2E test
**Severity**: MINOR
**Improvement suggestions**:
```java
// Option 1: Use type inference
Assertions.assertEquals(10, qdrantClient.countAsync(SINK_COLLECTION).get());
// Option 2: Explicitly convert type
long count = qdrantClient.countAsync(SINK_COLLECTION).get();
Assertions.assertEquals(10L, count);
```
**Rationale**:
Type consistency is important; implicit type conversions should be avoided.
---
### Issue 6: Qdrant image version pinning may lack flexibility
**Location**:
`seatunnel-e2e/seatunnel-connector-v2-e2e/connector-qdrant-e2e/src/test/java/org/apache/seatunnel/e2e/connector/v2/qdrant/QdrantIT.java:65`
**Related code**:
```java
// Before modification
private static final String IMAGE = "qdrant/qdrant:latest";
// After modification
private static final String IMAGE = "qdrant/qdrant:v1.15.0";
```
**Issue description**:
Changing the image from `:latest` to pinned version `v1.15.0` is good
practice, but there is no explanation for why v1.15.0 was chosen. Is there a
specific requirement with Testcontainers 1.21.4?
**Potential risks**:
- Risk 1: v1.15.0 may not be the latest stable version, lacking security
patches
- Risk 2: If Qdrant client library (io.qdrant:client:1.11.0) is incompatible
with v1.15.0, test failures will result
- Risk 3: Future manual version updates will be required
**Impact scope**:
- Direct impact: Qdrant E2E tests
- Indirect impact: Qdrant connector compatibility verification
- Impact surface: Single E2E test
**Severity**: MINOR
**Improvement suggestions**:
```java
// Option 1: Add comments explaining why the version was chosen
/**
* Use Qdrant v1.15.0 for testing.
* This version is compatible with Testcontainers 1.21.4 and
io.qdrant:client:1.11.0.
* See https://github.com/qdrant/qdrant/releases/tag/v1.15.0
*/
private static final String IMAGE = "qdrant/qdrant:v1.15.0";
// Option 2: Use configurable version (more flexible)
private static final String IMAGE =
System.getProperty("qdrant.test.image.version", "qdrant/qdrant:v1.15.0");
```
**Rationale**:
Image version selection should have clear justification and be documented.
---
### Issue 7: Testcontainers 1.21.4 and Java 8 compatibility verification is
missing
**Location**: Global impact (CI configuration)
**Related code**:
```yaml
# .github/workflows/backend.yml
matrix:
java: [ '8', '11' ] # CI 测试 Java 8 和 11
```
**Issue description**:
Testcontainers 1.21.4's support for Java 8 is not explained in the PR.
Although the CI configuration shows testing Java 8 and 11, new Testcontainers
versions may have dropped Java 8 support.
**Potential risks**:
- Risk 1: Testcontainers 1.21.4 may require Java 11+, causing Java 8 tests
to fail
- Risk 2: If Java 8 support is dropped, CI configuration and project
documentation should be updated
**Impact scope**:
- Direct impact: All E2E tests in Java 8 environment
- Indirect impact: Users using Java 8
- Impact surface: Core test framework
**Severity**: MAJOR
**Improvement suggestions**:
1. Verify whether Testcontainers 1.21.4 supports Java 8
2. If not supported, update CI configuration to remove Java 8 tests
3. Update project documentation to explain minimum Java version requirements
**Rationale**:
Compatibility is one of the most important considerations in major version
upgrades.
---
### Issue 8: CI configuration changes lack impact assessment on parallel
execution
**Location**: `.github/workflows/backend.yml:702-720`
**Related code**:
```yaml
- name: run seatunnel zeta on k8s test
run: |
./mvnw -B install -DskipTests -am # 新增的预构建步骤
```
**Issue description**:
The newly added `./mvnw -B install -DskipTests -am` prebuild step in K8s
tests will increase execution time by approximately 5-10 minutes. Since CI jobs
may execute in parallel, this may lead to:
- Resource contention (multiple jobs running Maven simultaneously)
- Cache invalidation
- Increased total CI time
**Potential risks**:
- Risk 1: If multiple jobs prebuild simultaneously, CI minute
over-allocation may result
- Risk 2: Maven local cache may be corrupted by concurrent writes
**Impact scope**:
- Direct impact: `k8s-e2e-test` job
- Indirect impact: Overall CI performance
- Impact surface: Single CI job
**Severity**: MINOR
**Improvement suggestions**:
```yaml
# Option 1: Use a dedicated pre-build job
k8s-e2e-prebuild:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- name: Set up JDK
uses: actions/setup-java@v3
with:
java-version: '11'
distribution: 'temurin'
cache: 'maven'
- name: Pre-build artifacts
run: ./mvnw -B install -DskipTests -am
- name: Upload artifacts
uses: actions/upload-artifact@v3
with:
name: k8s-test-artifacts
path: |
seatunnel-shade/seatunnel-hadoop3-3.1.4-uber/target/*.jar
seatunnel-core/seatunnel-starter/target/*.jar
# ... other files that need to be uploaded
k8s-e2e-test:
needs: k8s-e2e-prebuild
# ... subsequent test steps
```
**Rationale**:
Separating prebuild and testing can improve CI efficiency and avoid
duplicate builds.
---
### Issue 9: Docker image import to k3s method may be unstable
**Location**: `.github/workflows/backend.yml:718`
**Related code**:
```yaml
docker save seatunnel:latest | sudo k3s ctr images import -
```
**Issue description**:
Using a pipe to directly import `docker save` output into `k3s ctr` means
that if `k3s ctr` is not properly started or import fails, no error will be
reported but old images will be used.
**Potential risks**:
- Risk 1: Silent failures, difficult to debug
- Risk 2: k3s may use cached old images
**Impact scope**:
- Direct impact: K8s E2E test accuracy
- Indirect impact: K8s connector test reliability
- Impact surface: Single CI job
**Severity**: MINOR
**Improvement suggestions**:
```yaml
- name: run seatunnel zeta on k8s test
run: |
# ... previous steps
echo "Building Docker image..."
docker build -t seatunnel:latest -f $TARGET_PATH/seatunnel_dockerfile
$TARGET_PATH || { echo "Docker build failed"; exit 1; }
echo "Importing image to k3s..."
docker save seatunnel:latest | sudo k3s ctr images import - || { echo
"Failed to import image to k3s"; exit 1; }
echo "Verifying image in k3s..."
sudo k3s ctr images ls | grep seatunnel || { echo "Image not found in
k3s"; exit 1; }
echo "Image import completed successfully"
```
**Rationale**:
Critical steps need verification to avoid silent failures.
---
### Issue 10: Test coverage statement in PR description is too broad
**Location**: PR description
**Related content**:
```markdown
### How was this patch tested?
（留空）
```
**Issue description**:
The "How was this patch tested?" section in the PR description is empty, but
this is a modification to CI infrastructure and should explain:
1. Whether it was tested in a fork repository
2. Whether all E2E tests passed
3. Whether there are local verification steps
**Potential risks**:
- Risk 1: Reviewers cannot assess PR test completeness
- Risk 2: Edge case testing may be missed
**Impact scope**:
- Direct impact: PR review process
- Indirect impact: Post-merge CI stability
- Impact surface: Overall project quality
**Severity**: MINOR
**Improvement suggestions**:
Add to PR description:
```markdown
### How was this patch tested?
1. Validated Testcontainers 1.21.4 compatibility with all E2E modules
2. Verified Docker API 1.44+ support in CI environment
3. All E2E tests passed in fork repository (link to CI run)
4. Local testing confirmed with Docker API version X.X.X
### Known limitations
- Testcontainers 1.21.4 requires Docker API ≥1.44
- Developers using older Docker versions need to upgrade Docker
```
**Rationale**:
Infrastructure modification PRs require more detailed test explanations to
ensure reviewers can assess the completeness of changes.
---
--
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]
