STAR-894 Port Mockable FS from OSS by djatnieks · Pull Request #269 · datastax/cassandra

djatnieks · 2021-10-18T01:50:12Z

Co-authored-by: Benedict Elliott Smith <benedict@apache.org> Co-authored-by: Aleksey Yeschenko <aleksey@apache.org>

…d OSS ticket.

jasonstack · 2021-10-26T03:12:31Z

        writeSnapshotManifest(filesJSONArr, snapshotName);
        if (!SchemaConstants.isLocalSystemKeyspace(metadata.keyspace) && !SchemaConstants.isReplicatedSystemKeyspace(metadata.keyspace))
            writeSnapshotSchema(snapshotName);
+        if (!SchemaConstants.isLocalSystemKeyspace(metadata.keyspace) && !SchemaConstants.isReplicatedSystemKeyspace(metadata.keyspace))


jasonstack · 2021-10-26T03:20:30Z

+     * Directory where Cassandra persists logs from audit logging. If this property is not set, the audit log framework
+     * will set it automatically to {@link CassandraRelevantProperties#LOG_DIR} + "/audit".
+     */
+    LOG_DIR_AUDIT("cassandra.logdir.audit"),


not part of mockable FS

jasonstack · 2021-10-26T03:23:24Z

+    /**
+     * Directory where Cassandra puts its logs, defaults to "." which is current directory.
+     */
+    LOG_DIR("cassandra.logdir", "."),


not part of mockable FS

jasonstack · 2021-10-26T03:57:49Z

     * Otherwise recreate the bloom filter using the current FP chance.
     *
-     * @param sstableMetadata the sstable metadata, for extracting and changing the FP chance
+     * @param InvalidateJmxPermissionsCacheTestsstableMetadata the sstable metadata, for extracting and changing the FP chance


param mismatch

That was an inadvertent change on my part, as this file is only in CC ds-trunk and the only intended change was the import for org.apache.cassandra.io.util.File.

It looks like these javadoc params have been incorrect since this file was introduced; I could simply revert my change, but I think it should be ok to just remove these params.

jasonstack · 2021-10-26T06:13:22Z

    }
-}
+
+    public static RoleOptions getLoginRoleOptions()


not part of mockable FS

jasonstack

@djatnieks Thanks for the great work. Overall looks good to me, left some minor comments

JeremiahDJordan · 2021-10-26T22:31:55Z

72.7% Coverage

I thought our coverage threshold was %80, is it only %70?

k-rus · 2021-10-27T07:04:51Z

-                throw new ConfigurationException(String.format("Unable to check disk space available to '%s'. Perhaps the Cassandra user does not have the necessary permissions",
-                                                               conf.commitlog_directory), e);
-            }
+            // use 1/4 of available space.  See discussion on #10013 and #10199


Where are the references to? Since this repo is public, I guess it is only OK to refer to Apache ticket.

Yes, use of # seems to be the convention for referencing apache cassandra tickets; I see it all over the code.

I experienced previously that notation of CASSANDRA-XXXXX was used. But now I see that there are files with the same notation as here.

k-rus · 2021-10-27T07:05:23Z

-                    throw new ConfigurationException(String.format("Unable to check disk space available to '%s'. Perhaps the Cassandra user does not have the necessary permissions",
-                                                                   conf.cdc_raw_directory), e);
-                }
+                // use 1/8th of available space.  See discussion on #10013 and #10199 on the CL, taking half that for CDC


Same comment about referencing tickets.

See previous response, but yes, it is a C* ticket reference.

djatnieks · 2021-10-27T23:09:01Z

72.7% Coverage

I thought our coverage threshold was %80, is it only %70?

I thought so too, and was (pleasantly) surprised it passed with just over 70% :) Assuming the intention is to maintain 80% then I'll work to add more test coverage here.

djatnieks · 2021-10-27T23:23:30Z

@djatnieks Thanks for the great work. Overall looks good to me, left some minor comments

Thanks for the review @jasonstack, esp. for finding those changes not related to mockable FS - I tried to remove as many as I could spot myself and knew there could be more :)

k-rus

I have noticed number of nits, which you see in my comments. I understand that this PR is a port of existing code, still it was discussed that the code standard can be improved and keeping as close as possible to original code is not a target.

I noticed that the patch expands generic imports into specific packages in some files, e.g., java.util.*, but in some files it is not. Is it something for consideration in this PR?

I have some code formatting comments on test code, as I believe the test code should be readable as any other code.

JeremiahDJordan · 2021-10-28T12:42:29Z

One comment on this. Any changes made to the code to improve it we should make sure to keep track of for submission upstream to OSS C*. While improving the code is good, being close to OSS C* is also good, so we should try to do both as much as we can.

k-rus · 2021-10-28T18:57:58Z

One comment on this. Any changes made to the code to improve it we should make sure to keep track of for submission upstream to OSS C*. While improving the code is good, being close to OSS C* is also good, so we should try to do both as much as we can.

@JeremiahDJordan I guess it is a reply to my comment :) The ported code comes from DSE if I don't mistake. My minor suggestions for improvement will likely to bring the ported code closer to OSS requirements.

JeremiahDJordan · 2021-10-28T20:36:42Z

@k-rus Actually the code here comes from Cassandra trunk. We are porting it back to our 4.0 based branch

djatnieks · 2021-10-29T01:26:03Z

@k-rus Thank you for the review!

As mentioned above, these changes are being ported from C* OSS trunk branch, which is unreleased C* 4.1 (presumably, anyway the next release).

I agree that as we have ported DSE code to CC, we've often taken the opportunity to improve code, however that's a one-time port. Since this is coming from a future OSS release branch, we do have to consider that at some point in the future we'll refresh our code again that may result in merge conflicts.

I applied some suggestions that are not purely formatting. There is an OSS C* code style along with IntelliJ and Eclipse files that can be imported. For instance, I do not believe the C* code style requires if/else to be on separate lines from single expressions. I did not (intentionally) reformat any code when porting the code.

I always find imports somewhat confusing with regard to the rules around when to use .* and when to list each import separately. I sometimes wonder if the import rules are contained in the code style file.

While I was porting I did notice places with unused imports and with out of order (alphabetically) imports and I did occasionally let IntelliJ rearrange them. I hope I did not introduce more imports inconsistency, but I think there is a certain amount always present. Also, I think that merge conflicts with imports are the easiest to handle, as it's possible to accept both changes and then rearrange if needed.

sonarqubecloud · 2021-10-29T04:44:13Z

Kudos, SonarCloud Quality Gate passed!

1 Bug
0 Vulnerabilities
0 Security Hotspots
51 Code Smells

74.3% Coverage
0.0% Duplication

k-rus · 2021-10-29T07:11:33Z

Sorry that I missed that it is port from OSS C*. It is described in the ticket but I somehow missed it.
I believe then there is no comments from me :)

* STAR-894 Port [CASSANDRA-16926] CEP-10 Phase 1: Mockable Filesystem Co-authored-by: Benedict Elliott Smith <benedict@apache.org> Co-authored-by: Aleksey Yeschenko <aleksey@apache.org>

* STAR-894 Port [CASSANDRA-16926] CEP-10 Phase 1: Mockable Filesystem Co-authored-by: Benedict Elliott Smith <benedict@apache.org> Co-authored-by: Aleksey Yeschenko <aleksey@apache.org> (cherry picked from commit 477fda8) (cherry picked from commit c459c65)

* STAR-894 Port [CASSANDRA-16926] CEP-10 Phase 1: Mockable Filesystem Co-authored-by: Benedict Elliott Smith <benedict@apache.org> Co-authored-by: Aleksey Yeschenko <aleksey@apache.org> (cherry picked from commit 477fda8) (cherry picked from commit c459c65) (cherry picked from commit 545c778)

* STAR-894 Port [CASSANDRA-16926] CEP-10 Phase 1: Mockable Filesystem Co-authored-by: Benedict Elliott Smith <benedict@apache.org> Co-authored-by: Aleksey Yeschenko <aleksey@apache.org> (cherry picked from commit 477fda8) (cherry picked from commit c459c65) (cherry picked from commit 545c778) (cherry picked from commit 7ae967f)

* STAR-894 Port [CASSANDRA-16926] CEP-10 Phase 1: Mockable Filesystem Co-authored-by: Benedict Elliott Smith <benedict@apache.org> Co-authored-by: Aleksey Yeschenko <aleksey@apache.org> (cherry picked from commit 477fda8) (cherry picked from commit c459c65) (cherry picked from commit 545c778) (cherry picked from commit 7ae967f) (cherry picked from commit 3528cd9)

* STAR-894 Port [CASSANDRA-16926] CEP-10 Phase 1: Mockable Filesystem Co-authored-by: Benedict Elliott Smith <benedict@apache.org> Co-authored-by: Aleksey Yeschenko <aleksey@apache.org> (cherry picked from commit 477fda8) (cherry picked from commit c459c65) (cherry picked from commit 545c778) (cherry picked from commit 7ae967f) (cherry picked from commit 3528cd9) STAR-894 Fix MockSchema missing import for DeletionTime (cherry picked from commit 3f353a1) STAR-894 Fix BinLog conflicts with CASSANDRA-17933 (cherry picked from commit e9aa46d)

* STAR-894 Port [CASSANDRA-16926] CEP-10 Phase 1: Mockable Filesystem Co-authored-by: Benedict Elliott Smith <benedict@apache.org> Co-authored-by: Aleksey Yeschenko <aleksey@apache.org> (cherry picked from commit 477fda8) (cherry picked from commit c459c65) (cherry picked from commit 545c778) (cherry picked from commit 7ae967f) (cherry picked from commit 3528cd9) STAR-894 Fix MockSchema missing import for DeletionTime (cherry picked from commit 3f353a1) STAR-894 Fix BinLog conflicts with CASSANDRA-17933 (cherry picked from commit e9aa46d) (cherry picked from commit 5d20b49)

* STAR-894 Port [CASSANDRA-16926] CEP-10 Phase 1: Mockable Filesystem Co-authored-by: Benedict Elliott Smith <benedict@apache.org> Co-authored-by: Aleksey Yeschenko <aleksey@apache.org> (cherry picked from commit 477fda8) (cherry picked from commit c459c65) (cherry picked from commit 545c778) (cherry picked from commit 7ae967f) (cherry picked from commit 3528cd9) STAR-894 Fix MockSchema missing import for DeletionTime (cherry picked from commit 3f353a1) STAR-894 Fix BinLog conflicts with CASSANDRA-17933 (cherry picked from commit e9aa46d) (cherry picked from commit 5d20b49) (Rebase of commit 417b322)

djatnieks and others added 4 commits October 17, 2021 18:48

STAR-894 Port [CASSANDRA-16926] CEP-10 Phase 1: Mockable Filesystem

20f4202

Co-authored-by: Benedict Elliott Smith <benedict@apache.org> Co-authored-by: Aleksey Yeschenko <aleksey@apache.org>

STAR-894 Fix test testSnapshotWithoutFlushWithSecondaryIndexes

34dc9c8

STAR-894 Delete class FileSystemOwnershipCheck introduced in unrelate…

1db1181

…d OSS ticket.

STAR-894 Improve test coverage

d561066

jasonstack reviewed Oct 26, 2021

View reviewed changes

k-rus reviewed Oct 27, 2021

View reviewed changes

STAR-894 Apply review comments

28b8b4b

jasonstack approved these changes Oct 28, 2021

View reviewed changes

k-rus approved these changes Oct 28, 2021

View reviewed changes

STAR-894 Apply more review comments and improve test coverage.

3c048ea

djatnieks merged commit 477fda8 into ds-trunk Oct 29, 2021

djatnieks deleted the STAR-894-port-mockable-fs branch October 29, 2021 18:51

Conversation

djatnieks commented Oct 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonstack left a comment

Choose a reason for hiding this comment

Uh oh!

JeremiahDJordan commented Oct 26, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

djatnieks commented Oct 27, 2021

Uh oh!

djatnieks commented Oct 27, 2021

Uh oh!

k-rus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JeremiahDJordan commented Oct 28, 2021

Uh oh!

k-rus commented Oct 28, 2021

Uh oh!

JeremiahDJordan commented Oct 28, 2021

Uh oh!

djatnieks commented Oct 29, 2021

Uh oh!

sonarqubecloud Bot commented Oct 29, 2021

Uh oh!

k-rus commented Oct 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants