yifan-c commented on code in PR #4107:
URL: https://github.com/apache/cassandra/pull/4107#discussion_r2069539662


##########
src/java/org/apache/cassandra/io/sstable/SSTableZeroCopyWriter.java:
##########
@@ -233,4 +237,22 @@ private void write(AsyncStreamingInputPlus in, long size, 
SequentialWriter write
             throw new FSWriteError(e, writer.getPath());
         }
     }
+
+    private static class ZeroCopySequentialWriter extends SequentialWriter
+    {
+        private ZeroCopySequentialWriter(File file, SequentialWriterOption 
option, boolean strictFlushing)
+        {
+            super(file, ByteBufferUtil.EMPTY_BYTE_BUFFER, option, 
strictFlushing);
+        }
+
+//        @Override
+//        public void write(byte[] b, int off, int len) throws IOException
+//        {
+//            // This logic is never valid in production environments and is 
only here for unit tests.  By default we never
+//            // allocate this buffer, but in unit tests we need one, so 
allocate to make those unit tests happy.

Review Comment:
   Maybe rephrase and move this comment up to the method. In production, we do 
not expect this entire method to be called, as only `writeDirectlyToChannel` 
should be invoked for zero-copy



##########
test/distributed/org/apache/cassandra/distributed/test/cql3/SingleNodeTableWalkTest.java:
##########
@@ -371,10 +372,13 @@ public void test() throws IOException
                                                                         
.add(this::selectTokenRange))
                                   .addIf(State::hasEnoughMemtable, 
StatefulASTBase::flushTable)
                                   .addIf(State::hasEnoughSSTables, 
StatefulASTBase::compactTable)
+                                  .addAllIf(BaseState::allowRepair, b -> 
b.add(StatefulASTBase::incrementalRepair)
+                                                                          
.add(StatefulASTBase::previewRepair))

Review Comment:
   This comment is unrelated to the patch. It might look cleaner if allow 
composing multiple commands into one, and use `addIf` only. `addAllIf` is not 
descriptive. 



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

Reply via email to