chovy-3012 commented on PR #12390:
URL: https://github.com/apache/seatunnel/pull/12390#issuecomment-5869175116
> Thank you for the quick and careful update, @chovy-3012! I re-read the
whole change at the new head `036227a` and traced the runtime paths again. The
catalog-path gap from my first review is properly closed, the validation and
the tests are stronger than before, and I verified your "all five issues
addressed" note item by item against the code (table below). I found no
blocking problems in this version; only two small, optional nits remain.
>
> # What Problem Does This PR Solve?
> **User pain point:** The MaxCompute connector relied on the ODPS SDK's
built-in HTTP timeout/retry defaults. On flaky networks, or with very wide
schemas / large partitions, users could not tune them from the job config.
>
> **Fix approach:** Six optional options in `MaxcomputeBaseOptions`:
`connect_timeout_ms`, `read_timeout_ms`, `retry_times` (ODPS REST client,
control plane) and `tunnel_connect_timeout_ms`, `tunnel_read_timeout_ms`,
`tunnel_retry_times` (Tunnel client, data plane). Defaults equal the SDK 0.51.2
defaults, so omitting them changes nothing.
>
> **One-sentence summary:** Expose the SDK's two HTTP clients'
timeouts/retries as job options, applied on every place the connector builds an
`Odps` or a `TableTunnel`.
>
> Before / after example:
>
> ```hocon
> # before: no way to tune, SDK defaults only
> sink { Maxcompute { endpoint = "..." project = "p" table_name = "t" } }
>
> # after: slow metadata service + big upsert batches
> sink { Maxcompute { endpoint = "..." project = "p" table_name = "t"
> read_timeout_ms = 300000 retry_times = 6 tunnel_read_timeout_ms =
600000 } }
> ```
>
> # 1. Code Change Review
> ## 1.1 Core Logic Analysis
> **Status of my previous review (commit `09edf5d`) at the new head:**
>
> Prior issue Status Evidence at `036227a`
> 1 (High): REST options ignored on the catalog path FIXED
`MaxcomputeUtil.applyRestClientOptions(Odps, ReadonlyConfig)` is now a shared
public helper (`util/MaxcomputeUtil.java:118-131`), called from
`MaxcomputeUtil.getOdps` (`:107`) and from `MaxComputeCatalog.getOdps(String,
String)` (`catalog/MaxComputeCatalog.java:343`).
`MaxComputeCatalogFactory.optionRule()` now lists the three REST options
(`catalog/MaxComputeCatalogFactory.java:57-59`).
> 2 (Medium): silent clamp / no validation FIXED `toTimeoutSeconds`
rejects `ms < 1000` with a clear message (`MaxcomputeUtil.java:139-145`),
`toRetryTimes` rejects negatives (`:148`), both used by REST and Tunnel paths;
option descriptions and docs (en/zh, source/sink) state "converted to whole
seconds; minimum 1000".
> 3 (Medium): E2E / catalog not covered FIXED (see nit in Issue 2)
New `MaxComputeCatalogTest` (2 tests, real `Odps` built by the catalog); the
six options were added to the sink block of `maxcompute_to_maxcompute.conf`
(`:75-83`).
> 4 (Low): hard-coded SDK defaults FIXED (option B)
`MaxcomputeUtilTest.testOptionDefaultsMatchSdkDefaults` (`:362`) compares every
option default with `RestClient.DEFAULT_*` /
`GeneralConfiguration.DEFAULT_SOCKET_*`, so an SDK bump fails loudly.
> 5 (Low): FQCN in tests FIXED `Odps` / `TableTunnel` are imported; no
`com.aliyun...` in test bodies.
> **Runtime path re-verified (normal user path reaches the change):**
>
> ```
> Source, no user schema: MaxcomputeSource -> new
MaxComputeCatalog(readonlyConfig)
> -> catalog.getTable/listTables -> MaxComputeCatalog.getOdps(project,
schema)
> -> MaxcomputeUtil.applyRestClientOptions(odps, readonlyConfig)
[REST options]
> Source reader / split enumerator, Sink output format:
> MaxcomputeUtil.getDownloadSession / getTableTunnel
> -> getOdps(readonlyConfig) [REST options] +
tableTunnel.getConfig().setSocket* [Tunnel options]
> Sink save mode DDL: MaxcomputeSink.getSaveModeHandler ->
catalogFactory.createCatalog(id, readonlyConfig)
> -> createTable/dropTable/createPartition ->
MaxComputeCatalog.getOdps(...) [REST options]
> ```
>
> I grepped the connector: `new Odps(` now exists only in
`MaxcomputeUtil.getOdps` and `MaxComputeCatalog.getOdps`, and both apply the
helper, so there is no remaining path that builds an unconfigured `Odps`. The
sink hands the same `readonlyConfig` to the catalog, so user-set values reach
it.
>
> **Key findings**
>
> 1. The catalog path (source schema discovery, sink
`createTable`/`dropTable`/partition DDL) now honors `connect_timeout_ms` /
`read_timeout_ms` / `retry_times`, which makes the documented behavior true.
> 2. The tunnel client keeps its own independent options; `getTableTunnel`
builds the tunnel over the REST-configured `Odps`, and the independence test
asserts REST-only and Tunnel-only options do not leak into each other.
> 3. Fail-fast validation replaced the silent clamp; the old "clamp" test
became `testSubSecondTimeoutIsRejected`, which is a stronger assertion.
> 4. `getOdps(String, String)` was widened from private to package-private
with `@VisibleForTesting` (shaded guava), a minimal and reasonable testability
change.
>
> ## 1.2 Compatibility Impact
> **Fully compatible.** All options are optional and default to the SDK
0.51.2 values (`maxcompute.version` in `connector-maxcompute/pom.xml:33`), now
guarded by a unit test. No existing option is renamed or removed, and no
serialization or checkpoint state is touched, so `incompatible-changes.md`
needs no entry. The new validation only applies to the new options, so no
existing job can be rejected.
>
> ## 1.3 Performance / Side-Effect Analysis
> Setters run once per `Odps` / `TableTunnel` construction, no new threads,
locks or buffers. Larger read timeouts / retry counts only lengthen how long a
stuck request can block, which is the intended and documented trade-off.
>
> ## 1.4 Error Handling and Logging
> No exception is swallowed and nothing sensitive is logged. Invalid values
throw `IllegalArgumentException` naming the option and the offending value. Two
optional nits:
>
> **Issue 1: Upper-bound overflow produces an unhelpful message (and
validation happens late)**
>
> * **Location:**
`seatunnel-connectors-v2/connector-maxcompute/src/main/java/org/apache/seatunnel/connectors/seatunnel/maxcompute/util/MaxcomputeUtil.java:139-145`
> * **Problem:** `Math.toIntExact(ms / 1000)` throws a bare
`ArithmeticException: integer overflow` for values above roughly 2.1e12 ms,
without the option name, unlike the lower-bound check. Also, validation runs
when the first `Odps`/`TableTunnel` is built (on the source/sink side or at
reader/writer creation), not during `OptionRule` validation, so a bad value is
reported a little later than ideal.
> * **Potential risk:** Cosmetic; a confusing error for an absurd value. No
data or correctness impact.
> * **Best improvement:** Either fold the upper bound into the
`checkArgument` (for example `ms <= Integer.MAX_VALUE * 1000L`) so the message
names the option, or leave as is. Non-blocking.
> * **Severity:** Low
> * **Raised by another reviewer:** No
>
> **Issue 2: The E2E conf only proves option acceptance, and only on the
sink side**
>
> * **Location:**
`seatunnel-e2e/seatunnel-connector-v2-e2e/connector-maxcompute-e2e/src/test/resources/maxcompute_to_maxcompute.conf:75-83`
> * **Problem:** The six values equal the defaults and this conf reads from
a `FakeSource`, so the Maxcompute source-side `optionRule()` registration is
covered by the unit test only, and the E2E cannot show that a non-default value
is applied. The unit tests (`MaxComputeCatalogTest`, `MaxcomputeUtilTest`) do
cover the effect, so this is a small gap.
> * **Best improvement:** Optionally use a non-default but safe value in the
sink block (for example `read_timeout_ms = 121000`) and, if an existing conf
reads from the Maxcompute source, add the options there too. Non-blocking.
> * **Severity:** Low
> * **Raised by another reviewer:** No
>
> **Issue 3: PR title is truncated and the description template sections are
empty**
>
> * **Location:** PR metadata
> * **Problem:** The PR title currently ends with "retry op..." (the commit
title is complete), and "Does this PR introduce any user-facing change?" / "How
was this patch tested?" are still the template placeholders. With squash merge
the truncated title becomes the commit message.
> * **Best improvement:** Fix the title to "[Feature][Connector-V2] Expose
MaxCompute client timeout and retry options" and fill the two sections (new
optional options, defaults unchanged; unit tests plus `MaxComputeIT`).
> * **Severity:** Low
> * **Raised by another reviewer:** No
>
> # 2. Code Quality Assessment
> ## 2.1 Coding Standards
> Good. The shared helper has a clear Javadoc explaining why it exists (two
`Odps` factories) and why ms is divided by 1000; the option descriptions carry
the same constraint. No wildcard imports, no FQCN in code bodies, spotless
(Code style job) passed, license header job passed, and the new test class
`MaxComputeCatalogTest` has the ASF header.
>
> ## 2.2 Test Coverage and Test Stability
> Coverage now exercises the critical paths: per-option application for REST
and Tunnel, default fallback, REST/Tunnel independence, sub-second and negative
rejection, SDK-default consistency, factory option registration (source and
sink), and the catalog `Odps` (the path that was missing last time).
`MaxComputeIT` still passes with the extended conf.
>
> **Stability rating: Stable.** Evidence: `MaxcomputeUtilTest.java`
(validation and wiring tests) and `catalog/MaxComputeCatalogTest.java` only
build `Odps` / `TableTunnel` objects in memory (no network, sleeps, fixed
ports, temp files or shared static state) and assert exact integers. Fork CI on
this head: `MaxComputeCatalogTest` 2/2, `MaxcomputeSourceFactoryTest` 2/2,
`MaxcomputeUtilTest` 21/21, connector module 79 tests, 0 failures;
`MaxComputeIT` 30 run, 0 failures, 1 skipped in
`updated-modules-integration-test-part-1 (8, ubuntu-latest)`.
>
> ## 2.3 Documentation Updates
> `docs/en` and `docs/zh` are updated for source and sink: option tables,
per-option sections and the "Client timeout & retry" note (including the
1-second granularity and the minimum of 1000), all consistent with the code
names and defaults. The claim that the REST options cover table/schema lookup
and catalog calls is now accurate.
>
> # 3. Architectural Soundness
> ## 3.1 Elegance of the Solution
> Precise fix: one shared helper for the REST client, one place for the
Tunnel client, options modelled exactly like the SDK's two HTTP clients.
>
> ## 3.2 Maintainability
> Good. Two `Odps` factories remain, but both delegate to the same helper,
so they cannot drift.
>
> ## 3.3 Extensibility
> Good. The options live in `MaxcomputeBaseOptions` and are inherited by
source/sink options, so future connection-level knobs can follow the same
pattern.
>
> ## 3.4 Historical-Version Compatibility
> Compatible with existing jobs and configs; no upgrade action is required.
>
> # 4. Issue Summary
> No blocking issues found. Remaining non-blocking items:
>
> # Issue Location Severity
> 1 Overflow on huge millisecond values gives a bare ArithmeticException;
validation happens at first client build `util/MaxcomputeUtil.java:139-145`
Low
> 2 E2E conf uses default values and only exercises the sink
`maxcompute_to_maxcompute.conf:75-83` Low
> 3 PR title truncated; description template sections empty PR metadata
Low
> **CI status (head `036227a`):** the apache-side `Build` check is a pointer
to the fork run. In the fork run exactly one job failed: `Run / unit-test (8,
windows-latest)`, in `seatunnel-engine-server`:
`TaskExecutionServiceTest.testStaleTaskDoneCleansOnlyOwnedGenerationResources:639`
(`WantedButNotInvoked: scheduledFuture.cancel(false)`). This PR does not touch
`seatunnel-engine`, the test came in with #12238, and the same class passed on
ubuntu 8/11 and windows 11 in this very run, so it is not caused by this diff.
It looks like the known fixture-identity flake that open PR #12394 targets
(that PR cites this exact test); that is my inference from its description, I
have not verified it end to end. MaxCompute unit tests and `MaxComputeIT`
passed, and code style, license header, docs build and all
`updated-modules-integration-test` jobs are green. Suggested action: re-run
only the failed windows job; no branch sync is needed for this (#12394 is not
merged yet).
>
> # 5. Merge Recommendation
> ### Conclusion: Ready to merge
> 1. **Blockers - must be fixed:** none.
> 2. **Recommended fixes - non-blocking:** the three Low items above
(overflow message, optionally non-default values in the E2E conf, PR
title/description).
>
> **Overall assessment:** All five points from the previous round are
resolved, including the High one: the REST options now reach both `Odps`
factories, backed by a shared helper, a catalog unit test, and an extended E2E
conf. Defaults are verified against the SDK, both languages of docs match the
code, and I have no better alternative to suggest. There is no other reviewer's
approve/request-changes on this PR to respond to. The only gate left is
re-running the unrelated windows engine unit-test job. Thanks again for the
responsive and thorough follow-up!
Thanks for the re-review and the item-by-item verification! The two code
nits are fixed in the latest push; the PR metadata is updated as well.
--
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]
