-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add optional default TTL to Cassandra clustering #9221
base: main
Are you sure you want to change the base?
Conversation
@dotnet-policy-service agree company="Microsoft" |
PRIMARY KEY(partition_key, address, port, generation) | ||
) | ||
WITH compression = { 'class' : 'LZ4Compressor', 'enabled' : true } | ||
AND default_time_to_live = {{defaultTimeToLiveSeconds}}; |
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.
What's the Cassandra behavior if there's an existing table (like from before this PR) but it doesn't have the default TTL configured?
Does it
- not do anything to the table
- update the table's default TTL for new rows (but leave the existing rows without a TTL set)
- something else
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.
It will not do anything to the table
test/Extensions/Tester.Cassandra/Clustering/CassandraClusteringTableTests.cs
Outdated
Show resolved
Hide resolved
If we go with this implementation then it may be good to have general guidance on how other providers can implement this functionality. It's more dev related than user so not sure if those docs would go in the following place: https://github.com/dotnet/docs/tree/main/docs/orleans/implementation or here in dotnet/orleans somewhere. |
src/Cassandra/Orleans.Clustering.Cassandra/Hosting/CassandraClusteringOptions.cs
Outdated
Show resolved
Hide resolved
I had some additional discussions with @rkargMsft and we realized that this table-level default TTL is still cell-based and does not update the entire row's TTL when a single cell (e.g. the IAmAliveTime) is updated. I will be updating this with a different approach and adding more tests :) |
560ca99
to
f754410
Compare
src/Cassandra/Orleans.Clustering.Cassandra/Hosting/CassandraClusteringOptions.cs
Show resolved
Hide resolved
This is an initial implementation that will work for newly-created membership tables. It does not attempt to address updating existing tables or rows, which will be left unchanged. See https://docs.datastax.com/en/cql-oss/3.3/cql/cql_reference/cqlCreateTable.html#tabProp__cqlTableDefaultTTL for documentation on `default_time_to_live` in Cassandra table creation.
f754410
to
8fca72c
Compare
public bool UseCassandraTtl { get; set; } | ||
|
||
internal int? GetCassandraTtlSeconds(ClusterMembershipOptions clusterMembershipOptions) => | ||
UseCassandraTtl && clusterMembershipOptions.DefunctSiloCleanupPeriod.HasValue |
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.
Do we need the check for the CleanupPeriod value here? Presumably it would be valid to request the Cassandra TTL mechanism and not the periodic task.
This is an initial implementation of #9164 that will work for newly-created membership tables. It does not attempt to address updating existing tables or rows, which will be left unchanged.
See https://docs.datastax.com/en/cql-oss/3.3/cql/cql_reference/cqlCreateTable.html#tabProp__cqlTableDefaultTTL for documentation on
default_time_to_live
in Cassandra table creation.Microsoft Reviewers: Open in CodeFlow