-
Notifications
You must be signed in to change notification settings - Fork 3.8k
CASSANDRA-17619: Remove expired snapshots of dropped tables after restart #1615
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CASSANDRA-17619: Remove expired snapshots of dropped tables after restart #1615
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be explicit, this will be printed like: "Adding snapshots: ["snapshot1", "snapshot2","snapshot3"]" ?
I think there is some joining collector which just separates it by commas and leaves brackets. I consider it to be just nicer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. New output:
DEBUG [main] 2022-05-12 17:08:30,773 SnapshotManager.java:119 - Adding snapshots: ks:tbl:c7483545-103a-493e-b66e-6d96e60d3878:expired, ks:tbl:984bd8f1-35fd-44d7-b69b-52422614e25f:non-expired, ks:tbl:deff9907-495b-4497-8275-ffdfdfc3ad7b:non-expiring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ehm ... should not it be the name of the snapshot rather than keyspace:table:id? I mean, we are adding snapshots, I do not see the names of these snapshots or am I getting something wrong?
src/java/org/apache/cassandra/service/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why rf 2? dont we have just one node? Also, can no you use that distributed keyspace which is created already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why rf 2?
copied over from previous tests - fixed all tests to use RF=1 on 007958f.
Also, can no you use that distributed keyspace which is created already?
This whole class uses this format and fixing would be out of the scope of this ticket. Is it fine if we refactor this later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same, rf = 2, why not 1 and why dont we use default keyspace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed to use rf=1 on 007958f
test/distributed/org/apache/cassandra/distributed/test/SnapshotsTest.java
Outdated
Show resolved
Hide resolved
6731ee2 to
007958f
Compare
| @VisibleForTesting | ||
| protected synchronized void addSnapshots(Collection<TableSnapshot> snapshots) | ||
| { | ||
| logger.debug("Adding snapshots: {}.", Joiner.on(", ").join(snapshots.stream().map(s -> s.getId()).collect(Collectors.toList()))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks better but what I see is that we have "keyspace:table:id" rather than name of that snapshot (snapshot tag).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahaaaa I get this, so after "id" there is the snapshot tag, the whole name has four components, ks:tb:id:tag. I overlooked the last part. All good.
…ead of the query analyzer (apache#1615) Fix `RowFilter.AnalyzableExpression#numFilteredValues`, which is used by the `query_filters` guardrail, to use the query analyzer, instead of the index analyzer. That prevents wrong triggerings of the guardrail.
…ead of the query analyzer (apache#1615) Fix `RowFilter.AnalyzableExpression#numFilteredValues`, which is used by the `query_filters` guardrail, to use the query analyzer, instead of the index analyzer. That prevents wrong triggerings of the guardrail.
No description provided.