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 or from abstract base classes,
introduces more complexity and often 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 subclassing,
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: [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]
