[GitHub] [cassandra] adelapena commented on a diff in pull request #1891: Cassandra 14227 trunk

["adelapena (via GitHub)" <gi...@apache.org>]

adelapena commented on code in PR #1891:
URL: https://github.com/apache/cassandra/pull/1891#discussion_r1171475059


##########
src/java/org/apache/cassandra/io/sstable/metadata/StatsMetadata.java:
##########
@@ -307,8 +310,14 @@ public int serializedSize(Version version, StatsMetadata component) throws IOExc
             size += EstimatedHistogram.serializer.serializedSize(component.estimatedPartitionSize);
             size += EstimatedHistogram.serializer.serializedSize(component.estimatedCellPerPartitionCount);
             size += CommitLogPosition.serializer.serializedSize(component.commitLogIntervals.upperBound().orElse(CommitLogPosition.NONE));
-            size += 8 + 8 + 4 + 4 + 4 + 4 + 8 + 8; // mix/max timestamp(long), min/maxLocalDeletionTime(int), min/max TTL, compressionRatio(double), repairedAt (long)
-            size += TombstoneHistogram.serializer.serializedSize(component.estimatedTombstoneDropTime);
+            size += 8 + 8; // mix/max timestamp(long)
+            size += 4 + 4;   //min/maxLocalDeletionTime(either int or uint)
+            size +=  4 + 4 + 8 + 8;// min/max TTL, compressionRatio(double), repairedAt (long)
+            if (version.hasUIntDeletionTime())
+                size += TombstoneHistogram.serializer.serializedSize(component.estimatedTombstoneDropTime);
+            else
+                size += TombstoneHistogram.legacySerializer.serializedSize(component.estimatedTombstoneDropTime);

Review Comment:
   There are three similar if-else block on this class. They select the proper serializer according to the sstable version and then both if-else branches perform the same operation for the selected serializer. 
   
   I think we could get rid of the repeated if-else blocks if we had a single reusable method to get the serializer for the current sstable version. Also, the serialization tests on `StreamingTombstoneHistogramBuilderTest` should test both serializers. For example, [this way](https://github.com/adelapena/cassandra/commit/13a7a802ebc31142a5221d3f87e39d23b1d651b3).



-- 
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