Maxwell-Guo commented on code in PR #2284:
URL: https://github.com/apache/cassandra/pull/2284#discussion_r1172199837
##########
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:
I don't think we need this , as it may print the log every compaction time
not matter the data dir's availableSpace is really available.
we can remove this.
##########
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);
if (candidate.availableSpace < writeSize)
+ {
+ logger.warn("DataDirectory {} can't be used for compaction.
Only {} is available, but {} is the minimum write size.",
+ candidate.dataDirectory.location,
+
FileUtils.stringifyFileSize(candidate.availableSpace),
+ FileUtils.stringifyFileSize(writeSize));
continue;
+ }
totalAvailable += candidate.availableSpace;
}
- return totalAvailable > expectedTotalWriteSize;
+ if(totalAvailable <= expectedTotalWriteSize)
+ {
+ StringJoiner pathString = new StringJoiner(",", "[", "]");
+ for (DataDirectory p: paths)
+ {
+ pathString.add(p.location.getAbsolutePath());
+ }
+ logger.warn("Insufficient disk space for compaction! Across {}
there's only {} available, but {} is needed.",
Review Comment:
I think change "!" to "." is enough.
##########
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);
if (candidate.availableSpace < writeSize)
+ {
+ logger.warn("DataDirectory {} can't be used for compaction.
Only {} is available, but {} is the minimum write size.",
+ candidate.dataDirectory.location,
+
FileUtils.stringifyFileSize(candidate.availableSpace),
+ FileUtils.stringifyFileSize(writeSize));
continue;
+ }
totalAvailable += candidate.availableSpace;
}
- return totalAvailable > expectedTotalWriteSize;
+ if(totalAvailable <= expectedTotalWriteSize)
Review Comment:
nit:code format~~
##########
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){
Review Comment:
nit : code format "{" need to start a new line.
##########
test/unit/org/apache/cassandra/db/DirectoriesTest.java:
##########
@@ -23,18 +23,30 @@
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.*;
+import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.Callable;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
import com.google.common.collect.Sets;
import org.apache.commons.lang3.StringUtils;
+import org.junit.After;
import org.junit.AfterClass;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.Test;
+import org.slf4j.LoggerFactory;
+import org.slf4j.MDC;
+// Our version of Sfl4j seems to be missing the ListAppender class.
Review Comment:
I think these comments can be removed.
##########
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)
Review Comment:
code format
##########
test/unit/org/apache/cassandra/db/DirectoriesTest.java:
##########
@@ -71,6 +83,13 @@
private static Set<TableMetadata> CFM;
private static Map<String, List<File>> files;
+
+ private static final String MDCID = "test-DirectoriesTest-id";
+ private static AtomicInteger diyThreadId = new AtomicInteger(1);
+ private int myDiyId=-1;
Review Comment:
nit: code format : myDiyId = -1;
##########
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:
we may make a Parameterized test for three paramters: nearlyFullDir1,
uniformDir2, veryFullDir
,This will make the logic clearer and the code more concise。
As line 817 to 873 have sane logic for these three paramters
--
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]
