You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "henrikingo (via GitHub)" <gi...@apache.org> on 2023/04/19 15:01:18 UTC
[GitHub] [cassandra] henrikingo opened a new pull request, #2284: CASSANDRA-18260 Add details to Error: Not enough space for compaction
henrikingo opened a new pull request, #2284:
URL: https://github.com/apache/cassandra/pull/2284
Adds separate WARNing level messages to the check itself in Directories.java.
patch by Henrik Ingo; reviewed by Mick Semb Wever for CASSANDRA-18260
--
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
[GitHub] [cassandra] Maxwell-Guo commented on a diff in pull request #2284: CASSANDRA-18260 Add details to Error: Not enough space for compaction
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2284:
URL: https://github.com/apache/cassandra/pull/2284#discussion_r1178050678
##########
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:
agree with you
--
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
[GitHub] [cassandra] michaelsembwever commented on a diff in pull request #2284: CASSANDRA-18260 Add details to Error: Not enough space for compaction
Posted by "michaelsembwever (via GitHub)" <gi...@apache.org>.
michaelsembwever commented on code in PR #2284:
URL: https://github.com/apache/cassandra/pull/2284#discussion_r1172222392
##########
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:
good catch. agree.
--
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
[GitHub] [cassandra] Maxwell-Guo commented on a diff in pull request #2284: CASSANDRA-18260 Add details to Error: Not enough space for compaction
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2284:
URL: https://github.com/apache/cassandra/pull/2284#discussion_r1173236552
##########
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:
yeah，I have checked the trunk branch before left the comment and I also saw this debug log message, but I think we should also remove the debug log there.
Besides I think "logger.debug("FileStore {} has {} bytes available, checking if we can write {} bytes", toWrite.getKey(), availableForCompaction, toWrite.getValue());" this message just tell us we need a bytes and we have b bytes, if the dir have enough space , the message is meaningless and useless to me. if we do not have enough space, the warn log below this log can give enough information.
@michaelsembwever what do you think ?
--
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
[GitHub] [cassandra] michaelsembwever commented on a diff in pull request #2284: CASSANDRA-18260 Add details to Error: Not enough space for compaction
Posted by "michaelsembwever (via GitHub)" <gi...@apache.org>.
michaelsembwever commented on code in PR #2284:
URL: https://github.com/apache/cassandra/pull/2284#discussion_r1172220834
##########
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:
@henrikingo and I discussed this, and agreed there's value in the comment, bc we don't usually directly use the logging impl classes.
--
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
[GitHub] [cassandra] henrikingo commented on a diff in pull request #2284: CASSANDRA-18260 Add details to Error: Not enough space for compaction
Posted by "henrikingo (via GitHub)" <gi...@apache.org>.
henrikingo commented on code in PR #2284:
URL: https://github.com/apache/cassandra/pull/2284#discussion_r1174234458
##########
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 was thinking about it when I worked on trunk. I think the message makes sense as such: It records exactly the kind of environmental data you would use for DEBUG purposes, but 99% of the time is uninteresting. The fact that the result of the function can be determined from the message is actually a good property. It just means a single DEBUG message has captured everything.
Now... I do agree the DEBUG message is perhaps unnecessary after the addition of the WARN message.
The reason I didn't remove it was that it would have felt weird to remove something that wasn't even released yet. But of course we can do it, it's a poor reason.
But... Maybe ask the authors of the patch that added the debug message @krummas and @jmckenzie-dev ?
--
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
[GitHub] [cassandra] Maxwell-Guo commented on a diff in pull request #2284: CASSANDRA-18260 Add details to Error: Not enough space for compaction
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2284:
URL: https://github.com/apache/cassandra/pull/2284#discussion_r1173251193
##########
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:
I saw the ut test case in trunk and Parameterized test is created for sstable id for SequenceBasedSSTableId and UUIDBasedSSTableId.
I just modify the code :
` public static final DataDirectory uniformDir = new DataDirectory(new File("/uniformDir2"))
{
public long getAvailableSpace()
{
return 999L;
}
};
public static final DataDirectory nearlyFullDir = new DataDirectory(new File("/nearlyFullDir1"))
{
public long getAvailableSpace()
{
return 11L;
}
};
public static final DataDirectory veryFullDir = new DataDirectory(new File("/veryFullDir"))
{
public long getAvailableSpace()
{
return 4L;
}
};
@Parameterized.Parameter
public DataDirectory directory;
@Parameterized.Parameter(1)
publicLong value;
@Parameterized.Parameter(2)
public String message;
@Parameterized.Parameters
public static Collection<Object[]> idBuilders()
{
return Arrays.asList(new Object[]{ uniformDir, 999L, "uniformDir2" },
new Object[]{ nearlyFullDir, 11L, "nearlyFullDir1" },
new Object[]{ veryFullDir, 4L,"veryFullDir" });
}
`
so code logic only needs to be there once, and duplicate logic can be deleted, then every time we can just verify the realation between directory, value, message.
--
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
[GitHub] [cassandra] henrikingo commented on a diff in pull request #2284: CASSANDRA-18260 Add details to Error: Not enough space for compaction
Posted by "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
[GitHub] [cassandra] Maxwell-Guo commented on a diff in pull request #2284: CASSANDRA-18260 Add details to Error: Not enough space for compaction
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2284:
URL: https://github.com/apache/cassandra/pull/2284#discussion_r1178052239
##########
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:
yeah ，we may ask them for help
--
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
[GitHub] [cassandra] henrikingo commented on a diff in pull request #2284: CASSANDRA-18260 Add details to Error: Not enough space for compaction
Posted by "henrikingo (via GitHub)" <gi...@apache.org>.
henrikingo commented on code in PR #2284:
URL: https://github.com/apache/cassandra/pull/2284#discussion_r1174247473
##########
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:
Hmm... I don't think that's equivalent to the test as submitted in the PR. First of all the dataDirectories should all exist together in each test. They aren't different input values for a parameterized test.
From there, note that the lines
String logMsgFormat = "DataDirectory %s has %d bytes available, checking if we can write %d bytes";
String logMsg = String.format(logMsgFormat, "/nearlyFullDir1", 11, 2);
checkFormattedMessage(filteredLog, Level.DEBUG, logMsg, 1);
Are merely checking log output from a test that already ran. If we wanted to parameterize, we can, but that would be just these 4 lines
assertTrue(d.hasAvailableDiskSpace(1,2));
assertTrue(d.hasAvailableDiskSpace(10,99));
assertFalse(d.hasAvailableDiskSpace(10,1024));
assertFalse(d.hasAvailableDiskSpace(1024,1024*1024));
And in the log output checking code, only 1 value could be parameterized. (The number 2 in what I quoted above.)
In short,I don't see the benefit of Parameterized here. The question remains: If I wrap the assertions into a simple loop:
Collection<Object[]> checkTheseOut = Arrays.asList(new Object[]{ Level.DEBUG, "/nearlyFullDir1", 11, 2},
new Object[]{ "/uniformDir2, 999, 2 },
...);
for ( values: checkTheseOut )
{
String logMsg = String.format(logMsgFormat, (String)values[1], (int)values[2], (int)values[3]);
checkFormattedMessage(filteredLog, (Level)values[0], logMsg, 1);
}
...then how will the future developer running this unit test easily interpret the results when something failed?
--
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
[GitHub] [cassandra] Maxwell-Guo commented on a diff in pull request #2284: CASSANDRA-18260 Add details to Error: Not enough space for compaction
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
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: 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
[GitHub] [cassandra] Maxwell-Guo commented on a diff in pull request #2284: CASSANDRA-18260 Add details to Error: Not enough space for compaction
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2284:
URL: https://github.com/apache/cassandra/pull/2284#discussion_r1172216069
##########
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)
Review Comment:
code format : for ()
##########
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:
yeah，I have checked the trunk branch before left the comment and I also saw this debug log message, but I think we should also remove the debug log there.
Besides I think "logger.debug("FileStore {} has {} bytes available, checking if we can write {} bytes", toWrite.getKey(), availableForCompaction, toWrite.getValue());" this message just tell us we need a bytes and we have b bytes, if the dir have enough space , the message is meaningless and useless to me. if we do not have enough space, the warn log below this log can give enough information.
##########
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))
Review Comment:
code format : if ()
--
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