Skip to content

Commit

Permalink
review fixes and suggested improvements
Browse files Browse the repository at this point in the history
  • Loading branch information
@smiklosovic
smiklosovic committed Jan 11, 2024
1 parent 7163029 commit 4ff28f9
Show file tree
Hide file tree
Showing 5 changed files with 188 additions and 51 deletions.
7 changes: 3 additions & 4 deletions src/java/org/apache/cassandra/service/StartupChecks.java
View file
Expand Up @@ -73,7 +73,6 @@
import org.apache.cassandra.utils.FBUtilities;
import org.apache.cassandra.utils.JavaUtils;
import org.apache.cassandra.utils.NativeLibrary;
import org.apache.cassandra.utils.SystemInfo;

import static org.apache.cassandra.config.CassandraRelevantProperties.CASSANDRA_JMX_LOCAL_PORT;
import static org.apache.cassandra.config.CassandraRelevantProperties.COM_SUN_MANAGEMENT_JMXREMOTE_PORT;
Expand Down Expand Up @@ -422,12 +421,12 @@ public void execute(StartupChecksOptions options) throws StartupException
@Override
public void execute(StartupChecksOptions options)
{
Optional<String> degradations = SystemInfo.instance().isDegraded();
Optional<String> degradations = FBUtilities.getSystemInfo().isDegraded();

if (degradations.isPresent())
logger.warn("Cassandra server running in degraded mode. " + degradations.get());
logger.warn("Cassandra server running in degraded mode. " + degradations.get());
else
logger.info("Checked OS settings and found them configured for optimal performance.");
logger.info("Checked OS settings and found them configured for optimal performance.");
}
};

Expand Down
14 changes: 13 additions & 1 deletion src/java/org/apache/cassandra/utils/FBUtilities.java
View file
Expand Up @@ -137,7 +137,8 @@ public class FBUtilities

private static int availableProcessors = CASSANDRA_AVAILABLE_PROCESSORS.getInt(DatabaseDescriptor.getAvailableProcessors());

private static volatile Supplier<Semver> kernelVersionSupplier = Suppliers.memoize(() -> SystemInfo.instance().getKernelVersion());
private static volatile Supplier<SystemInfo> systemInfoSupplier = Suppliers.memoize(SystemInfo::new);
private static volatile Supplier<Semver> kernelVersionSupplier = Suppliers.memoize(() -> systemInfoSupplier.get().getKernelVersion());

This comment has been minimized.

Sign in to view

Copy link
@jacek-lewandowski

jacek-lewandowski Jan 11, 2024

we do not need additional supplier for kernel version if we have a supplier for systeminfo

public static void setAvailableProcessors(int value)
{
Expand All @@ -150,6 +151,12 @@ public static void setKernelVersionSupplier(Supplier<Semver> supplier)
kernelVersionSupplier = supplier;
}

@VisibleForTesting
public static void setSystemInfoSupplier(Supplier<SystemInfo> supplier)
{
systemInfoSupplier = supplier;
}

public static int getAvailableProcessors()
{
if (availableProcessors > 0)
Expand Down Expand Up @@ -1450,4 +1457,9 @@ public static Semver getKernelVersion()
{
return kernelVersionSupplier.get();
}

public static SystemInfo getSystemInfo()
{
return systemInfoSupplier.get();
}
}
60 changes: 20 additions & 40 deletions src/java/org/apache/cassandra/utils/SystemInfo.java
View file
Expand Up @@ -39,7 +39,7 @@
* An abstraction of System information, this class provides access to system information without specifying how
* it is retrieved.
*/
public final class SystemInfo
public class SystemInfo
{
// TODO: Determine memlock limits if possible
// TODO: Determine if file system is remote or local
Expand All @@ -48,9 +48,14 @@ public final class SystemInfo
private static final Logger logger = LoggerFactory.getLogger(SystemInfo.class);

private static final long INFINITY = -1L;
private static final long EXPECTED_MIN_NOFILE = 10000L; // number of files that can be opened
private static final long EXPECTED_NPROC = 32768L; // number of processes
private static final long EXPECTED_AS = 0x7FFFFFFFL; // address space
static final long EXPECTED_MIN_NUMBER_OF_OPENED_FILES = 10000L; // number of files that can be opened
static final long EXPECTED_MIN_NUMBER_OF_PROCESSES = 32768L; // number of processes
static final long EXPECTED_ADDRESS_SPACE = 0x7FFFFFFFL; // address space

static final String OPEN_FILES_VIOLATION_MESSAGE = format("Minimum value for max open files should be >= %s. ", EXPECTED_MIN_NUMBER_OF_OPENED_FILES);
static final String NUMBER_OF_PROCESSES_VIOLATION_MESSAGE = format("Number of processes should be >= %s. ", EXPECTED_MIN_NUMBER_OF_PROCESSES);
static final String ADDRESS_SPACE_VIOLATION_MESSAGE = format("Amount of available address space should be >= %s. ", EXPECTED_ADDRESS_SPACE);
static final String SWAP_VIOLATION_MESSAGE = "Swap should be disabled. ";

/**
* The default number of processes that are reported if the actual value can not be retrieved.
Expand All @@ -59,30 +64,6 @@ public final class SystemInfo

private static final Pattern SPACES_PATTERN = Pattern.compile("\\s+");

private static Supplier<SystemInfo> provider = () -> new SystemInfo();

/**
* Sets the Supplier of the SystemInfo.
* <p>
* If {@code null} is passed as the provider the provider will be reset to the default provider.
* </p>
* @param provider the Supplier of SystemInfo that will be used for {@code instance} calls. May be null.
*/
public static void setProvider(Supplier<SystemInfo> provider) {
SystemInfo.provider = provider == null ? () -> new SystemInfo() : provider;
}

/**
* Gets an instance of SystemInfo. Whether the SystemInfo instance is new or memoized depends upon the
* implementation of the provider specified by {@code setProvider}. By default a new version of SystemInfo
* is created on every call.
* @return
*/
public static SystemInfo instance()
{
return provider.get();
}

/**
* The oshi.SystemInfo has the following note:
* Platform-specific Hardware and Software objects are retrieved via memoized suppliers. To conserve memory at the
Expand All @@ -94,7 +75,6 @@ public static SystemInfo instance()
*/
private final oshi.SystemInfo si;

// package private for testing
SystemInfo()
{
si = new oshi.SystemInfo();
Expand Down Expand Up @@ -137,9 +117,9 @@ public long getMaxProcess()
return "unlimited".equals(limit) ? INFINITY : Long.parseLong(limit);
}
}
logger.error( "'Max processes' not found in "+path);
logger.error("'Max processes' not found in {}", path);
}
catch (Throwable t)
catch (Exception t)
{
logger.error(format("Unable to read %s", path), t);
}
Expand Down Expand Up @@ -205,21 +185,21 @@ public Optional<String> isDegraded()
{
Supplier<String> expectedNumProc = () -> {
// only check proc on nproc linux
if (oshi.SystemInfo.getCurrentPlatform() == PlatformEnum.LINUX)

This comment has been minimized.

Sign in to view

Copy link
@jacek-lewandowski

jacek-lewandowski Jan 11, 2024

what's this???

This comment has been minimized.

Sign in to view

Copy link
@smiklosovic

smiklosovic Jan 11, 2024

Author

yeah, this should just use platform() as mentioned here apache#2842 (comment)
return invalid(getMaxProcess(), EXPECTED_NPROC) ? format("Number of processes should be >= %s. ", EXPECTED_NPROC)
: null;
if (platform() == PlatformEnum.LINUX)
return invalid(getMaxProcess(), EXPECTED_MIN_NUMBER_OF_PROCESSES) ? NUMBER_OF_PROCESSES_VIOLATION_MESSAGE
: null;
else
return format("System is running %s, Linux OS is recommended", platform());
return format("System is running %s, Linux OS is recommended. ", platform());
};

Supplier<String> swapShouldBeDisabled = () -> (getSwapSize() > 0) ? "Swap should be disabled. " : null;
Supplier<String> swapShouldBeDisabled = () -> (getSwapSize() > 0) ? SWAP_VIOLATION_MESSAGE : null;

Supplier<String> expectedAddressSpace = () -> invalid(getVirtualMemoryMax(), EXPECTED_AS)
? format("Amount of available address space should be >= %s. ", EXPECTED_AS)
Supplier<String> expectedAddressSpace = () -> invalid(getVirtualMemoryMax(), EXPECTED_ADDRESS_SPACE)
? ADDRESS_SPACE_VIOLATION_MESSAGE
: null;

Supplier<String> expectedMinNoFile = () -> invalid(getMaxOpenFiles(), EXPECTED_MIN_NOFILE)
? format("Minimum value for max open files should be >= %s. ", EXPECTED_MIN_NOFILE)
Supplier<String> expectedMinNoFile = () -> invalid(getMaxOpenFiles(), EXPECTED_MIN_NUMBER_OF_OPENED_FILES)
? OPEN_FILES_VIOLATION_MESSAGE
: null;

StringBuilder sb = new StringBuilder();
Expand Down
3 changes: 2 additions & 1 deletion test/distributed/org/apache/cassandra/distributed/test/ResourceLeakTest.java
View file
Expand Up @@ -41,6 +41,7 @@
import org.apache.cassandra.distributed.api.IInvokableInstance;
import org.apache.cassandra.distributed.shared.JMXUtil;
import org.apache.cassandra.io.util.File;
import org.apache.cassandra.utils.FBUtilities;
import org.apache.cassandra.utils.SystemInfo;

import static org.apache.cassandra.distributed.api.Feature.GOSSIP;
Expand Down Expand Up @@ -91,7 +92,7 @@ static String outputFilename(String base, String description, String extension)
*/
private static Long getProcessId()
{
long pid = SystemInfo.instance().getPid();
long pid = FBUtilities.getSystemInfo().getPid();

if (pid >= 0)
return Long.valueOf(pid);
Expand Down
155 changes: 150 additions & 5 deletions test/unit/org/apache/cassandra/utils/SystemInfoTest.java
View file
Expand Up @@ -18,19 +18,164 @@

package org.apache.cassandra.utils;

import com.vdurmont.semver4j.Semver;
import java.util.Optional;

import org.junit.Assert;
import org.junit.Assume;
import org.junit.Test;

import com.vdurmont.semver4j.Semver;
import oshi.PlatformEnum;

import static org.apache.cassandra.utils.SystemInfo.ADDRESS_SPACE_VIOLATION_MESSAGE;
import static org.apache.cassandra.utils.SystemInfo.NUMBER_OF_PROCESSES_VIOLATION_MESSAGE;
import static org.apache.cassandra.utils.SystemInfo.OPEN_FILES_VIOLATION_MESSAGE;
import static org.apache.cassandra.utils.SystemInfo.SWAP_VIOLATION_MESSAGE;
import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;

public class SystemInfoTest
{
private static class TestSystemInfo extends SystemInfo
{
@Override
public PlatformEnum platform()
{
return PlatformEnum.LINUX;
}

@Override
public long getMaxProcess()
{
return EXPECTED_MIN_NUMBER_OF_PROCESSES;
}

@Override
public long getMaxOpenFiles()
{
return EXPECTED_MIN_NUMBER_OF_OPENED_FILES;
}

@Override
public long getVirtualMemoryMax()
{
return EXPECTED_ADDRESS_SPACE;
}

@Override
public long getSwapSize()
{
return 0;
}
}

@Test
public void testSystemInfo()
{
SystemInfo oldSystemInfo = FBUtilities.getSystemInfo();

try
{
// valid testing system info does not violate anything
TestSystemInfo testSystemInfo = new TestSystemInfo();
assertFalse(testSystemInfo.isDegraded().isPresent());

// platform

public class SystemInfoTest {
assertDegradation(new TestSystemInfo()
{
@Override
public PlatformEnum platform()
{
return PlatformEnum.FREEBSD;
}
}, "System is running FREEBSD, Linux OS is recommended. ");

// swap


assertDegradation(new TestSystemInfo()
{
@Override
public long getSwapSize()
{
return 100;
}
}, SWAP_VIOLATION_MESSAGE);

// address space

assertDegradation(new TestSystemInfo()
{
@Override
public long getVirtualMemoryMax()
{
return 1234;
}
}, ADDRESS_SPACE_VIOLATION_MESSAGE);

// expected minimal number of opened files

assertDegradation(new TestSystemInfo()
{
@Override
public long getMaxOpenFiles()
{
return 10;
}
}, OPEN_FILES_VIOLATION_MESSAGE);

// expected number of processes

assertDegradation(new TestSystemInfo()
{
@Override
public long getMaxProcess()
{
return 5;
}
}, NUMBER_OF_PROCESSES_VIOLATION_MESSAGE);

// test multiple violations

assertDegradation(new TestSystemInfo()
{
@Override
public PlatformEnum platform()
{
return PlatformEnum.FREEBSD;
}

@Override
public long getSwapSize()
{
return 10;
}
}, "System is running FREEBSD, Linux OS is recommended. " + SWAP_VIOLATION_MESSAGE);
}
finally
{
FBUtilities.setSystemInfoSupplier(() -> oldSystemInfo);
}
}

@Test
public void testGetKernelVersion()
{
Assume.assumeTrue(FBUtilities.isLinux);
Semver kernelVersion = SystemInfo.instance().getKernelVersion();
assertThat(kernelVersion).isGreaterThan(new Semver("0.0.0", Semver.SemverType.LOOSE));
assertThat(kernelVersion).isLessThan(new Semver("100.0.0", Semver.SemverType.LOOSE));
Semver kernelVersion = FBUtilities.getSystemInfo().getKernelVersion();
assertThat(kernelVersion).isGreaterThan(new Semver("0.0.0", Semver.SemverType.LOOSE))
.isLessThan(new Semver("100.0.0", Semver.SemverType.LOOSE));
}

private void assertDegradation(final SystemInfo systemInfo, String expectedDegradation)
{
FBUtilities.setSystemInfoSupplier(() -> systemInfo);
Optional<String> degradations = FBUtilities.getSystemInfo().isDegraded();

assertTrue(degradations.isPresent());
assertEquals(expectedDegradation, degradations.get());
}
}

0 comments on commit 4ff28f9

Please sign in to comment.