You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@iotdb.apache.org by "HTHou (via GitHub)" <gi...@apache.org> on 2026/10/23 08:00:44 UTC
[PR] Adapt file-scoped TsFile contexts and resource lifecycles [iotdb]
HTHou opened a new pull request, #18999:
URL: https://github.com/apache/iotdb/pull/18999
## Description
Upgrade TsFile to `2.5.1-261023-SNAPSHOT` and adapt the storage/query integration to its file-scoped encryption contexts. This PR adds reusable integration points only; it does not add a crypto implementation, wallet, key manager, WAL encryption, or a new user configuration option.
### Reader APIs and Resource Ownership
- Use the reader-reported data start offset and pass chunk ordinals/page indexes to page deserialization in query, Pipe, Load, repair, and split paths.
- Retain file-scoped contexts for lazy pages until their last page is consumed, or until query/iterator cleanup. Only page-AEAD chunks enter the query ownership registry; ordinary chunks are not strongly tracked.
- Release cached chunks on eviction, retain returned chunk contexts independently, and handle eviction racing with a cache hit.
- Close partially constructed aligned readers and Pipe iterators on errors and early termination.
### Per-File Writing and Rewrite Policy
- Add the optional `FileEncryptionParameterProvider` SPI. The default uses the existing legacy parameters unchanged. No production provider is bundled. Ambiguous providers and a custom provider returning null fail explicitly.
- Create fresh file parameters through shared writer factories and propagate the writer's context to memtable/chunk writers, including target-file switches during cross compaction.
- Provide a rewrite-policy hook for compaction and Load. Policies requiring new file contexts use the existing read-point/tablet write pipelines instead of copying source ciphertext or staging plain chunks. Source deletion remains the Load scheduler's responsibility.
- Preserve existing plain/legacy behavior when no custom policy is installed. The test provider is framing-only and lives entirely in test sources.
### Verification
- Targeted DataNode tests: **110 tests, 0 failures, 0 errors, 3 skipped**. Covers file-context lifecycle, aligned/non-aligned lazy readers, cache eviction, plain files, memtables/flush, Load/splitting, Pipe, and representative read-point/read-chunk/fast compaction cases.
- Full-reactor English and Chinese test-source compilation, including the integration-test module.
- Spotless and `git diff --check`.
No running-cluster integration test was executed for this PR.
<hr>
This PR has:
- [x] been self-reviewed, including cache eviction/read ownership.
- [x] added Javadocs for the new provider contract and resource ownership.
- [x] updated the TsFile dependency version.
- [x] added unit tests and exercised existing regression tests.
- [ ] been tested in a test IoTDB cluster.
##### Key Changed/Added Classes
`FileEncryptionParameterProvider`, `EncryptDBUtils`, `QueryChunkReaders`, `ChunkCache`, `DiskChunkLoader`, `DiskAlignedChunkLoader`, `CompactionTsFileWriter`, `AbstractCompactionWriter`, `TabletRewriteLoadStrategy`, and TsFile-based Pipe/Load readers.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adapt file-scoped TsFile contexts and resource lifecycles [iotdb]
Posted by "shuwenwei (via GitHub)" <gi...@apache.org>.
shuwenwei commented on PR #18999:
URL: https://github.com/apache/iotdb/pull/18999#issuecomment-6406549384
### `DiskAlignedChunkLoader` may double-close chunks on failure
At `DiskAlignedChunkLoader.getChunkReader()` (around L127-L147), the catch block closes `timeChunk` and every chunk in `valueChunkList`. However, in TsFile 2.5.1:
- `AlignedChunkReader` / `TableChunkReader` already closes its chunks if `initAllPageReaders()` fails.
- `QueryChunkReaders.retain()` also closes the reader before throwing when the query resource registry is already closed.
Therefore failures in either `new AlignedChunkReader(...)` / `new TableChunkReader(...)` or `QueryChunkReaders.retain(...)` can execute `Chunk.close()` twice. `Chunk.close()` releases a page-AEAD `EncryptParameter` reference and is not idempotent, so this can over-release the file-level context and destroy the provider context too early.
Suggested fix: track whether ownership has been transferred to the `IChunkReader`/`QueryChunkReaders`; only close the raw chunks when the reader was not successfully constructed or ownership was not transferred. Otherwise let `IChunkReader.close()` perform the cleanup once.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adapt file-scoped TsFile contexts and resource lifecycles [iotdb]
Posted by "HTHou (via GitHub)" <gi...@apache.org>.
HTHou commented on PR #18999:
URL: https://github.com/apache/iotdb/pull/18999#issuecomment-6410285114
@shuwenwei Thanks for checking the failure paths. Regarding the [double-close comment](https://github.com/apache/iotdb/pull/18999#issuecomment-6406549384), I checked both the sources and bytecode of the exact dependency used here, `org.apache.tsfile:tsfile:2.5.1-261023-SNAPSHOT`.
`Chunk.close()` delegates to `EncryptParameter.close()`, which calls `destroy()`. That method guards the release as follows (`EncryptParameter.java:302–319`):
```java
synchronized (this) {
if (destroyed) {
return;
}
destroyed = true;
// ...
}
if (fileEncryptContext != null) {
fileEncryptContext.release();
}
```
Also, `retain()` returns a distinct `EncryptParameter` instance sharing the underlying file context. Each retained instance has its own `destroyed` flag.
So the constructor/registry cleanup and the loader's catch block can indeed both close the same chunk, but the second close does not decrement the shared reference count again. In this dependency version, repeated close is redundant cleanup; it does not over-release the context or invalidate other retained references.
This addresses the double-close/reference-count claim specifically. Reusing an already-closed object during a retry is a separate ownership question.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adapt file-scoped TsFile contexts and resource lifecycles [iotdb]
Posted by "HTHou (via GitHub)" <gi...@apache.org>.
HTHou merged PR #18999:
URL: https://github.com/apache/iotdb/pull/18999
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adapt file-scoped TsFile contexts and resource lifecycles [iotdb]
Posted by "shuwenwei (via GitHub)" <gi...@apache.org>.
shuwenwei commented on PR #18999:
URL: https://github.com/apache/iotdb/pull/18999#issuecomment-6406569845
### Pipe aligned-chunk retry can reuse chunks already closed by a failed reader constructor
In `TsFileInsertionEventScanParser.useNextPendingAlignedChunk()` (around L915-L943), the pending group is removed only after `AlignedChunkReader` / `AlignedSinglePageWholeChunkReader` is successfully constructed. If construction throws:
- `AlignedChunkReader` closes the passed time/value chunks internally.
- `AlignedSinglePageWholeChunkReader` explicitly calls `close()` in its constructor catch, closing the same value chunks.
The pending group therefore still references value chunks whose page-AEAD references have already been released. If the exception is a classified memory-pressure exception, the code marks `shouldRetryChunkHeaderAfterMemoryPressure` and retries with that same closed group; otherwise later `clearCachedAlignedChunkData()` closes them again. This can both make the retry invalid and over-release the file crypto context.
Suggested fix: do not leave the original borrowed chunk list in `pendingAlignedChunkGroups` after a reader constructor has taken ownership and failed. Either construct with independently retained chunks and only release that attempt's references on failure, or remove/reload the pending group before retry so it never reuses closed chunks or closes them a second time.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org