[jira] [Commented] (CASSANDRA-16789) Add TTL support to nodetool snapshots
[
https://issues.apache.org/jira/browse/CASSANDRA-16789?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17419261#comment-17419261
]
Ekaterina Dimitrova commented on CASSANDRA-16789:
-
Thank you [~paulo]. I didn't spend time on this ticket so IMHO it won't be fair
to be marked as a reviewer. I removed myself.
I personally think at this point just adding a note on the ticket is enough.
{quote}We didn't have any intention to claim authorship on this class since
that work would be eventually included in the context of CASSANDRA-15234 so its
authorship was well-known in my view, but I agree it should have been given
proper credit anyway.
{quote}
Exactly, it took me one minute to recognize it and that's why we need to be
careful as not everyone knows the history and have our understanding that
eventually it will be in and it doesn't make any sense to write different code
from scratch. Please, feel free to add also the unit tests as I think they
might prevent regressions as the framework is still not incorporated and things
are still hectic around the units.
Also, I want to encourage you to join the ML discussion. I plan to summarize
the outcome next week so there is still some time if you have any thoughts to
share.
> Add TTL support to nodetool snapshots
> -
>
> Key: CASSANDRA-16789
> URL: https://issues.apache.org/jira/browse/CASSANDRA-16789
> Project: Cassandra
> Issue Type: Sub-task
> Components: Tool/nodetool
>Reporter: Paulo Motta
>Assignee: Abuli Palagashvili
>Priority: Normal
> Fix For: 4.1
>
> Time Spent: 40m
> Remaining Estimate: 0h
>
> Add new parameter {{--ttl}} to {{nodetool snapshot}} command. This parameter
> can be specified in human readable duration (ie. 30mins, 1h, 300d) and should
> not be lower than 1 minute.
> The expiration date should be added to the snapshot manifest in ISO format.
> A periodic thread should efficiently scan snapshots and automatically clear
> those past expiration date. The periodicity of the scan thread should be 1
> minute by default but be overridable via a system property.
> The command {{nodetool listsnapshots}} should display the expiration date
> when the snapshot contains a TTL.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-16789) Add TTL support to nodetool snapshots
[
https://issues.apache.org/jira/browse/CASSANDRA-16789?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17419187#comment-17419187
]
Paulo Motta commented on CASSANDRA-16789:
-
Given we needed a way to parse Durations during this work I suggested Abi to
reuse the already-implemented {{Duration}} class from CASSANDRA-15234 during
our discussons on the #cassandra-gsoc channel but forgot to mention this on
this ticket and give proper authorship. We didn't have any intention to claim
authorship on this class since that work would be eventually included in the
context of CASSANDRA-15234 so its authorship was notorious in my view, but I
agree it should have been given proper credit anyway. Sorry about this! Since
there's no way to change a commit message after the fact I will set
[~e.dimitrova] as reviewer of this ticket, but please let me know if there's a
better way to remediate this.
> Add TTL support to nodetool snapshots
> -
>
> Key: CASSANDRA-16789
> URL: https://issues.apache.org/jira/browse/CASSANDRA-16789
> Project: Cassandra
> Issue Type: Sub-task
> Components: Tool/nodetool
>Reporter: Paulo Motta
>Assignee: Abuli Palagashvili
>Priority: Normal
> Fix For: 4.1
>
> Time Spent: 40m
> Remaining Estimate: 0h
>
> Add new parameter {{--ttl}} to {{nodetool snapshot}} command. This parameter
> can be specified in human readable duration (ie. 30mins, 1h, 300d) and should
> not be lower than 1 minute.
> The expiration date should be added to the snapshot manifest in ISO format.
> A periodic thread should efficiently scan snapshots and automatically clear
> those past expiration date. The periodicity of the scan thread should be 1
> minute by default but be overridable via a system property.
> The command {{nodetool listsnapshots}} should display the expiration date
> when the snapshot contains a TTL.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-16789) Add TTL support to nodetool snapshots
[
https://issues.apache.org/jira/browse/CASSANDRA-16789?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17419177#comment-17419177
]
Andres de la Peña commented on CASSANDRA-16789:
---
{quote}If you have told me you are porting the Duration class from
CASSANDRA-15234, I might have told you that I also wrote a few unit tests for
it so you can add them too
Also, there is ongoing discussion on the ML around that work, considering your
interest to my code you might want to take a part in that discussion too.
{quote}
I think that [~e.dimitrova]'s authorship of that {{Duration}} class should have
been mentioned, maybe with a co-author entry in the commit message.
> Add TTL support to nodetool snapshots
> -
>
> Key: CASSANDRA-16789
> URL: https://issues.apache.org/jira/browse/CASSANDRA-16789
> Project: Cassandra
> Issue Type: Sub-task
> Components: Tool/nodetool
>Reporter: Paulo Motta
>Assignee: Abuli Palagashvili
>Priority: Normal
> Fix For: 4.1
>
> Time Spent: 40m
> Remaining Estimate: 0h
>
> Add new parameter {{--ttl}} to {{nodetool snapshot}} command. This parameter
> can be specified in human readable duration (ie. 30mins, 1h, 300d) and should
> not be lower than 1 minute.
> The expiration date should be added to the snapshot manifest in ISO format.
> A periodic thread should efficiently scan snapshots and automatically clear
> those past expiration date. The periodicity of the scan thread should be 1
> minute by default but be overridable via a system property.
> The command {{nodetool listsnapshots}} should display the expiration date
> when the snapshot contains a TTL.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-16789) Add TTL support to nodetool snapshots
[
https://issues.apache.org/jira/browse/CASSANDRA-16789?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17418911#comment-17418911
]
Ekaterina Dimitrova commented on CASSANDRA-16789:
-
If you have told me you are porting the Duration class from CASSANDRA-15234, I
might have told you that I also wrote a few unit tests for it so you can add
them too ;)
Also, there is ongoing discussion on the ML around that work, considering your
interest to my code you might want to take a part in that discussion too.
> Add TTL support to nodetool snapshots
> -
>
> Key: CASSANDRA-16789
> URL: https://issues.apache.org/jira/browse/CASSANDRA-16789
> Project: Cassandra
> Issue Type: Sub-task
> Components: Tool/nodetool
>Reporter: Paulo Motta
>Assignee: Abuli Palagashvili
>Priority: Normal
> Fix For: 4.1
>
> Time Spent: 40m
> Remaining Estimate: 0h
>
> Add new parameter {{--ttl}} to {{nodetool snapshot}} command. This parameter
> can be specified in human readable duration (ie. 30mins, 1h, 300d) and should
> not be lower than 1 minute.
> The expiration date should be added to the snapshot manifest in ISO format.
> A periodic thread should efficiently scan snapshots and automatically clear
> those past expiration date. The periodicity of the scan thread should be 1
> minute by default but be overridable via a system property.
> The command {{nodetool listsnapshots}} should display the expiration date
> when the snapshot contains a TTL.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-16789) Add TTL support to nodetool snapshots
[
https://issues.apache.org/jira/browse/CASSANDRA-16789?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17405402#comment-17405402
]
Paulo Motta commented on CASSANDRA-16789:
-
Adding last successful test link:
https://ci-cassandra.apache.org/view/patches/job/Cassandra-devbranch/1059/
LGTM, great job!
Created documentation ticket on CASSANDRA-16887 since this depends on
CASSANDRA-16761.
Committed as
[ad249424814836bd00f47931258ad58bfefb24fd|https://github.com/apache/cassandra/commit/ad249424814836bd00f47931258ad58bfefb24fd]
to trunk.
> Add TTL support to nodetool snapshots
> -
>
> Key: CASSANDRA-16789
> URL: https://issues.apache.org/jira/browse/CASSANDRA-16789
> Project: Cassandra
> Issue Type: Sub-task
> Components: Tool/nodetool
>Reporter: Paulo Motta
>Assignee: Abuli Palagashvili
>Priority: Normal
> Fix For: 4.1
>
> Time Spent: 40m
> Remaining Estimate: 0h
>
> Add new parameter {{--ttl}} to {{nodetool snapshot}} command. This parameter
> can be specified in human readable duration (ie. 30mins, 1h, 300d) and should
> not be lower than 1 minute.
> The expiration date should be added to the snapshot manifest in ISO format.
> A periodic thread should efficiently scan snapshots and automatically clear
> those past expiration date. The periodicity of the scan thread should be 1
> minute by default but be overridable via a system property.
> The command {{nodetool listsnapshots}} should display the expiration date
> when the snapshot contains a TTL.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-16789) Add TTL support to nodetool snapshots
[
https://issues.apache.org/jira/browse/CASSANDRA-16789?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17404482#comment-17404482
]
Stefan Miklosovic commented on CASSANDRA-16789:
---
+1
> Add TTL support to nodetool snapshots
> -
>
> Key: CASSANDRA-16789
> URL: https://issues.apache.org/jira/browse/CASSANDRA-16789
> Project: Cassandra
> Issue Type: Sub-task
> Components: Tool/nodetool
>Reporter: Paulo Motta
>Assignee: Abuli Palagashvili
>Priority: Normal
> Fix For: 4.1
>
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> Add new parameter {{--ttl}} to {{nodetool snapshot}} command. This parameter
> can be specified in human readable duration (ie. 30mins, 1h, 300d) and should
> not be lower than 1 minute.
> The expiration date should be added to the snapshot manifest in ISO format.
> A periodic thread should efficiently scan snapshots and automatically clear
> those past expiration date. The periodicity of the scan thread should be 1
> minute by default but be overridable via a system property.
> The command {{nodetool listsnapshots}} should display the expiration date
> when the snapshot contains a TTL.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-16789) Add TTL support to nodetool snapshots
[
https://issues.apache.org/jira/browse/CASSANDRA-16789?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17401572#comment-17401572
]
Abuli Palagashvili commented on CASSANDRA-16789:
[Main PR|https://github.com/apache/cassandra/pull/1046]
[Website|https://github.com/apache/cassandra-website/pull/69]
> Add TTL support to nodetool snapshots
> -
>
> Key: CASSANDRA-16789
> URL: https://issues.apache.org/jira/browse/CASSANDRA-16789
> Project: Cassandra
> Issue Type: Sub-task
> Components: Tool/nodetool
>Reporter: Paulo Motta
>Assignee: Abuli Palagashvili
>Priority: Normal
>
> Add new parameter {{--ttl}} to {{nodetool snapshot}} command. This parameter
> can be specified in human readable duration (ie. 30mins, 1h, 300d) and should
> not be lower than 1 minute.
> The expiration date should be added to the snapshot manifest in ISO format.
> A periodic thread should efficiently scan snapshots and automatically clear
> those past expiration date. The periodicity of the scan thread should be 1
> minute by default but be overridable via a system property.
> The command {{nodetool listsnapshots}} should display the expiration date
> when the snapshot contains a TTL.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-16789) Add TTL support to nodetool snapshots
[
https://issues.apache.org/jira/browse/CASSANDRA-16789?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17401570#comment-17401570
]
Abuli Palagashvili commented on CASSANDRA-16789:
Patch is almost ready.We've added new entities {{TableSnapshot}} and
{{SnapshotManifest}}.Now all information about snapshots (including creation
and expiration times) is stored into this objects.We've also added
{{SnapshotManager}} class for tracking, indexing and removing of expiring
snapshots.We made it a part of {{StorageService}}.Cleanup process is
implemented as regularly run task.Cassandra operator can specify period between
launches.We've also added columns {{CreatedAt}} and {{ExpiresAt}} columns to
{{listsnapshots}} nodetool command output.
> Add TTL support to nodetool snapshots
> -
>
> Key: CASSANDRA-16789
> URL: https://issues.apache.org/jira/browse/CASSANDRA-16789
> Project: Cassandra
> Issue Type: Sub-task
> Components: Tool/nodetool
>Reporter: Paulo Motta
>Assignee: Abuli Palagashvili
>Priority: Normal
>
> Add new parameter {{--ttl}} to {{nodetool snapshot}} command. This parameter
> can be specified in human readable duration (ie. 30mins, 1h, 300d) and should
> not be lower than 1 minute.
> The expiration date should be added to the snapshot manifest in ISO format.
> A periodic thread should efficiently scan snapshots and automatically clear
> those past expiration date. The periodicity of the scan thread should be 1
> minute by default but be overridable via a system property.
> The command {{nodetool listsnapshots}} should display the expiration date
> when the snapshot contains a TTL.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-16789) Add TTL support to nodetool snapshots
[
https://issues.apache.org/jira/browse/CASSANDRA-16789?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17378509#comment-17378509
]
Abuli Palagashvili commented on CASSANDRA-16789:
Okay, first of all let's exclude SnapshotSizeDetails into separate class and
make it field of SnapshotDetails.After that we modify
Directories.getSnapshotDetails
> Add TTL support to nodetool snapshots
> -
>
> Key: CASSANDRA-16789
> URL: https://issues.apache.org/jira/browse/CASSANDRA-16789
> Project: Cassandra
> Issue Type: Sub-task
> Components: Tool/nodetool
>Reporter: Paulo Motta
>Assignee: Abuli Palagashvili
>Priority: Normal
>
> Add new parameter {{--ttl}} to {{nodetool snapshot}} command. This parameter
> can be specified in human readable duration (ie. 30mins, 1h, 300d) and should
> not be lower than 1 minute.
> The expiration date should be added to the snapshot manifest in ISO format.
> A periodic thread should efficiently scan snapshots and automatically clear
> those past expiration date. The periodicity of the scan thread should be 1
> minute by default but be overridable via a system property.
> The command {{nodetool listsnapshots}} should display the expiration date
> when the snapshot contains a TTL.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-16789) Add TTL support to nodetool snapshots
[
https://issues.apache.org/jira/browse/CASSANDRA-16789?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17378026#comment-17378026
]
Paulo Motta commented on CASSANDRA-16789:
-
Thanks for your patch update, looking good so far! Added a few comments to the
PR itself and below you can find some design comments:
I noticed the manifest file is being read in two places, which violates the
don't repeat yourself principle:
* [SnapshotDetails
constructor|https://github.com/apache/cassandra/pull/1046/files#diff-ad15ce2b3f9473fd99ec3f72362497ec43599c63b8f71b509e338677eeaca41aR40]
*
[SnapshotDetailsTabularData.from|https://github.com/apache/cassandra/pull/1046/files#diff-76d6ca8149019cb3e777d971489de8ba6c20d6caee2718f638ad044f7fddc3c9R87]
Another potential issue I spotted is that
{{Directories.getSnapshotManifestFile(snapshotName)}} is being used to find the
snapshot manifest location. As [~stefan.miklosovic] mentioned in an earlier
discussion, this method is only used to write snapshots, since it will pick one
of many data directories to write the manifest file. Using this same method for
reading can be a problem when using many data directories because a different
directory can be chosen from the one the manifest was written to, so the
manifest would not be found.
I think the best place to read the manifest is on
{{Directories.getSnapshotDetails}}, since this will already go through all the
data directories and you can try to find the manifest in all data directories.
We should probably merge the class {{SnapshotSizeDetails}} into
{{SnapshotDetails}} and use this single class to represent information about a
table snapshot, and this method would return {{SnapshotDetails}} instead of
{{SnapshotSizeDetails}}.
Please let me know if this makes sense to you or if you have any questions or
concerns.
> Add TTL support to nodetool snapshots
> -
>
> Key: CASSANDRA-16789
> URL: https://issues.apache.org/jira/browse/CASSANDRA-16789
> Project: Cassandra
> Issue Type: Sub-task
> Components: Tool/nodetool
>Reporter: Paulo Motta
>Assignee: Abuli Palagashvili
>Priority: Normal
>
> Add new parameter {{--ttl}} to {{nodetool snapshot}} command. This parameter
> can be specified in human readable duration (ie. 30mins, 1h, 300d) and should
> not be lower than 1 minute.
> The expiration date should be added to the snapshot manifest in ISO format.
> A periodic thread should efficiently scan snapshots and automatically clear
> those past expiration date. The periodicity of the scan thread should be 1
> minute by default but be overridable via a system property.
> The command {{nodetool listsnapshots}} should display the expiration date
> when the snapshot contains a TTL.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-16789) Add TTL support to nodetool snapshots
[
https://issues.apache.org/jira/browse/CASSANDRA-16789?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17378004#comment-17378004
]
Abuli Palagashvili commented on CASSANDRA-16789:
Nice, I've updated PR, according to your comment.It remains to add new tests.
> Add TTL support to nodetool snapshots
> -
>
> Key: CASSANDRA-16789
> URL: https://issues.apache.org/jira/browse/CASSANDRA-16789
> Project: Cassandra
> Issue Type: Sub-task
> Components: Tool/nodetool
>Reporter: Paulo Motta
>Assignee: Abuli Palagashvili
>Priority: Normal
>
> Add new parameter {{--ttl}} to {{nodetool snapshot}} command. This parameter
> can be specified in human readable duration (ie. 30mins, 1h, 300d) and should
> not be lower than 1 minute.
> The expiration date should be added to the snapshot manifest in ISO format.
> A periodic thread should efficiently scan snapshots and automatically clear
> those past expiration date. The periodicity of the scan thread should be 1
> minute by default but be overridable via a system property.
> The command {{nodetool listsnapshots}} should display the expiration date
> when the snapshot contains a TTL.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-16789) Add TTL support to nodetool snapshots
[
https://issues.apache.org/jira/browse/CASSANDRA-16789?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17376686#comment-17376686
]
Paulo Motta commented on CASSANDRA-16789:
-
Hi Abi,
Your initial working POC from
[PR#1046|https://github.com/apache/cassandra/pull/1046] looks great so far,
good job!
As we discussed in our last call let's make the following changes in order to
make this production-ready:
* Use Instant instead of String on {{SnapshotDetails.createdAt}} and
{{SnapshotDetails.expiresAt}}
* Rename {{SnapshotDetails}} -> {{SnapshotManifest}}, since a single keyspace
snapshot can have contain multiple table manifests
* Create new inner class {{SnapshotDetails}} on {{SnapshotManager}}, which
represents a set of {{SnapshotManifest}} of a given keyspace with the same name.
* Update SnapshotManager to use the following structure
{code:java}
class SnapshotManager {
ScheduledFuture cleanupTaskFuture;
Map activeSnapshots;
void start() {
populateActiveSnapshots();
cleanupTaskFuture =
scheduleWithFixedDelay(this::clearExpiredSnapshots, 0, 1, TimeUnit.MINUTES);
}
void addSnapshot(String name, SnapshotManifest manifest) {
// Create new SnapshotDetails if it's a new snapshot, otherwise add to
existing
}
void populateActiveSnapshots(){
// Read snapshots from disk and add it to activeSnapshots
}
void clearExpiredSnapshots() {
for (SnapshotDetails snapshot : activeSnapshots.values()) {
if (snapshot.isExpired()) {
logger.info("Clearing expired snapshot {}", snapshot);
snapshot.clear();
}
}
}
}
{code}
* Add unit test for each method of SnapshotManager on different scenarios
* Make {{ColumnFamilyStore.snapshotWithoutFlush}} add newly created snapshot
manifests to {{SnapshotManager}} (via {{SnapshotManager.addSnapshot}}
* Add dtest with the following structure:
** create snapshot with 1 minute TTL
** stop node
** start node
** Wait *up to* 1 minute for snapshot expiration
** Check snapshot is removed from disk
> Add TTL support to nodetool snapshots
> -
>
> Key: CASSANDRA-16789
> URL: https://issues.apache.org/jira/browse/CASSANDRA-16789
> Project: Cassandra
> Issue Type: Sub-task
> Components: Tool/nodetool
>Reporter: Paulo Motta
>Assignee: Abuli Palagashvili
>Priority: Normal
>
> Add new parameter {{--ttl}} to {{nodetool snapshot}} command. This parameter
> can be specified in human readable duration (ie. 30mins, 1h, 300d) and should
> not be lower than 1 minute.
> The expiration date should be added to the snapshot manifest in ISO format.
> A periodic thread should efficiently scan snapshots and automatically clear
> those past expiration date. The periodicity of the scan thread should be 1
> minute by default but be overridable via a system property.
> The command {{nodetool listsnapshots}} should display the expiration date
> when the snapshot contains a TTL.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
-
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
