Skip to content

CNDB-16697: CNDB-16135: reuse table directory from Directories into Descriptor#2277

Open
zgorzalyj wants to merge 2 commits intomain-5.0from
CNDB-16697-main-5.0
Open

CNDB-16697: CNDB-16135: reuse table directory from Directories into Descriptor#2277
zgorzalyj wants to merge 2 commits intomain-5.0from
CNDB-16697-main-5.0

Conversation

@zgorzalyj
Copy link
Copy Markdown

@zgorzalyj zgorzalyj commented Mar 17, 2026 •
edited
Loading

https://github.com/riptano/cndb/issues/16697

Port into main-5.0

commit 79f25c7
Author: Zhao Yang zhaoyangsingapore@gmail.com
Date: Tue Dec 9 16:24:25 2025 +0800

CNDB-16135: reuse table directory from Directories into Descriptor (#2150)

### What is the issue
CNDB-16135: Descriptor uses different instances of `Path directory` for
the same table as constructor parameter

### What does this PR fix and why was it fixed

Use the table directory from `Directories` in `Descriptor` as
constructor parameter . `Descriptor#directory` is still different
instance in C* due to `directory#toCanonical()` which always creates new
instance in local file system

@jasonstack @zgorzalyj
CNDB-16135: reuse table directory from Directories into Descriptor (#…
c8e2585 
…2150)

CNDB-16135: Descriptor uses different instances of `Path directory` for
the same table as constructor parameter

Use the table directory from `Directories` in `Descriptor` as
constructor parameter . `Descriptor#directory` is still different
instance in C* due to `directory#toCanonical()` which always creates new
instance in local file system
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 17, 2026

Checklist before you submit for review

  • This PR adheres to the Definition of Done
  • Make sure there is a PR in the CNDB project updating the Converged Cassandra version
  • Use NoSpamLogger for log lines that may appear frequently in the logs
  • Verify test results on Butler
  • Test coverage for new/modified code is > 80%
  • Proper code formatting
  • Proper title for each commit staring with the project-issue number, like CNDB-1234
  • Each commit has a meaningful description
  • Each commit is not very long and contains related changes
  • Renames, moves and reformatting are in distinct commits
  • All new files should contain the DataStax copyright header instead of the Apache License one

@zgorzalyj zgorzalyj changed the title CNDB-16135: reuse table directory from Directories into Descriptor (#… CNDB16697: CNDB-16135: reuse table directory from Directories into Descriptor Mar 17, 2026
@zgorzalyj zgorzalyj marked this pull request as ready for review March 18, 2026 09:13
@zgorzalyj zgorzalyj requested a review from djatnieks March 18, 2026 09:13
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Mar 18, 2026

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@cassci-bot
Copy link
Copy Markdown

cassci-bot commented Mar 18, 2026

❌ Build ds-cassandra-pr-gate/PR-2277 rejected by Butler

3 regressions found
See build details here

Found 3 new test failures

Test Explanation Runs Upstream
o.a.c.index.sai.cql.VectorCompaction100dTest.testZeroOrOneToManyCompaction[ca false] () NEW 🔴 0 / 25
o.a.c.index.sai.cql.VectorSiftSmallTest.testCompaction[ca false] () NEW 🔴 0 / 25
o.a.c.index.sai.disk.v1.SegmentFlushTest.testFlushBetweenRowIds[aa] () NEW 🔴 0 / 25

Found 1 known test failures

djatnieks
djatnieks requested changes Mar 19, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@djatnieks djatnieks left a comment

Choose a reason for hiding this comment

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

The changes you have did match the ones I originally recommended.

However, I made a mistake in thinking the existing methods in 5.0 could be re-used. We do need the additional override method and we do need to avoid using new File.

The original fix here was specifically for CNDB code where we use a RemoteFilesystem and RemotePath, and (in CNDB) calls through PathUtils.toCanonicalPath() is able to return the same instance rather than always creating new instances.

Comment thread src/java/org/apache/cassandra/io/sstable/Descriptor.java

/**
* Parse a sstable filename, extracting both the {@code Descriptor} and {@code Component} part.
* The keyspace/table name will be extracted from the directory path.
Copy link
Copy Markdown
Member

@djatnieks djatnieks Mar 19, 2026

Choose a reason for hiding this comment

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

nit: suggest to keep this line, as it was not present/affected I think on main branch

* The keyspace/table name will be extracted from the directory path.

Comment thread src/java/org/apache/cassandra/db/Directories.java
File dir = dataPaths[dirNumber];
File file = dir.resolve(filename);
return Descriptor.fromFile(file);
return Descriptor.fromFileWithComponent(new File(dir, filename), true).left;
Copy link
Copy Markdown
Member

@djatnieks djatnieks Mar 19, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
return Descriptor.fromFileWithComponent(new File(dir, filename), true).left;
return Descriptor.fromFileWithComponent(dir, filename, true).left;

Actually, I think using new File here defeats the purpose of the original ticket, so I made a few additional changes.

Copy link
Copy Markdown
Member

@djatnieks djatnieks Apr 7, 2026

Choose a reason for hiding this comment

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

@zgorzalyj I think this should be changed to

        return Descriptor.fromFileWithComponent(dir, filename, true).left;

to use the change you made in commit 97ab09d

Comment thread src/java/org/apache/cassandra/io/sstable/Descriptor.java Outdated
Comment thread src/java/org/apache/cassandra/io/sstable/Descriptor.java Outdated
Comment thread src/java/org/apache/cassandra/io/sstable/Descriptor.java Outdated
Comment thread src/java/org/apache/cassandra/io/sstable/Descriptor.java Outdated
Comment thread src/java/org/apache/cassandra/io/sstable/Descriptor.java Outdated
Comment thread src/java/org/apache/cassandra/io/sstable/Descriptor.java Outdated
Comment thread src/java/org/apache/cassandra/io/sstable/Descriptor.java Outdated
Comment thread src/java/org/apache/cassandra/io/sstable/Descriptor.java Outdated
@djatnieks djatnieks requested a review from jasonstack March 19, 2026 23:12
@djatnieks
Copy link
Copy Markdown
Member

djatnieks commented Mar 19, 2026

@jasonstack Hello, can you help review these changes please? I think the original guidance I gave to @zgorzalyj was not correct and I have attempted to correct that, but your input would be appreciated!

@jasonstack
Copy link
Copy Markdown

jasonstack commented Mar 23, 2026

@djatnieks I will take a look this week

@zgorzalyj zgorzalyj changed the title CNDB16697: CNDB-16135: reuse table directory from Directories into Descriptor CNDB-16697: CNDB-16135: reuse table directory from Directories into Descriptor Mar 23, 2026
@zgorzalyj @djatnieks
CNDB-16697 apply code review suggestions to align with original commit
97ab09d 
Co-authored-by: Daniel Jatnieks <dan.jatnieks@ibm.com>
@djatnieks
Copy link
Copy Markdown
Member

djatnieks commented Apr 6, 2026

@jasonstack Did you have a chance to review?

jasonstack
jasonstack approved these changes Apr 7, 2026
View reviewed changes
Copy link
Copy Markdown

@jasonstack jasonstack left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM.

CNDB side has unit tests with remote file system to verify the changes in CC. When CC5 is ready to integrate with CNDB, we will be able to validate the changes here

djatnieks
djatnieks requested changes Apr 7, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@djatnieks djatnieks left a comment

Choose a reason for hiding this comment

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

I think one more change is needed

Comment thread src/java/org/apache/cassandra/db/Directories.java
File dir = dataPaths[dirNumber];
File file = dir.resolve(filename);
return Descriptor.fromFile(file);
return Descriptor.fromFileWithComponent(new File(dir, filename), true).left;
Copy link
Copy Markdown
Member

@djatnieks djatnieks Apr 7, 2026

Choose a reason for hiding this comment

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

@zgorzalyj I think this should be changed to

        return Descriptor.fromFileWithComponent(dir, filename, true).left;

to use the change you made in commit 97ab09d

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

Reviewers

@djatnieks djatnieks djatnieks requested changes

@jasonstack jasonstack jasonstack approved these changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@zgorzalyj @cassci-bot @djatnieks @jasonstack