Skip to content

Scope the TsFile scan of IoTDBLoadStorageTsFileWithCompactionIT to its own database - #19169

Open
luoluoyuyu wants to merge 1 commit into
apache:masterfrom
luoluoyuyu:fix-it-tsfile-scan-scope
Open

luoluoyuyu wants to merge 1 commit into
apache:masterfrom
luoluoyuyu:fix-it-tsfile-scan-scope

Conversation

@luoluoyuyu

Copy link
Copy Markdown
Member

Problem

IoTDBLoadStorageTsFileWithCompactionIT (added in #19100) walks the whole DataNode data
directory
and requires every TsFile it finds to carry the table schema of table t1. The
TsFile counting helpers used by the compaction assertions are built on the same scan.

The DataNode data directory is shared by every database of the node, including the internal
databases that store system records. Those are tree-model databases whose TsFiles legitimately
carry an empty table schema map, so as soon as the node holds one of them the test fails with

TsFile has no schema entry for table t1: {}

This reproduces on a downstream distribution that records the password history of root in
root.__audit on first login: the audit TsFile is flushed next to the test database and is
picked up by the scan. The failure message did not name the file, so it looked like a schema
registration bug rather than an over-broad scan.

The same scan also makes the compaction assertions unsound:

  • countTsFiles(...) >= 2 (precondition before compaction) can be satisfied by a single data
    file plus an unrelated TsFile;
  • until(countTsFiles(...) == 1) and findSingleTsFile(...) can never hold while an unrelated
    TsFile is present.

Change

  • add listDatabaseTsFiles(env), which lists only the TsFiles under
    <dataPath>/sequence/load_storage_db and <dataPath>/unsequence/load_storage_db;
  • route assertAllTsFilesHaveTableSchema, countTsFiles and findSingleTsFile through it;
  • keep every assertion unchanged (nothing is relaxed) and name the offending TsFile in the
    failure messages.

Test plan

  • Reproduced the original failure before the change, on a build where the DataNode also stores a
    tree-model system database, and confirmed it is gone with the change while that database still
    exists (i.e. the fix is in the scan scope, not in a weakened assertion).
  • mvn spotless:apply -pl integration-test -P with-integration-tests
  • mvn verify -DskipUTs -Dit.test=IoTDBLoadStorageTsFileWithCompactionIT -DfailIfNoTests=false -Dfailsafe.failIfNoSpecifiedTests=false -pl integration-test -P with-integration-tests
    → Tests run: 1, Failures: 0, Errors: 0

IoTDBLoadStorageTsFileWithCompactionIT walked the whole DataNode data
directory and required every TsFile below it to carry the table schema of
table t1, and the TsFile counting helpers were built on the same scan.

That directory is shared by every database of the node.  Internal
databases that store system records are tree-model databases, whose
TsFiles legitimately have an empty table schema map, so the assertion
fails with

  TsFile has no schema entry for table t1: {}

as soon as the node holds such a database (observed on a downstream
distribution that records the password history of "root" in root.__audit).
The same files also polluted the counting helpers: "two TsFiles before
compaction" could be satisfied by a single data file, and "exactly one
TsFile after compaction" could never hold.

Restrict every scan to <dataPath>/{sequence,unsequence}/load_storage_db,
keep the assertions unchanged, and name the offending file in the failure
messages.

@Caideyipi Caideyipi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed commit 95d7335c6ea24afd08aff57d98de285a0b1b8c5d. I found no actionable issue in the Java changes: the database directory matches the storage layout, both sequence and unsequence files remain covered, and the schema checks and compaction counts use the same scope.

[P3] Correct the test profile in the test plan. The documented command enables only with-integration-tests, leaving the integration-test module's default SimpleIT profile active. That profile selects LocalStandaloneIT, whereas this class is categorized only as MultiClusterIT2DualTableManualBasic. The -Dit.test class filter does not override the category filter, and the no-tests flags allow a successful exit without executing this test. Please add -P MultiClusterIT2DualTableManualBasic to the documented command (alongside -P with-integration-tests) and confirm that a fresh Failsafe report records testLoadStorageTsFileAfterCompaction. The reported one-test result is not reproducible from the listed command against the upstream POM as written.

Validation for this review: source inspection and git diff --check passed; I did not rerun the IT. CI was still in progress when inspected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants