[GitHub] [cassandra] henrikingo commented on a diff in pull request #2284: CASSANDRA-18260 Add details to Error: Not enough space for compaction

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

henrikingo commented on code in PR #2284:
URL: https://github.com/apache/cassandra/pull/2284#discussion_r1172687643


##########
test/unit/org/apache/cassandra/db/DirectoriesTest.java:
##########
@@ -696,4 +732,144 @@ private List<Directories.DataDirectoryCandidate> getWriteableDirectories(DataDir
         return candidates;
     }
 
+    private int getDiyThreadId()
+    {
+        return myDiyId = diyThreadId.getAndIncrement();
+    }
+
+    private void detachLogger()
+    {
+        logger.detachAppender(listAppender);
+        MDC.remove(this.MDCID);
+    }
+
+    private void tailLogs()
+    {
+        int diyId = getDiyThreadId();
+        MDC.put(this.MDCID, String.valueOf(diyId));
+        logger = (Logger) LoggerFactory.getLogger(Directories.class);
+
+        // create and start a ListAppender
+        listAppender = new ListAppender<>();
+        listAppender.start();
+
+        // add the appender to the logger
+        logger.addAppender(listAppender);
+    }
+
+    private List<ILoggingEvent> filterLogByDiyId(List<ILoggingEvent> log)
+    {
+        ArrayList<ILoggingEvent> filteredLog = new ArrayList<>();
+        for(ILoggingEvent event : log)
+        {
+            int mdcId = Integer.parseInt(event.getMDCPropertyMap().get(this.MDCID));
+            if(mdcId == myDiyId){
+                filteredLog.add(event);
+            }
+        }
+        return filteredLog;
+    }
+
+    private void checkFormattedMessage(List<ILoggingEvent> log, Level expectedLevel, String expectedMessage, int expectedCount)
+    {
+        int found=0;
+        for(ILoggingEvent e: log)
+        {
+            System.err.println(e.getFormattedMessage());
+            if(e.getFormattedMessage().endsWith(expectedMessage))
+            {
+                if (e.getLevel() == expectedLevel)
+                    found++;
+            }
+        }
+
+        assertEquals(expectedCount, found);
+    }
+
+    @Test
+    public void testHasAvailableDiskSpace()
+    {
+        DataDirectory[] dataDirectories = new DataDirectory[]

Review Comment:
   Yes. Thanks for saying this, I was hoping to discuss alternatives.
   
   Your proposal will even make it easier to write and maintain these tests. The reason I didn't do that already is that it's unclear to me how this will affect the triaging of CI results. If you have a unit test like:
   
       for ( value : hundredValues)
           assertEquals(value, funtionToTest());
   
   Now, if you get an assertion, how will you know which of the 100 values is the wrong one? JUnite will just show you the line number of that line, but the failure could be any of the 100 values we are looping over.
   
   If you could suggest  a way to do that and also maintain great usability for those who run and ultimately have to fix it() anyway.



##########
src/java/org/apache/cassandra/db/Directories.java:
##########
@@ -485,18 +485,39 @@ public boolean hasAvailableDiskSpace(long estimatedSSTables, long expectedTotalW
     {
         long writeSize = expectedTotalWriteSize / estimatedSSTables;
         long totalAvailable = 0L;
+        boolean hasSpace = true;
 
         for (DataDirectory dataDir : paths)
         {
             if (DisallowedDirectories.isUnwritable(getLocationForDisk(dataDir)))
                   continue;
             DataDirectoryCandidate candidate = new DataDirectoryCandidate(dataDir);
             // exclude directory if its total writeSize does not fit to data directory
+            logger.debug("DataDirectory {} has {} bytes available, checking if we can write {} bytes", dataDir.location, candidate.availableSpace, writeSize);

Review Comment:
   Actually, that message exists in trunk. I backported it in an effort to make the very different  <=4.1 branches at least a bit more similar to trunk. For one thing, it allowed to reuse more test code.
   
   We can remove it, but then I would ask, why wouldn't we remove it from trunk as well. Where it exists already, before this patch?



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