loustler commented on PR #11661:
URL: https://github.com/apache/seatunnel/pull/11661#issuecomment-5191004549
Thanks — this is a careful review, and Issue 1 is a real problem that I
inherited rather than noticed. Fixed in f80ec5eef.
## Issue 1 — fixed with Option A, and it is worse than "wrong extra options"
I went with **Option A** (use the parameter consistently). Two reasons.
First, `getConfiguration(HadoopConf)` is an interface method that takes its
conf as an argument, so "configure from the argument" is the only contract that
doesn't surprise a caller. Option B would have meant deleting the parameter
from `buildConfiguration` and narrowing the guard to "reference changed →
rebuild from the field anyway", which keeps the method's signature promising
something it doesn't do.
Second — and this is the part I had not appreciated until I chased your
comment down — the blast radius of the asymmetry is bigger than the extra
options. `HadoopConf#setExtraOptionsForConfiguration` doesn't only copy
`extraOptions` in; it also decides which keys an `hdfs-site.xml` resource may
**not** overwrite:
```java
public void setExtraOptionsForConfiguration(Configuration configuration) {
if (!extraOptions.isEmpty()) {
removeUnwantedOverwritingProps(extraOptions); // keys from
getFsDefaultNameKey/getHdfsImplKey/...
extraOptions.forEach(configuration::set);
}
if (StringUtils.isNotBlank(hdfsSitePath)) {
Configuration hdfsSiteConfiguration = new Configuration();
hdfsSiteConfiguration.addResource(new Path(hdfsSitePath));
unsetUnwantedOverwritingProps(hdfsSiteConfiguration); // same keys
configuration.addResource(hdfsSiteConfiguration);
}
}
```
`getHdfsImplKey()` / `getHdfsImplDisableCacheKey()` are
`String.format("fs.%s.impl", getSchema())`, and **`getSchema()` is overridden
by every filesystem subclass** — `FtpConf`, `S3HadoopConf`, `CosConf`,
`SftpConf`, `ObsConf`, `OssHadoopConf`, jindo `OssConf`, `LocalFileHadoopConf`.
So with two different confs in play, `unsetUnwantedOverwritingProps` would
unset the protected keys **for the wrong scheme**, leaving the `hdfs-site.xml`
resource free to overwrite the foreign conf's own `fs.defaultFS` and
`fs.<its-scheme>.impl` — precisely the overwrite that method exists to prevent.
That moved this from "a nit on a dead branch" to something I wanted fixed
regardless of reachability.
**Behaviour on every path reachable today is unchanged**, since all three
call sites pass the strategy's own field, so parameter and field are the same
object.
The `buildConfiguration` Javadoc you asked for is there too, and it records
the scheme-derivation reason rather than just saying "this is the expensive
one".
## Coverage gap — taken, and it caught something
You were right that the existing tests prove the returned `Configuration` is
correct but would pass just as well if no caching happened at all. Added
`testTheExpensiveBuildHappensOncePerHadoopConf`, which counts
`toConfiguration()` invocations through a test-local `HadoopConf` subclass: 5
further `getConfiguration` calls must not increase the count, and a foreign
conf must be built exactly once from itself.
Writing it turned up a detail worth stating explicitly, because my first
version of the test asserted the wrong thing and failed:
> **`HadoopFileSystemProxy`'s constructor eagerly builds its own
`Configuration` from the same `HadoopConf`** (`initialize()` →
`hadoopConf.toConfiguration()`), so **two** builds happen during `init`, not
one.
That second one is per **writer**, not per output file, and the proxy
already memoizes it in its own `private transient Configuration` — so it is not
part of the defect this PR fixes and I have not touched it. But it does mean an
absolute assertion after `init` would be pinning `init`'s internal structure
rather than this cache's contract, so the test asserts `>= 1` after init and
then exact equality across the following calls. The comment in the test says so.
Also added `testForeignHadoopConfGetsItsOwnExtraOptions` so the Issue 1 fix
is covered rather than just asserted in a commit message.
`connector-file-base` on JDK 8: **242 tests, 0 failures**, `spotless:check`
clean.
## On the two things you flagged but did not ask me to change
- **The unexplained ~2.5× isolated-vs-in-situ gap** stays unexplained. I
would rather leave it in the description as an open observation than quietly
drop it; every number quoted in the PR is the smaller isolated one, so the
claim does not depend on it.
- **The reference-equality guard is dead code today** — agreed, and I have
kept it rather than removing it, on the grounds that a public method silently
ignoring its own argument is the worse failure mode. With Option A applied it
is now also *correct* dead code, which it wasn't before.
--
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]
