SEPURI-SAI-KRISHNA opened a new pull request, #12568:
URL: https://github.com/apache/seatunnel/pull/12568
## Purpose of this pull request
`multi_table_sink_replica` has no lower bound. The option layer accepts `0`
and negative values, and the job then fails on the worker with an
`IllegalArgumentException` that carries no message at all, naming neither the
option nor the value that caused it.
This adds the missing bound where the option is read, so the same class of
exception is raised at sink construction with both the option key and the
rejected value in the message.
## What the failure looks like today
Measured on `dev`, driving the real public API rather than reasoning about
it. With `multi_table_sink_replica = 0`:
```
BEFORE ctor -> constructed OK, replicaNum accepted
BEFORE createWriter -> java.lang.IllegalArgumentException: null
at
java.util.concurrent.ThreadPoolExecutor.<init>(ThreadPoolExecutor.java:1303)
at java.util.concurrent.Executors.newFixedThreadPool(Executors.java:155)
at
org.apache.seatunnel.api.sink.multitablesink.MultiTableSinkWriter.<init>(MultiTableSinkWriter.java:206)
at
org.apache.seatunnel.api.sink.multitablesink.MultiTableSink.createWriter(MultiTableSink.java:150)
```
The message is literally `null`. An operator who typos this option gets an
exception that mentions no option, no value and no table, thrown from a JDK
class, on a worker rather than at submission.
The chain, every link checked on `dev`:
1. `SinkConnectorCommonOptions.java:30-34` is `.intType().defaultValue(1)`
with no validation.
2. `ReadonlyConfig` parses `0`, `-1` and `-7` back verbatim. Measured.
3. `MultiTableSink.java:91-92` reads it into `replicaNum`.
4. `MultiTableSink.java:130` loops `for (int i = 0; i < replicaNum; i++)`,
so zero writers are created.
5. `MultiTableSink.java:144-147` passes `replicaNum` as the `queueSize`
argument.
6. `MultiTableSinkWriter.java:205-209` calls
`Executors.newFixedThreadPool(queueSize * 2)`, which rejects any size below 1
with a message-less `IllegalArgumentException`.
## What this changes
The check goes in `MultiTableSink`'s constructor, next to where the option
is read. That is the single place this option enters from user config, and both
`createWriter` and `restoreWriter` build their writer through it, so one check
covers both paths.
The exception type is deliberately unchanged. `IllegalArgumentException` is
what this configuration already produces today, so anything catching it keeps
working. What changes is that it now has a message, and that it is raised
before any writer, queue or thread pool is created.
## Backward compatibility
No option is renamed, no default is changed, and no public API or SPI
signature moves. `multi_table_sink_replica` remains `int`, optional, default
`1`.
Nothing that works today starts failing. A value below 1 has never produced
a running job; it has only ever produced the message-less exception above. The
only values used anywhere in this repository are `1`, `2` and `4`, all of which
remain valid, and I confirmed no `.conf`, `.json`, `.yaml` or test in the tree
sets it below 1.
## Tests
Three cases added to the existing `MultiTableSinkWriterTest`, per the
guidance against creating new test classes. That class already builds a
`MultiTableSink` through `MultiTableFactoryContext` and already sets this
option, so the harness was there.
- `testReplicaNumBelowOneIsRejectedWithTheOptionNamed` runs `0` and `-1` and
asserts the message is non-null and contains both the option key and the
rejected value, rather than merely asserting that something was thrown.
- `testReplicaNumOfOneAndAboveStillBuilds` pins `1` and `4` so the bound
cannot later regress into rejecting valid configuration.
- `testReplicaNumDefaultsToOneWhenUnset` pins the default, so the new check
cannot start rejecting jobs that never set the option.
`seatunnel-api` is green: `Tests run: 404, Failures: 0, Errors: 0, Skipped:
0`. `spotless:check` passes and `-DskipTests verify` is `BUILD SUCCESS` on the
module.
## Mutation check
Removing the guard and rerunning the suite fails exactly one test, and fails
it for the right reason:
```
Tests run: 31, Failures: 1, Errors: 0, Skipped: 0
testReplicaNumBelowOneIsRejectedWithTheOptionNamed
org.opentest4j.AssertionFailedError: Expected
java.lang.IllegalArgumentException
to be thrown, but nothing was thrown.
```
"Nothing was thrown" is the point worth reading twice. Without the guard the
sink constructs cleanly and the failure is deferred to writer creation, which
is why the old exception surfaced on a worker instead of at submission.
## Documentation
Deliberately not touching the connector option tables. This change does not
alter the option's name, type, default or required-ness, so every one of those
rows is still accurate, and the three inline examples in `multi-table.md` use
`1`, `2` and `4`, which all remain valid. Editing 93 markdown files to restate
a bound none of them contradict would be a large unrelated diff.
The bound is instead recorded on the option itself, in
`SinkConnectorCommonOptions.withDescription`, which is the single source of
truth and is what surfaces through `optionRule`.
## Relationship to #12290
#12290 migrates `MultiTableSinkWriter` routing to `HashUtils.bucketIndex`.
During review there it was reasonable to read a zero bucket count as
unreachable. This is the config-side path that reaches it, so the two changes
are complementary: #12290 makes the routing arithmetic correct, and this makes
the degenerate writer count impossible to configure in the first place. Neither
depends on the other, and this PR touches no file that #12290 touches.
--
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]
