[GitHub] [cassandra] pkolaczk commented on a diff in pull request #2460: Cassandra 18490 Fix checksum validation and checksum validate indexes streaming and SSTable import

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

pkolaczk commented on code in PR #2460:
URL: https://github.com/apache/cassandra/pull/2460#discussion_r1253906519


##########
src/java/org/apache/cassandra/index/sai/disk/io/IndexFileUtils.java:
##########
@@ -77,7 +77,7 @@ public IndexInput openBlockingInput(File file)
 
     public interface ChecksumWriter
     {
-        long getChecksum();
+        long getChecksum() throws IOException;
     }
 
     static class IncrementalChecksumSequentialWriter extends SequentialWriter implements ChecksumWriter

Review Comment:
   > Perhaps we could address the flushData() duplication by having a SequentialWriter subclass w/ an abstract method that updates the checksum in a subclass-appropriate way.
   
   I don't agree with that. Subclassing, especially subclassing with implementation inheritance / overrides, introduces more complexity and causes way more pain than a few duplicated lines of code. In this particular case, there were several issues caused by `writeXXX` overrides which is a textbook example of inheritance gone bad. If I'm going to do a refactoring, it would be reducing the amount of inheritance, not adding more of it ;)
   
   `ChecksummedSequentialWriter` looks like a class that has too many responsibilities. It computes the checksum *and* writes checksum to a separate component. It looks what we really want is to split those responsibilities, and really have a separate `ChecksummedSequentialWriter` that does only the checksummng part, and a separate optional component/function responsible for writing the checksum to a file. If I refactor code like that, then I could get rid of duplication, because we'd have only one checksumming writer (and no subclasses). 



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