Pengzna commented on code in PR #17335:
URL: https://github.com/apache/iotdb/pull/17335#discussion_r2971694114
##########
iotdb-core/datanode/src/test/java/org/apache/iotdb/db/pipe/event/PipeTsFileInsertionEventTest.java:
##########
@@ -157,6 +159,82 @@ public void testAuthCheck() throws Exception {
}
}
+ @Test
+ public void testLateCreatedModFileCanStillBeObservedAfterShallowCopy()
throws Exception {
+ final File baseDir = new File(TestConstant.BASE_OUTPUT_PATH, "late-mod");
+ final File tsFile = new File(baseDir, "late-mod.tsfile");
+ PipeTsFileInsertionEvent originalEvent = null;
+ PipeTsFileInsertionEvent copiedEvent = null;
+ try {
+ Assert.assertTrue(baseDir.mkdirs() || baseDir.exists());
+ Assert.assertTrue(tsFile.createNewFile() || tsFile.exists());
+
+ final TsFileResource resource = new TsFileResource(tsFile);
+ resource.setStatus(TsFileResourceStatus.NORMAL);
+
+ originalEvent =
+ new PipeTsFileInsertionEvent(
+ false,
+ "root.db",
+ resource,
+ null,
+ true,
+ false,
+ false,
+ null,
+ "testPipe",
+ 1L,
+ null,
+ buildUnionPattern(true, Collections.singletonList(new
IoTDBTreePattern(true, null))),
+ new TablePattern(false, null, null),
+ null,
+ null,
+ null,
+ true,
+ Long.MIN_VALUE,
+ Long.MAX_VALUE);
+ copiedEvent =
+ originalEvent.shallowCopySelfAndBindPipeTaskMetaForProgressReport(
+ "testPipeCopy",
+ 2L,
+ null,
+ buildUnionPattern(true, Collections.singletonList(new
IoTDBTreePattern(true, null))),
+ new TablePattern(false, null, null),
+ null,
+ null,
+ null,
+ true,
+ Long.MIN_VALUE,
+ Long.MAX_VALUE);
+
+ Assert.assertFalse(originalEvent.isWithMod());
+ Assert.assertFalse(copiedEvent.isWithMod());
+
+ resource
+ .getExclusiveModFile()
+ .write(new TreeDeletionEntry(new MeasurementPath("root.db.d1.s1"),
0, 1));
+ final File modFile = resource.getExclusiveModFile().getFile();
+ Assert.assertTrue(modFile.exists());
+
+ Assert.assertTrue(originalEvent.isWithMod());
+ Assert.assertEquals(modFile, originalEvent.getModFile());
+ Assert.assertTrue(copiedEvent.isWithMod());
+ Assert.assertEquals(modFile, copiedEvent.getModFile());
+
Review Comment:
Good point. I added a dedicated regression test
`testPinnedModFilePathIsStableAfterIncreaseReferenceCount` to cover the typical
transfer path:
- create the mod after event construction
- call `increaseReferenceCount()` to pin/copy it into the pipe dir
- assert the returned `modFile` path is stable across repeated
`getModFile()/isWithMod()` calls (and differs from the original path)
This should catch regressions where the pinned mod path gets overwritten or
ref counting becomes asymmetric.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/pipe/sink/protocol/thrift/async/IoTDBDataRegionAsyncSink.java:
##########
@@ -404,6 +404,12 @@ private boolean transferWithoutCheck(final
TsFileInsertionEvent tsFileInsertionE
throw new
FileNotFoundException(pipeTsFileInsertionEvent.getTsFile().getAbsolutePath());
}
+ final boolean supportMod =
clientManager.supportModsIfIsDataNodeReceiver();
+ final File modFile =
+ (supportMod && pipeTsFileInsertionEvent.isWithMod())
+ ? pipeTsFileInsertionEvent.getModFile()
+ : null;
Review Comment:
Agreed — updated this sink to take a single snapshot via `getModFile()`
(when `supportMod` is true) and then decide by `modFile != null`.
This removes the `isWithMod()` gate/TOCTOU window and ensures late-created
mods can still be observed. Also,
`PipeTsFileInsertionEvent.refreshModFileState()` now keeps pinned paths stable
while still lazily pinning a mod file created after the event is already pinned.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/pipe/event/common/tsfile/PipeTsFileInsertionEvent.java:
##########
@@ -859,6 +865,17 @@ public PipeEventResource eventResourceBuilder() {
this.eventParser);
}
+ private void refreshModFileState() {
+ if (!shouldTransferModFile || Objects.isNull(resource)) {
+ isWithMod = false;
+ modFile = null;
+ return;
+ }
+
+ isWithMod = resource.anyModFileExists();
+ modFile = isWithMod ? resource.getExclusiveModFile().getFile() : null;
Review Comment:
Thanks! Agreed. Followed this direction: once the event is pinned we keep
`modFile` as the copied/pinned file (no overwrite back to
`resource.getExclusiveModFile()`), and if the mod is created after pinning we
now lazily pin/copy it and then keep that pinned path stable. Added a
regression test to ensure pinned mod path stability after
`increaseReferenceCount()`.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/pipe/event/common/tsfile/PipeTsFileInsertionEvent.java:
##########
@@ -859,6 +865,17 @@ public PipeEventResource eventResourceBuilder() {
this.eventParser);
}
+ private void refreshModFileState() {
+ if (!shouldTransferModFile || Objects.isNull(resource)) {
+ isWithMod = false;
+ modFile = null;
+ return;
+ }
+
+ isWithMod = resource.anyModFileExists();
+ modFile = isWithMod ? resource.getExclusiveModFile().getFile() : null;
Review Comment:
Update: I adjusted the implementation slightly. Instead of completely
skipping refresh after pinning, we now avoid overwriting an already-pinned
`modFile` path, but still support the case where the mod file is created
*after* pinning by lazily pinning/copying it (and then keeping that pinned path
stable). This keeps ref tracking correct while covering the late-mod edge case.
--
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]
