Skip to content

Remove legacy versioned logic for DefaultSystemMemoryInfo #85761

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 12, 2022
Merged

Remove legacy versioned logic for DefaultSystemMemoryInfo #85761

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9b2031d
Remove legacy versioned logic for DefaultSystemMemoryInfo
rjernst Apr 8, 2022
76d98ce
address feedback
rjernst Apr 11, 2022
7e5a66a
Merge branch 'master' into cli/system_memory_info
rjernst Apr 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 11 additions & 13 deletions ...ls/launchers/src/main/java/org/elasticsearch/tools/launchers/DefaultSystemMemoryInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,28 +11,26 @@
import com.sun.management.OperatingSystemMXBean;

import java.lang.management.ManagementFactory;
import java.util.function.LongSupplier;

/**
* A {@link SystemMemoryInfo} which delegates to {@link OperatingSystemMXBean}.
*
* <p>Prior to JDK 14 {@link OperatingSystemMXBean} did not take into consideration container memory limits when reporting total system
* memory. Therefore attempts to use this implementation on earlier JDKs will result in an {@link SystemMemoryInfoException}.
*/
@SuppressForbidden(reason = "Using com.sun internals is the only way to query total system memory")
public final class DefaultSystemMemoryInfo implements SystemMemoryInfo {
private final OperatingSystemMXBean operatingSystemMXBean;
private final LongSupplier totalMemory;

public DefaultSystemMemoryInfo() {
this.operatingSystemMXBean = (OperatingSystemMXBean) ManagementFactory.getOperatingSystemMXBean();
this.totalMemory = getOSBeanMemoryGetter();
}

@Override
@SuppressWarnings("deprecation")
public long availableSystemMemory() throws SystemMemoryInfoException {
if (Runtime.version().feature() < 14) {
throw new SystemMemoryInfoException("The minimum required Java version is 14 to use " + this.getClass().getName());
}
@SuppressForbidden(reason = "Using com.sun internals is the only way to query total system memory")
private static LongSupplier getOSBeanMemoryGetter() {
OperatingSystemMXBean bean = (OperatingSystemMXBean) ManagementFactory.getOperatingSystemMXBean();
return bean::getTotalMemorySize;
}

return operatingSystemMXBean.getTotalPhysicalMemorySize();
@Override
public long availableSystemMemory() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we've removed SystemMemoryInfoException from the signature here we should probably remove it everywhere else as well (catch statements, SystemMemoryInfo interface, etc). As far as I can tell this was the only place we ever threw such an exception, unless we want to keep this in place for furture implementations?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed SystemMemoryInfoException

return totalMemory.getAsLong();
}
}
10 changes: 2 additions & 8 deletions ...tools/launchers/src/main/java/org/elasticsearch/tools/launchers/MachineDependentHeap.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,14 +72,8 @@ public List<String> determineHeapSettings(Path configDir, List<String> userDefin

List<String> determineHeapSettings(InputStream config) {
MachineNodeRole nodeRole = NodeRoleParser.parse(config);

try {
long availableSystemMemory = systemMemoryInfo.availableSystemMemory();
return options(nodeRole.heap(availableSystemMemory));
} catch (SystemMemoryInfo.SystemMemoryInfoException e) {
// If unable to determine system memory (ex: incompatible jdk version) fallback to defaults
return options(DEFAULT_HEAP_SIZE_MB);
}
long availableSystemMemory = systemMemoryInfo.availableSystemMemory();
return options(nodeRole.heap(availableSystemMemory));
}

private static List<String> options(int heapSize) {
Expand Down
2 changes: 1 addition & 1 deletion ...aunchers/src/main/java/org/elasticsearch/tools/launchers/OverridableSystemMemoryInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ public OverridableSystemMemoryInfo(final List<String> userDefinedJvmOptions, Sys
}

@Override
public long availableSystemMemory() throws SystemMemoryInfoException {
public long availableSystemMemory() {

return userDefinedJvmOptions.stream()
.filter(option -> option.startsWith("-Des.total_memory_bytes="))
Expand Down
9 changes: 1 addition & 8 deletions ...ion/tools/launchers/src/main/java/org/elasticsearch/tools/launchers/SystemMemoryInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,6 @@ public interface SystemMemoryInfo {
/**
*
* @return total system memory available to heap or native process allocation in bytes
* @throws SystemMemoryInfoException if unable to determine available system memory
*/
long availableSystemMemory() throws SystemMemoryInfoException;

class SystemMemoryInfoException extends Exception {
public SystemMemoryInfoException(String message) {
super(message);
}
}
long availableSystemMemory();
}
10 changes: 0 additions & 10 deletions .../launchers/src/test/java/org/elasticsearch/tools/launchers/MachineDependentHeapTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,12 +77,6 @@ public void testDataNodeOptions() {
assertThat(options, containsInAnyOrder("-Xmx128m", "-Xms128m"));
}

public void testFallbackOptions() throws Exception {
MachineDependentHeap machineDependentHeap = new MachineDependentHeap(errorThrowingMemoryInfo());
List<String> options = machineDependentHeap.determineHeapSettings(configPath(), Collections.emptyList());
assertThat(options, containsInAnyOrder("-Xmx1024m", "-Xms1024m"));
}

private static List<String> calculateHeap(double memoryInGigabytes, String... roles) {
MachineDependentHeap machineDependentHeap = new MachineDependentHeap(systemMemoryInGigabytes(memoryInGigabytes));
String configYaml = "node.roles: [" + String.join(",", roles) + "]";
Expand All @@ -101,10 +95,6 @@ private static SystemMemoryInfo systemMemoryInGigabytes(double gigabytes) {
return () -> (long) (gigabytes * 1024 * 1024 * 1024);
}

private static SystemMemoryInfo errorThrowingMemoryInfo() {
return () -> { throw new SystemMemoryInfo.SystemMemoryInfoException("something went wrong"); };
}

private static Path configPath() {
URL resource = MachineDependentHeapTests.class.getResource("/config/elasticsearch.yml");
try {
Expand Down
16 changes: 7 additions & 9 deletions ...ers/src/test/java/org/elasticsearch/tools/launchers/OverridableSystemMemoryInfoTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@

package org.elasticsearch.tools.launchers;

import org.elasticsearch.tools.launchers.SystemMemoryInfo.SystemMemoryInfoException;

import java.util.List;

import static org.hamcrest.Matchers.is;
Expand All @@ -20,41 +18,41 @@ public class OverridableSystemMemoryInfoTests extends LaunchersTestCase {

private static final long FALLBACK = -1L;

public void testNoOptions() throws SystemMemoryInfoException {
public void testNoOptions() {
final SystemMemoryInfo memoryInfo = new OverridableSystemMemoryInfo(List.of(), fallbackSystemMemoryInfo());
assertThat(memoryInfo.availableSystemMemory(), is(FALLBACK));
}

public void testNoOverrides() throws SystemMemoryInfoException {
public void testNoOverrides() {
final SystemMemoryInfo memoryInfo = new OverridableSystemMemoryInfo(List.of("-Da=b", "-Dx=y"), fallbackSystemMemoryInfo());
assertThat(memoryInfo.availableSystemMemory(), is(FALLBACK));
}

public void testValidSingleOverride() throws SystemMemoryInfoException {
public void testValidSingleOverride() {
final SystemMemoryInfo memoryInfo = new OverridableSystemMemoryInfo(
List.of("-Des.total_memory_bytes=123456789"),
fallbackSystemMemoryInfo()
);
assertThat(memoryInfo.availableSystemMemory(), is(123456789L));
}

public void testValidOverrideInList() throws SystemMemoryInfoException {
public void testValidOverrideInList() {
final SystemMemoryInfo memoryInfo = new OverridableSystemMemoryInfo(
List.of("-Da=b", "-Des.total_memory_bytes=987654321", "-Dx=y"),
fallbackSystemMemoryInfo()
);
assertThat(memoryInfo.availableSystemMemory(), is(987654321L));
}

public void testMultipleValidOverridesInList() throws SystemMemoryInfoException {
public void testMultipleValidOverridesInList() {
final SystemMemoryInfo memoryInfo = new OverridableSystemMemoryInfo(
List.of("-Des.total_memory_bytes=123456789", "-Da=b", "-Des.total_memory_bytes=987654321", "-Dx=y"),
fallbackSystemMemoryInfo()
);
assertThat(memoryInfo.availableSystemMemory(), is(987654321L));
}

public void testNegativeOverride() throws SystemMemoryInfoException {
public void testNegativeOverride() {
final SystemMemoryInfo memoryInfo = new OverridableSystemMemoryInfo(
List.of("-Da=b", "-Des.total_memory_bytes=-123", "-Dx=y"),
fallbackSystemMemoryInfo()
Expand All @@ -67,7 +65,7 @@ public void testNegativeOverride() throws SystemMemoryInfoException {
}
}

public void testUnparsableOverride() throws SystemMemoryInfoException {
public void testUnparsableOverride() {
final SystemMemoryInfo memoryInfo = new OverridableSystemMemoryInfo(
List.of("-Da=b", "-Des.total_memory_bytes=invalid", "-Dx=y"),
fallbackSystemMemoryInfo()
Expand Down