adelapena commented on code in PR #1891:
URL: https://github.com/apache/cassandra/pull/1891#discussion_r1185992950
##########
src/java/org/apache/cassandra/db/rows/Cell.java:
##########
@@ -59,16 +70,37 @@
public interface Factory<V>
{
- Cell<V> create(ColumnMetadata column, long timestamp, int ttl, int
localDeletionTime, V value, CellPath path);
+ Cell<V> create(ColumnMetadata column, long timestamp, int ttl, long
localDeletionTime, V value, CellPath path);
}
protected Cell(ColumnMetadata column)
{
super(column);
}
+ public static int deletionTimeLongToUnsignedInteger(long deletionTime)
+ {
+ return deletionTime == NO_DELETION_TIME ?
NO_DELETION_TIME_UNSIGNED_INTEGER : CassandraUInt.fromLong(deletionTime);
+ }
+
+ public static long deletionTimeUnsignedIntegerToLong(int
deletionTimeUnsignedInteger)
+ {
+ return deletionTimeUnsignedInteger ==
NO_DELETION_TIME_UNSIGNED_INTEGER ? NO_DELETION_TIME :
CassandraUInt.toLong(deletionTimeUnsignedInteger);
+ }
+
+ public static long getVersionedMaxDeletiontionTime()
+ {
+ if (BigFormat.ttlMode.equals(TTL_MODE.EXTENDED_TTL_2106))
+ // The whole cluster is 2016, we're out of the 2038/2106 mixed
clusters scenario. Short cut to avoid the 'minClusterVersion' volatile read
Review Comment:
Does the distinction between `COMPATIBILITY` and `EXTENDED_TTL_2106` exist
only to avoid the cost of reading a `volatile` while checking the cluster
version? I doubt that reading yet another `volatile` is going to have a lot of
impact.
If the problem is also not trusting the cluster version, I think that the
situation is no different to what happens during any other operation done
during a rolling upgrade, independently of the extended expiration dates. IMO
that problem should be addressed by CASSANDRA-18301, and here we can assume
that announcing the specified messaging version is enough. I mean, setting the
messaging version with the property makes this identical to any other operation
during a rolling upgrade.
So I think the property could be a boolean. Going a step beyond, the
property could set the major Cassandra version we want to stay compatible with.
That would determine the sstable, commitlog, hints and messaging versions, as
well as the availability of the extended TTLs. That is quite easy to do given
the excellent state of this patch. It has the advantage of making things a bit
easier for CASSANDRA-18301. It also breaks the odd dependency on `BigFormat`
that the cells, messaging, commitlog and hints currently have.
I gave it a go in this commit:
https://github.com/adelapena/cassandra/commit/0a87f1ee13e5eed92dee637638667a30695c3426
It replaces the `cassandra.sstable.format.ttl_mode` enum property by a more
generic `cassandra.storage_compatibility_version` int property. That property
is meant to determine the entire adoption of not-backwards compatible features.
As for the cost of checking the `volatile` in
`Cell.getVersionedMaxDeletiontionTime`, I think we can do an optimization to
reduce its cost in the common case. Most of the callers of that method are
interested only in checking is a certain timestamp exceeds it. And most of the
checked timestamps will be, I assume, under both the 2038 and 2106 limit. So we
can add a `Cell.exceedsVersionMaxDeletionTime` method that returns `false` if
the value is lesser than both caps without checking the cluster version. This
way:
https://github.com/adelapena/cassandra/commit/ae0941014bf32cfd471bc5b8d57fb27b4365b959
--
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]
