LuciferYang opened a new pull request, #9583:
URL: https://github.com/apache/paimon/pull/9583
### Purpose
close #9581
`OrcWriterFactory` cached `OrcFile.WriterOptions` in a field, and `create()`
writes per-file state into whatever it gets back:
```java
opts.compress(CompressionKind.valueOf(compression.toUpperCase()));
opts.physicalWriter(new PhysicalFsWriter(new FSDataOutputStream(out, null)
{...}, opts, ...));
```
`WriterImpl` reads the physical writer out of the options in its
constructor, so two `create()` calls on one factory racing between those two
statements can both end up with the second call's writer, i.e. two writers
appending to one output stream. This builds the options per call instead.
No factory is used concurrently today, so it is not reachable: each rolling
writer builds its own `FormatWriterFactory`, and `RollingFileWriterImpl` rolls
files strictly one at a time, closing the current writer before opening the
next. What makes it worth removing is that factory sharing in this shape is
what FLINK-27070 was about, and that was fixed by making the call sites build a
writer factory per writer rather than by changing this class, so the convention
it established is written down nowhere. `FileFormat` does carry "NOTE: This
class must be thread safe", added by that same commit, while
`FormatWriterFactory` says nothing about it, and ORC was the only
implementation of it that kept per-file state in a field.
To be clear about the scope: this does not make the factory safe to share.
The `Vectorizer`'s `TypeDescription` is still common to every writer a factory
creates. It removes one trap, it does not establish thread safety.
The cost is one Hadoop `Configuration` and one `WriterOptions` per file
instead of per factory, which is noise next to writing the file, and
`createWithShreddingWritePlan` already built a `Configuration` per call. No
memory manager is created per file: `OrcFile.WriterOptions` takes the JVM-wide
static one, which is why `testNotOverrideInMemoryManager` still means what it
did.
### Tests
`OrcWriterFactoryTest.testWriterOptionsNotSharedBetweenCalls` asserts two
calls return different instances. It fails against the current code with
`Expected not same: org.apache.orc.OrcFile$WriterOptions@...`.
That pins the mechanism rather than the outcome, so a different fix that
kept the field and copied per `create()` would fail it. I kept it because it is
one line and reaches through the same `@VisibleForTesting` seam the
neighbouring test already uses; a behavioural version would open two writers
from one factory, write to each and read both files back, which is worth adding
if reviewers prefer it.
`mvn -pl paimon-format test` on JDK 8: 597 tests, 0 failures.
`spotless:check` and `checkstyle:check` are clean.
--
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]
