Skip to content

CASSANDRA-18111 snapshotting centralisation#3374

Closed
smiklosovic wants to merge 1 commit into
apache:trunkfrom
instaclustr:CASSANDRA-18111
Closed

CASSANDRA-18111 snapshotting centralisation#3374
smiklosovic wants to merge 1 commit into
apache:trunkfrom
instaclustr:CASSANDRA-18111

Conversation

@smiklosovic
Copy link
Copy Markdown
Contributor

@smiklosovic smiklosovic commented Jun 13, 2024

Thanks for sending a pull request! Here are some tips if you're new here:

  • Ensure you have added or run the appropriate tests for your PR.
  • Be sure to keep the PR description updated to reflect all changes.
  • Write your PR title to summarize what this PR proposes.
  • If possible, provide a concise example to reproduce the issue for a faster review.
  • Read our contributor guidelines
  • If you're making a documentation change, see our guide to documentation contribution

Commit messages should follow the following format:

<One sentence description, usually Jira title or CHANGES.txt summary>

<Optional lengthier description (context on patch)>

patch by <Authors>; reviewed by <Reviewers> for CASSANDRA-#####

Co-authored-by: Name1 <email1>
Co-authored-by: Name2 <email2>

The Cassandra Jira

smiklosovic
smiklosovic commented Jun 13, 2024
View reviewed changes
Comment thread src/java/org/apache/cassandra/service/snapshot/SnapshotWatcher.java Outdated
@smiklosovic smiklosovic force-pushed the CASSANDRA-18111 branch 18 times, most recently from 3bc670a to 15d9eef Compare June 20, 2024 08:30
frankgh
frankgh reviewed Jul 1, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@frankgh frankgh left a comment

Choose a reason for hiding this comment

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

This is a work in progress review. I will add more comments in the next couple of days if anything else catches my eye

Comment thread tools/stress/src/org/apache/cassandra/stress/CompactionStress.java
Comment thread src/java/org/apache/cassandra/service/SnapshotVerbHandler.java Outdated
Comment thread src/java/org/apache/cassandra/db/ColumnFamilyStore.java Outdated
Comment thread src/java/org/apache/cassandra/db/ColumnFamilyStore.java Outdated
frankgh
frankgh reviewed Jul 18, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@frankgh frankgh left a comment

Choose a reason for hiding this comment

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

Added some additional comments

Comment thread test/unit/org/apache/cassandra/service/snapshot/TableSnapshotTest.java Outdated
Comment thread test/distributed/org/apache/cassandra/distributed/impl/Instance.java Outdated
Comment thread src/java/org/apache/cassandra/service/snapshot/TableSnapshot.java
Comment thread src/java/org/apache/cassandra/service/StorageService.java Outdated
Comment thread src/java/org/apache/cassandra/service/StorageService.java Outdated
Comment thread src/java/org/apache/cassandra/service/StorageService.java Outdated
Comment thread test/unit/org/apache/cassandra/db/DirectoriesTest.java
Comment thread src/java/org/apache/cassandra/db/Directories.java
Comment thread src/java/org/apache/cassandra/db/Directories.java
Comment thread src/java/org/apache/cassandra/service/snapshot/SnapshotWatcher.java Outdated
@smiklosovic smiklosovic force-pushed the CASSANDRA-18111 branch 2 times, most recently from 43c9baa to cbc5362 Compare July 22, 2024 12:05
frankgh
frankgh approved these changes Jul 22, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@frankgh frankgh left a comment

Choose a reason for hiding this comment

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

Took a second look. Looks good to me

@smiklosovic smiklosovic force-pushed the CASSANDRA-18111 branch 4 times, most recently from d0bde54 to 0eb8fe7 Compare July 24, 2024 09:42
@smiklosovic smiklosovic force-pushed the CASSANDRA-18111 branch 4 times, most recently from 70f5952 to a518ea9 Compare September 25, 2024 16:21
@smiklosovic smiklosovic force-pushed the CASSANDRA-18111 branch 4 times, most recently from 77e6605 to 91a4898 Compare November 3, 2024 17:09
@smiklosovic smiklosovic force-pushed the CASSANDRA-18111 branch 3 times, most recently from f207044 to 2740072 Compare November 12, 2024 18:07
dcapwell
dcapwell reviewed Nov 12, 2024
View reviewed changes
Comment thread test/distributed/org/apache/cassandra/fuzz/snapshots/SnapshotsTest.java Outdated
Comment thread test/distributed/org/apache/cassandra/fuzz/snapshots/SnapshotsTest.java Outdated
Comment thread test/distributed/org/apache/cassandra/fuzz/snapshots/SnapshotsTest.java Outdated
Comment thread test/distributed/org/apache/cassandra/fuzz/snapshots/SnapshotsTest.java Outdated
Comment thread test/distributed/org/apache/cassandra/fuzz/snapshots/SnapshotsTest.java Outdated
Comment thread test/distributed/org/apache/cassandra/fuzz/snapshots/SnapshotsTest.java Outdated
Comment thread test/distributed/org/apache/cassandra/fuzz/snapshots/SnapshotsTest.java Outdated
Comment thread test/distributed/org/apache/cassandra/fuzz/snapshots/SnapshotsTest.java Outdated
Comment thread test/distributed/org/apache/cassandra/fuzz/snapshots/SnapshotsTest.java Outdated
Comment thread test/distributed/org/apache/cassandra/fuzz/snapshots/SnapshotsTest.java Outdated
@smiklosovic smiklosovic force-pushed the CASSANDRA-18111 branch 2 times, most recently from f7047c1 to cc8bffd Compare November 13, 2024 13:46
@smiklosovic smiklosovic changed the title CASSANDRA-18111 cache snapshots in memory CASSANDRA-18111 snapshotting centralisation Nov 26, 2024
@smiklosovic smiklosovic force-pushed the CASSANDRA-18111 branch 11 times, most recently from af1e5fb to c156ab4 Compare December 3, 2024 12:02
frankgh
frankgh reviewed Dec 7, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@frankgh frankgh left a comment

Choose a reason for hiding this comment

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

I've added some additional comments

Comment thread src/java/org/apache/cassandra/service/snapshot/SnapshotManager.java Outdated
Comment thread src/java/org/apache/cassandra/io/util/PathUtils.java Outdated
Comment thread src/java/org/apache/cassandra/repair/PreviewRepairTask.java Outdated
Comment thread src/java/org/apache/cassandra/service/CassandraDaemon.java Outdated
Comment thread src/java/org/apache/cassandra/service/StorageServiceMBean.java Outdated
Comment thread src/java/org/apache/cassandra/service/snapshot/ClearSnapshotTask.java Outdated
frankgh
frankgh reviewed Dec 9, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@frankgh frankgh left a comment

Choose a reason for hiding this comment

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

small final comments

Comment thread test/unit/org/apache/cassandra/index/sasi/SASIIndexTest.java Outdated
Comment thread src/java/org/apache/cassandra/db/compaction/CompactionTask.java Outdated
Comment thread src/java/org/apache/cassandra/service/snapshot/TableSnapshot.java Outdated
Comment thread src/java/org/apache/cassandra/db/repair/CassandraValidationIterator.java Outdated
@smiklosovic
Consolidate all snapshot management to SnapshotManager
28e72d5 
patch by Stefan Miklosovic; reviewed by Francisco Guerrero for CASSANDRA-18111
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pauloricardomg pauloricardomg pauloricardomg left review comments

@dcapwell dcapwell dcapwell left review comments

@frankgh frankgh frankgh approved these changes

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

@smiklosovic @pauloricardomg @dcapwell @frankgh