[GitHub] [kafka] vcrfxia commented on a diff in pull request #13186: KAFKA-14491: [4/N] Improvements to segment value format for RocksDB versioned store

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

vcrfxia commented on code in PR #13186:
URL: https://github.com/apache/kafka/pull/13186#discussion_r1093920852


##########
streams/src/main/java/org/apache/kafka/streams/state/internals/RocksDBVersionedStoreSegmentValueFormatter.java:
##########
@@ -146,41 +164,39 @@ interface SegmentValue {
         void insertAsLatest(long validFrom, long validTo, byte[] value);
 
         /**
-         * Inserts the provided record into the segment as the earliest record in the segment.
-         * This operation is allowed even if the segment is degenerate. It is the caller's responsibility
-         * to ensure that this action is valid, i.e., that record's (validFrom) timestamp is smaller
-         * than the current {@code minTimestamp} of the segment.
+         * Inserts the provided record into the segment row as the earliest record in the row.
+         * It is the caller's responsibility to ensure that this action is valid, i.e.,
+         * that record's (validFrom) timestamp is smaller than the current {@code minTimestamp}
+         * of the segment row.
          *
          * @param timestamp the (validFrom) timestamp of the record to insert
          * @param value the value of the record to insert
          */
         void insertAsEarliest(long timestamp, byte[] value);
 
         /**
-         * Inserts the provided record into the segment at the provided index. This operation
-         * requires that the segment is not degenerate, and that
-         * {@link SegmentValue#find(long, boolean)} has already been called in order to deserialize
-         * the relevant index (to insert into index n requires that index n-1 has already been deserialized).
+         * Inserts the provided record into the segment row at the provided index. This operation
+         * requires that {@link SegmentValue#find(long, boolean)} has already been called in order to deserialize
+         * the relevant index (to insert into index n requires that index n has already been deserialized).

Review Comment:
   The requirement change from n-1 to n is because using n-1 means callers could attempt to call this method on a degenerate segment, which would not be valid. In practice, callers never have reason to call this method with n-1; only the internal implementation of `insertAsLatest()` has reason to do this, but this method calls the helper method `doInsert()` directly instead. 
   
   By updating this requirement from n-1 to n, it ensures that callers will never be allowed to call this method on a degenerate segment, and that note can be removed from the javadocs as a result.



-- 
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: jira-unsubscribe@kafka.apache.org

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