github-actions[bot] commented on code in PR #66612:
URL: https://github.com/apache/doris/pull/66612#discussion_r3849552378
##########
fe/be-java-extensions/paimon-connector/src/main/java/org/apache/doris/paimon/PaimonJniWriter.java:
##########
@@ -576,13 +583,31 @@ private void closeWriter() throws Exception {
throw new IllegalStateException(
"A previous Paimon SDK close failed; native memory cannot
be released safely");
}
- Exception failure = closeResource(writer, null);
- failure = closeResource(globalIndexAssigner, failure);
- failure = closeResource(ioManager, failure);
+ Exception lifecycleFailure = closeResource(writer, null);
+ Exception compactionFailure = closeCompactionExecutor();
+ if (compactionFailure != null) {
+ lifecycleFailure = appendFailure(lifecycleFailure,
compactionFailure);
+ // The task may still reference Doris-backed memory and spill
files. Leave all dependent
+ // Java resources reachable and open; the native backend will
retain their handles.
+ sdkCloseFailed = true;
+ throw lifecycleFailure;
+ }
+ lifecycleFailure = closeResource(globalIndexAssigner,
lifecycleFailure);
+ Exception cleanupFailure = closeResource(ioManager, null);
clearWriterState();
- if (failure != null) {
+ if (lifecycleFailure != null) {
+ if (cleanupFailure != null) {
+ lifecycleFailure.addSuppressed(cleanupFailure);
+ }
sdkCloseFailed = true;
- throw failure;
+ throw lifecycleFailure;
+ }
+ if (cleanupFailure != null) {
Review Comment:
[P1] Keep residual spill files charged after cleanup failure
`DorisIOManager.close()` deliberately leaves any still-existing channel
charged when delegate deletion fails, but this branch logs that failure and
returns Java close success. Native close then destroys the spill session, whose
destructor subtracts all accounted bytes and drops the query-directory lease
even though the files remain until later QueryContext teardown/GC. Other
writers can therefore reserve that occupied space and overcommit the disk.
Please retain or transfer the accounting and lease to the pending cleanup
record until deletion succeeds, and extend the failure test with a real
residual channel plus native quota assertions.
##########
regression-test/suites/external_table_p0/paimon/write/test_paimon_write_external_paths.groovy:
##########
@@ -134,8 +134,14 @@ suite("test_paimon_write_external_paths",
"p0,external,paimon") {
FROM numbers("number" = "16")
"""
def oldRoundFiles = dataFiles("t_round_robin")
- assertTrue(oldRoundFiles.any { it.startsWith("${pathRoot}/round-a/") })
- assertTrue(oldRoundFiles.any { it.startsWith("${pathRoot}/round-b/") })
+ assertFalse(oldRoundFiles.isEmpty())
+ // Paimon randomizes the initial position of each round-robin
provider. Since providers are
+ // scoped to a partition/bucket writer, independent writes are not
guaranteed to hit every
+ // configured path. Verify the stable contract that all files use the
configured path set.
+ assertTrue(oldRoundFiles.every {
Review Comment:
[P2] Keep a deterministic round-robin oracle
This now passes if the provider always chooses `round-a`, so it no longer
tests the configured round-robin strategy. Paimon randomizes only the initial
provider position and then advances deterministically, while the current 16-row
bulk does not force a second file because rolling is checked every 1000
records. Removing the cross-writer probabilistic assertion is reasonable, but
please add one deterministic writer/bucket with enough non-compressible rows to
roll multiple files and assert that it uses both roots.
##########
fe/be-java-extensions/paimon-connector/src/main/java/org/apache/doris/paimon/PaimonJniWriter.java:
##########
@@ -228,6 +232,10 @@ public void writeArrow(long arrayAddress, long
schemaAddress) throws Exception {
VectorSchemaRoot root = Data.importVectorSchemaRoot(
allocator, array, schema, dictionaries)) {
writeBatch(root);
+ // Some Paimon writers (lookup stores, global indexes and
clustering indexes)
+ // write raw files below IOManager paths. Account their
observed growth before
+ // returning control to the native pipeline.
+ ioManager.reconcile();
Review Comment:
[P1] Avoid recursively scanning every spill file after each block
This runs once per non-empty Doris `Block`, and the native callback holds
the session mutex while recursively walking every managed root and calling
`file_size` on every file, including buffer channels already covered by
callbacks. After spill starts, a writer with B blocks and F accumulated files
pays O(B x F) synchronous metadata work and blocks reserve/release callbacks
behind each scan; this can make large writes increasingly expensive. Please use
incremental raw-file accounting, or at least dirty/throttled and raw-only
reconciliation with a bounded final pass, and add a many-block/many-file
regression.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
