alex-ninja commented on a change in pull request #1189:
URL: https://github.com/apache/cassandra/pull/1189#discussion_r708662164
##########
File path:
test/distributed/org/apache/cassandra/distributed/test/SnapshotsTest.java
##########
@@ -180,10 +184,25 @@ public void testSecondaryIndexCleanup() throws Exception {
listSnapshotsResult.stdoutNotContains("first");
}
- private void populate(Cluster cluster) {
- for (int i = 0; i < 100; i++) {
- cluster.coordinator(1).execute("INSERT INTO default.tbl (key,
value) VALUES (?, 'txt')", ConsistencyLevel.ONE, i);
- }
+ @Test
+ public void testSameTimestampOnEachTableOfSnaphot()
+ {
+ cluster.get(1).nodetoolResult("snapshot", "-t",
"sametimestamp").asserts().success();
+ NodeToolResult result = cluster.get(1).nodetoolResult("listsnapshots");
+
+ int distinctDates = Arrays.stream(result.getStdout().split("\n"))
+ .filter(line ->
line.startsWith("sametimestamp"))
+ .map(line -> line.replaceAll(" +", "
").split(" ")[7])
+ .collect(Collectors.toSet())
Review comment:
nit: probably it would be a bit clearer:
```
.distinct()
.count()
```
##########
File path: src/java/org/apache/cassandra/service/snapshot/SnapshotManifest.java
##########
@@ -60,13 +60,18 @@ private SnapshotManifest() {
this.expiresAt = null;
}
- public SnapshotManifest(List<String> files, Duration ttl)
+ public SnapshotManifest(List<String> files, Duration ttl, long timestamp)
{
this.files = files;
- this.createdAt = Instant.now();
+ this.createdAt = Instant.ofEpochMilli(timestamp);
this.expiresAt = ttl == null ? null :
createdAt.plusMillis(ttl.toMilliseconds());
}
+ public SnapshotManifest(List<String> files, Duration ttl)
Review comment:
Here is a conceptual question - do we still need to have this
constructor? It is only used in tests. We can update tests to use the new one.
If we remove this constructor we will enforce developers to always pass a
timestamp, in turn that will not let them introduce the same issue as you're
fixing now.
The same is applicable to a few other methods you've updated.
##########
File path: src/java/org/apache/cassandra/db/ColumnFamilyStore.java
##########
@@ -1839,13 +1843,13 @@ public ClusteringComparator getComparator()
public TableSnapshot snapshotWithoutFlush(String snapshotName)
Review comment:
There is another usage of this method in `ConpactionTask`:
```
if (DatabaseDescriptor.isSnapshotBeforeCompaction())
cfs.snapshotWithoutFlush(System.currentTimeMillis() + "-compact-" +
cfs.name);
```
Looks like it has a timestamp in the name as well. Should we fix this one
too? If yes, probably, I'd just change the signature of this method and add
`timestamp` argument.
##########
File path:
test/distributed/org/apache/cassandra/distributed/test/SnapshotsTest.java
##########
@@ -180,10 +184,25 @@ public void testSecondaryIndexCleanup() throws Exception {
listSnapshotsResult.stdoutNotContains("first");
}
- private void populate(Cluster cluster) {
- for (int i = 0; i < 100; i++) {
- cluster.coordinator(1).execute("INSERT INTO default.tbl (key,
value) VALUES (?, 'txt')", ConsistencyLevel.ONE, i);
- }
+ @Test
+ public void testSameTimestampOnEachTableOfSnaphot()
+ {
+ cluster.get(1).nodetoolResult("snapshot", "-t",
"sametimestamp").asserts().success();
+ NodeToolResult result = cluster.get(1).nodetoolResult("listsnapshots");
+
+ int distinctDates = Arrays.stream(result.getStdout().split("\n"))
Review comment:
nit: `distinctDates` -> `distinctTimestamps`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
