[GitHub] [cassandra] bereng commented on a diff in pull request #2051: Cassandra 18118 3.11

[GitBox <gi...@apache.org>]

bereng commented on code in PR #2051:
URL: https://github.com/apache/cassandra/pull/2051#discussion_r1064314758


##########
src/java/org/apache/cassandra/db/Memtable.java:
##########
@@ -387,9 +397,16 @@ public Partition getPartition(DecoratedKey key)
         return partitions.get(key);
     }
 
+    /**
+     * Returns the minTS if one available, otherwise -1.
+     *
+     * EncodingStats uses a synthetic epoch TS at 2015. We don't want to leak that (CASSANDRA-18118) so we return -1 instead.
+     *
+     * @return The minTS or -1 if none available
+     */
     public long getMinTimestamp()
     {
-        return minTimestamp;
+        return minTimestamp != EncodingStats.NO_STATS.minTimestamp ? minTimestamp : -1;

Review Comment:
   Icwym but a TS in 2015, while debugging or if it got logged somewhere, reads to the eyes just like any other valid TS. A `-1` is clearly an 'out of band' value and it would have cut my debugging time by 10!. It would have immediately triggered me to look and identified the problem and I want to spare that pain to any future developers. Also, historically, `-1` has been used widely in returns values to signal similar things imo. It's a 'better' arbitrary value if such a thing exists imo.
   
   Having said that, I also considered other alternatives such as a `checkTSisValid()` method just to raise awareness in the API about the 2015 TS but this being a hot path, I didn't want to add an extra method call.
   
   Given my experience debugging this little devil and trying to prevent that pain to future devs, I can only think of the `-1` or even raise an exception to make it even more obvious. If you think about it, the nasty tricky bugs that can arise from this little TS may even grant an exception. Wdyt?



-- 
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: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org