From 14b60bab7e0f51bc21176068b354cd8f0bf45852 Mon Sep 17 00:00:00 2001 From: Henning Baldersheim Date: Wed, 10 Jun 2020 14:19:47 +0200 Subject: Revert "Add feature flag control of default jvm gc options" --- .../com/yahoo/config/model/api/ModelContext.java | 3 --- .../yahoo/config/model/deploy/TestProperties.java | 6 ----- .../model/container/xml/ContainerModelBuilder.java | 31 ++++++++++++---------- .../vespa/model/container/xml/JvmOptionsTest.java | 29 +++++++++++++------- .../config/server/deploy/ModelContextImpl.java | 5 ---- .../src/main/java/com/yahoo/vespa/flags/Flags.java | 5 ---- 6 files changed, 36 insertions(+), 43 deletions(-) diff --git a/config-model-api/src/main/java/com/yahoo/config/model/api/ModelContext.java b/config-model-api/src/main/java/com/yahoo/config/model/api/ModelContext.java index 2fa3217ef4a..ad91ad6e1bc 100644 --- a/config-model-api/src/main/java/com/yahoo/config/model/api/ModelContext.java +++ b/config-model-api/src/main/java/com/yahoo/config/model/api/ModelContext.java @@ -89,9 +89,6 @@ public interface ModelContext { String docprocLoadBalancerType(); - /// Default setting for the gc-options attribute if not specified explicit by application - String jvmGCOptions(); - boolean useDistributorBtreeDb(); boolean useThreePhaseUpdates(); diff --git a/config-model/src/main/java/com/yahoo/config/model/deploy/TestProperties.java b/config-model/src/main/java/com/yahoo/config/model/deploy/TestProperties.java index af3a8335475..b00eb900221 100644 --- a/config-model/src/main/java/com/yahoo/config/model/deploy/TestProperties.java +++ b/config-model/src/main/java/com/yahoo/config/model/deploy/TestProperties.java @@ -43,7 +43,6 @@ public class TestProperties implements ModelContext.Properties { private double threadPoolSizeFactor = 0.0; private double queueSizeFactor = 0.0; private String docprocLoadBalancerType = null; - private String jvmGCOptions = null; private Optional endpointCertificateSecrets = Optional.empty(); private AthenzDomain athenzDomain; private ApplicationRoles applicationRoles; @@ -57,7 +56,6 @@ public class TestProperties implements ModelContext.Properties { @Override public boolean hostedVespa() { return hostedVespa; } @Override public Zone zone() { return zone; } @Override public Set endpoints() { return endpoints; } - @Override public String jvmGCOptions() { return jvmGCOptions; } @Override public boolean isBootstrap() { return false; } @Override public boolean isFirstTimeDeployment() { return false; } @@ -80,10 +78,6 @@ public class TestProperties implements ModelContext.Properties { docprocLoadBalancerType = type; return this; } - public TestProperties setJvmGCOptions(String gcOptions) { - jvmGCOptions = gcOptions; - return this; - } public TestProperties setDefaultTermwiseLimit(double limit) { defaultTermwiseLimit = limit; return this; diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java index 6ae01ad0177..559a4b8b668 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java @@ -513,14 +513,17 @@ public class ContainerModelBuilder extends ConfigModelBuilder { return (gcAlgorithm.matcher(jvmargs).find() ||cmsArgs.matcher(jvmargs).find()); } - private static String buildJvmGCOptions(DeployState deployState, String jvmGCOPtions) { - String options = (jvmGCOPtions != null) - ? jvmGCOPtions - : deployState.getProperties().jvmGCOptions(); - return (options == null ||options.isEmpty()) ? ContainerCluster.G1GC : options; + private static String buildJvmGCOptions(Zone zone, String jvmGCOPtions, boolean isHostedVespa) { + if (jvmGCOPtions != null) { + return jvmGCOPtions; + } else if ((zone.system() == SystemName.dev) || isHostedVespa) { + return null; + } else { + return ContainerCluster.G1GC; + } } private static String getJvmOptions(ApplicationContainerCluster cluster, Element nodesElement, DeployLogger deployLogger) { - String jvmOptions; + String jvmOptions = ""; if (nodesElement.hasAttribute(VespaDomBuilder.JVM_OPTIONS)) { jvmOptions = nodesElement.getAttribute(VespaDomBuilder.JVM_OPTIONS); if (nodesElement.hasAttribute(VespaDomBuilder.JVMARGS_ATTRIB_NAME)) { @@ -538,17 +541,15 @@ public class ContainerModelBuilder extends ConfigModelBuilder { return jvmOptions; } - private static String extractAttribute(Element element, String attrName) { - return element.hasAttribute(attrName) ? element.getAttribute(attrName) : null; - } - void extractJvmFromLegacyNodesTag(List nodes, ApplicationContainerCluster cluster, Element nodesElement, ConfigModelContext context) { applyNodesTagJvmArgs(nodes, getJvmOptions(cluster, nodesElement, context.getDeployLogger())); if (!cluster.getJvmGCOptions().isPresent()) { - String jvmGCOptions = extractAttribute(nodesElement, VespaDomBuilder.JVM_GC_OPTIONS); - cluster.setJvmGCOptions(buildJvmGCOptions(context.getDeployState(), jvmGCOptions)); + String jvmGCOptions = nodesElement.hasAttribute(VespaDomBuilder.JVM_GC_OPTIONS) + ? nodesElement.getAttribute(VespaDomBuilder.JVM_GC_OPTIONS) + : null; + cluster.setJvmGCOptions(buildJvmGCOptions(context.getDeployState().zone(), jvmGCOptions, context.getDeployState().isHosted())); } applyMemoryPercentage(cluster, nodesElement.getAttribute(VespaDomBuilder.Allocated_MEMORY_ATTRIB_NAME)); @@ -558,8 +559,10 @@ public class ContainerModelBuilder extends ConfigModelBuilder { Element jvmElement, ConfigModelContext context) { applyNodesTagJvmArgs(nodes, jvmElement.getAttribute(VespaDomBuilder.OPTIONS)); applyMemoryPercentage(cluster, jvmElement.getAttribute(VespaDomBuilder.Allocated_MEMORY_ATTRIB_NAME)); - String jvmGCOptions = extractAttribute(jvmElement, VespaDomBuilder.GC_OPTIONS); - cluster.setJvmGCOptions(buildJvmGCOptions(context.getDeployState(), jvmGCOptions)); + String jvmGCOptions = jvmElement.hasAttribute(VespaDomBuilder.GC_OPTIONS) + ? jvmElement.getAttribute(VespaDomBuilder.GC_OPTIONS) + : null; + cluster.setJvmGCOptions(buildJvmGCOptions(context.getDeployState().zone(), jvmGCOptions, context.getDeployState().isHosted())); } /** diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/JvmOptionsTest.java b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/JvmOptionsTest.java index 448ffffc04b..484709a0c18 100644 --- a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/JvmOptionsTest.java +++ b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/JvmOptionsTest.java @@ -50,6 +50,7 @@ public class JvmOptionsTest extends ContainerModelBuilderTestBase { VespaModel model = new VespaModel(new NullConfigModelRegistry(), new DeployState.Builder() .applicationPackage(applicationPackage) .deployLogger(logger) + .properties(new TestProperties().setHostedVespa(true)) .build()); QrStartConfig.Builder qrStartBuilder = new QrStartConfig.Builder(); model.getConfig(qrStartBuilder, "container/container.0"); @@ -84,11 +85,11 @@ public class JvmOptionsTest extends ContainerModelBuilderTestBase { } private static void verifyIgnoreJvmGCOptions(boolean isHosted) throws IOException, SAXException { - verifyIgnoreJvmGCOptionsIfJvmArgs("jvmargs", ContainerCluster.G1GC); - verifyIgnoreJvmGCOptionsIfJvmArgs( "jvm-options", "-XX:+UseG1GC"); + verifyIgnoreJvmGCOptionsIfJvmArgs(isHosted, "jvmargs", ContainerCluster.G1GC); + verifyIgnoreJvmGCOptionsIfJvmArgs(isHosted, "jvm-options", "-XX:+UseG1GC"); } - private static void verifyIgnoreJvmGCOptionsIfJvmArgs(String jvmOptionsName, String expectedGC) throws IOException, SAXException { + private static void verifyIgnoreJvmGCOptionsIfJvmArgs(boolean isHosted, String jvmOptionsName, String expectedGC) throws IOException, SAXException { String servicesXml = "" + " " + @@ -101,6 +102,8 @@ public class JvmOptionsTest extends ContainerModelBuilderTestBase { VespaModel model = new VespaModel(new NullConfigModelRegistry(), new DeployState.Builder() .applicationPackage(applicationPackage) .deployLogger(logger) + .zone(new Zone(SystemName.cd, Environment.dev, RegionName.from("here"))) + .properties(new TestProperties().setHostedVespa(isHosted)) .build()); QrStartConfig.Builder qrStartBuilder = new QrStartConfig.Builder(); model.getConfig(qrStartBuilder, "container/container.0"); @@ -114,7 +117,7 @@ public class JvmOptionsTest extends ContainerModelBuilderTestBase { verifyIgnoreJvmGCOptions(true); } - private void verifyJvmGCOptions(String featureFlagDefault, String override, String expected) throws IOException, SAXException { + private void verifyJvmGCOptions(boolean isHosted, String override, Zone zone, String expected) throws IOException, SAXException { String servicesXml = "" + " " : ("jvm-gc-options='" + override + "'>")) + @@ -127,7 +130,8 @@ public class JvmOptionsTest extends ContainerModelBuilderTestBase { VespaModel model = new VespaModel(new NullConfigModelRegistry(), new DeployState.Builder() .applicationPackage(applicationPackage) .deployLogger(logger) - .properties(new TestProperties().setJvmGCOptions(featureFlagDefault)) + .zone(zone) + .properties(new TestProperties().setHostedVespa(isHosted)) .build()); QrStartConfig.Builder qrStartBuilder = new QrStartConfig.Builder(); model.getConfig(qrStartBuilder, "container/container.0"); @@ -135,13 +139,18 @@ public class JvmOptionsTest extends ContainerModelBuilderTestBase { assertEquals(expected, qrStartConfig.jvm().gcopts()); } + private void verifyJvmGCOptions(boolean isHosted, Zone zone, String expected) throws IOException, SAXException { + verifyJvmGCOptions(isHosted, null, zone, expected); + verifyJvmGCOptions(isHosted, "-XX:+UseG1GC", zone, "-XX:+UseG1GC"); + Zone DEV = new Zone(SystemName.dev, zone.environment(), zone.region()); + verifyJvmGCOptions(isHosted, null, DEV, ContainerCluster.G1GC); + verifyJvmGCOptions(isHosted, "-XX:+UseConcMarkSweepGC", DEV, "-XX:+UseConcMarkSweepGC"); + } + @Test public void requireThatJvmGCOptionsIsHonoured() throws IOException, SAXException { - verifyJvmGCOptions(null,null, ContainerCluster.G1GC); - verifyJvmGCOptions("-XX:+UseConcMarkSweepGC",null, "-XX:+UseConcMarkSweepGC"); - verifyJvmGCOptions(null,"-XX:+UseG1GC", "-XX:+UseG1GC"); - verifyJvmGCOptions("-XX:+UseConcMarkSweepGC","-XX:+UseG1GC", "-XX:+UseG1GC"); - verifyJvmGCOptions(null,"-XX:+UseConcMarkSweepGC", "-XX:+UseConcMarkSweepGC"); + verifyJvmGCOptions(false, Zone.defaultZone(),ContainerCluster.G1GC); + verifyJvmGCOptions(true, Zone.defaultZone(), ContainerCluster.G1GC); } } diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/ModelContextImpl.java b/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/ModelContextImpl.java index 3a2bc9f1a81..c925157b980 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/ModelContextImpl.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/ModelContextImpl.java @@ -154,7 +154,6 @@ public class ModelContextImpl implements ModelContext { private final double threadPoolSizeFactor; private final double queueSizefactor; private final String docprocLoadBalancerType; - private final String jvmGCOPtions; private final Optional athenzDomain; private final Optional applicationRoles; private final int jdiscHealthCheckProxyClientTimeout; @@ -198,8 +197,6 @@ public class ModelContextImpl implements ModelContext { .with(FetchVector.Dimension.APPLICATION_ID, applicationId.serializedForm()).value(); docprocLoadBalancerType = Flags.DOCPROC_LOADBALANCER_TYPE.bindTo(flagSource) .with(FetchVector.Dimension.APPLICATION_ID, applicationId.serializedForm()).value(); - jvmGCOPtions = Flags.JVM_GC_OPTIONS.bindTo(flagSource) - .with(FetchVector.Dimension.APPLICATION_ID, applicationId.serializedForm()).value(); this.athenzDomain = athenzDomain; this.applicationRoles = applicationRoles; jdiscHealthCheckProxyClientTimeout = Flags.JDISC_HEALTH_CHECK_PROXY_CLIENT_TIMEOUT.bindTo(flagSource) @@ -283,8 +280,6 @@ public class ModelContextImpl implements ModelContext { } @Override public Duration jdiscHealthCheckProxyClientTimeout() { return Duration.ofMillis(jdiscHealthCheckProxyClientTimeout); } - @Override public String jvmGCOptions() { return jvmGCOPtions; } - } } diff --git a/flags/src/main/java/com/yahoo/vespa/flags/Flags.java b/flags/src/main/java/com/yahoo/vespa/flags/Flags.java index 30383393e97..a9ded0f5fd2 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/Flags.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/Flags.java @@ -152,11 +152,6 @@ public class Flags { "Selects what kind of load balancer to use for document processing {'adaptive', 'legacy' ''}", "Takes effect at redeployment", ZONE_ID, APPLICATION_ID); - public static final UnboundStringFlag JVM_GC_OPTIONS = defineStringFlag( - "jvm-gc-options", "", - "Sets deafult jvm gc options", - "Takes effect at redeployment", - ZONE_ID, APPLICATION_ID); public static final UnboundBooleanFlag USE_DISTRIBUTOR_BTREE_DB = defineFeatureFlag( "use-distributor-btree-db", false, -- cgit v1.2.3