STAR-933 Add methods to PathUtils need for serverless by djatnieks · Pull Request #288 · datastax/cassandra

djatnieks · 2021-11-22T00:40:40Z

No description provided.

k-rus

I suggest to fix the patch so it follows existing code in the file and remove unnecessary code duplication.

I also suggest to provide a meaningful commit message. Note that the discussion in the ticket doesn't really help to understand this patch.

I hope you are also looking to add new API to tests.

djatnieks · 2021-11-23T02:33:47Z

I also suggest to provide a meaningful commit message. Note that the discussion in the ticket doesn't really help to understand this patch.

Sure, I will update the commit message when squashing; I could perhaps describe the methods added:

Add deleteContent method that recursively deletes the contents of a directory, leaving the directory empty;
Add listPaths methods to list all the paths in a directory, optionally using a provided filter.

Suggestions welcome.

k-rus · 2021-11-23T09:45:20Z

Sure, I will update the commit message when squashing; I could perhaps describe the methods added:

Add deleteContent method that recursively deletes the contents of a directory, leaving the directory empty;

Add listPaths methods to list all the paths in a directory, optionally using a provided filter.

Suggestions welcome.

It is good together with the current PR subject. So it is clear that the commit adds new API functions for the serverless/AstraDB need.

k-rus

LGTM
I have just two requests:

Can you update PR message with the commit message to be used on squash?
Can you update comments for new API functions, so they follow the documentation standard?

k-rus · 2021-11-23T10:04:30Z

I don't know if it is expected to always have a new line at the end of file or not. Here it is missing.

My understanding is a new line is expected, so I will add it

k-rus · 2021-11-23T10:05:18Z

Should the comment follow the javadoc pattern as in all other methods as it is part of API?

k-rus · 2021-11-23T10:15:49Z

I am wonder if the reason for this check is an assumption that default file system stored in filesystem can be used in and is used in several API functions. If the file system is different from default, should the API functions not rely on the default? If I understood correctly, the default file system is used for getPath.

This check prevents File being able to wrap a remote filesystem path; that is why the check is removed.
My checking of the uses of filesystem are:

in constructors when a Path is not provided

in pathSeparator(); this is potential issue, however all known filesystem implementations currently use the same separator value (/)

in toPath() only when path is null

There is also some comment about this in the jira.

k-rus · 2021-11-23T10:29:07Z

Apache Cassandra requires to specify each imported package explicitly, but it is not forced yet.

Add methods to PathUtils: * deleteContent method that recursively deletes the contents of a directory, leaving the directory empty; * listPaths methods to list all the paths in a directory, optionally using a provided filter. Add method to Descriptor: * validFilenameWithComponent to return the Component from an sstable file name

sonarqubecloud · 2021-11-24T06:03:56Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

85.4% Coverage
0.0% Duplication

Add methods to PathUtils: * deleteContent method that recursively deletes the contents of a directory, leaving the directory empty; * listPaths methods to list all the paths in a directory, optionally using a provided filter. Add method to Descriptor: * validFilenameWithComponent to return the Component from an sstable file name (cherry picked from commit 4f1c86b)

Add methods to PathUtils: * deleteContent method that recursively deletes the contents of a directory, leaving the directory empty; * listPaths methods to list all the paths in a directory, optionally using a provided filter. Add method to Descriptor: * validFilenameWithComponent to return the Component from an sstable file name (cherry picked from commit 4f1c86b) (cherry picked from commit ef840e5)

Add methods to PathUtils: * deleteContent method that recursively deletes the contents of a directory, leaving the directory empty; * listPaths methods to list all the paths in a directory, optionally using a provided filter. Add method to Descriptor: * validFilenameWithComponent to return the Component from an sstable file name (cherry picked from commit 4f1c86b) (cherry picked from commit ef840e5) (cherry picked from commit dc75ab6)

Add methods to PathUtils: * deleteContent method that recursively deletes the contents of a directory, leaving the directory empty; * listPaths methods to list all the paths in a directory, optionally using a provided filter. Add method to Descriptor: * validFilenameWithComponent to return the Component from an sstable file name (cherry picked from commit 4f1c86b) (cherry picked from commit ef840e5) (cherry picked from commit dc75ab6) (cherry picked from commit a5a4794)

Add methods to PathUtils: * deleteContent method that recursively deletes the contents of a directory, leaving the directory empty; * listPaths methods to list all the paths in a directory, optionally using a provided filter. Add method to Descriptor: * validFilenameWithComponent to return the Component from an sstable file name (cherry picked from commit 4f1c86b) (cherry picked from commit ef840e5) (cherry picked from commit dc75ab6) (cherry picked from commit a5a4794) (cherry picked from commit 1570f19)

Add methods to PathUtils: * deleteContent method that recursively deletes the contents of a directory, leaving the directory empty; * listPaths methods to list all the paths in a directory, optionally using a provided filter. Add method to Descriptor: * validFilenameWithComponent to return the Component from an sstable file name (cherry picked from commit 4f1c86b) (cherry picked from commit ef840e5) (cherry picked from commit dc75ab6) (cherry picked from commit a5a4794) (cherry picked from commit 1570f19) STAR-933 Fix PathUtilsTest with initialization of DatabaseDescriptor STAR-933 Add Descriptor.validFilenameWithComponent expected by CNDB

Add methods to PathUtils: * deleteContent method that recursively deletes the contents of a directory, leaving the directory empty; * listPaths methods to list all the paths in a directory, optionally using a provided filter. Add method to Descriptor: * validFilenameWithComponent to return the Component from an sstable file name (cherry picked from commit 4f1c86b) (cherry picked from commit ef840e5) (cherry picked from commit dc75ab6) (cherry picked from commit a5a4794) (cherry picked from commit 1570f19) STAR-933 Fix PathUtilsTest with initialization of DatabaseDescriptor STAR-933 Add Descriptor.validFilenameWithComponent expected by CNDB (Rebase of commit a3c3f18)

k-rus requested changes Nov 22, 2021

View reviewed changes

Comment thread src/java/org/apache/cassandra/io/util/PathUtils.java Outdated

Comment thread src/java/org/apache/cassandra/io/util/PathUtils.java Outdated

Comment thread src/java/org/apache/cassandra/io/util/PathUtils.java Outdated

k-rus reviewed Nov 23, 2021

View reviewed changes

djatnieks force-pushed the STAR-933 branch from 435a359 to 6befac6 Compare November 23, 2021 22:48

djatnieks force-pushed the STAR-933 branch from 6befac6 to 17a30dd Compare November 24, 2021 01:06

k-rus approved these changes Nov 24, 2021

View reviewed changes

djatnieks merged commit 4f1c86b into ds-trunk Nov 25, 2021

djatnieks deleted the STAR-933 branch November 25, 2021 21:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

STAR-933 Add methods to PathUtils need for serverless#288

STAR-933 Add methods to PathUtils need for serverless#288
djatnieks merged 1 commit intods-trunkfrom
STAR-933

djatnieks commented Nov 22, 2021

Uh oh!

k-rus left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

djatnieks commented Nov 23, 2021

Uh oh!

k-rus commented Nov 23, 2021

Uh oh!

k-rus left a comment

Uh oh!

k-rus Nov 23, 2021

Uh oh!

djatnieks Nov 23, 2021

Uh oh!

k-rus Nov 23, 2021

Uh oh!

k-rus Nov 23, 2021

Uh oh!

djatnieks Nov 23, 2021

Uh oh!

k-rus Nov 23, 2021

Uh oh!

sonarqubecloud Bot commented Nov 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

djatnieks commented Nov 22, 2021

Uh oh!

k-rus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

djatnieks commented Nov 23, 2021

Uh oh!

k-rus commented Nov 23, 2021

Uh oh!

k-rus left a comment

Choose a reason for hiding this comment

Uh oh!

k-rus Nov 23, 2021

Choose a reason for hiding this comment

Uh oh!

djatnieks Nov 23, 2021

Choose a reason for hiding this comment

Uh oh!

k-rus Nov 23, 2021

Choose a reason for hiding this comment

Uh oh!

k-rus Nov 23, 2021

Choose a reason for hiding this comment

Uh oh!

djatnieks Nov 23, 2021

Choose a reason for hiding this comment

Uh oh!

k-rus Nov 23, 2021

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Nov 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants