diff options
118 files changed, 1690 insertions, 1272 deletions
diff --git a/bundle-plugin/src/test/java/com/yahoo/container/plugin/osgi/ExportPackageParserTest.java b/bundle-plugin/src/test/java/com/yahoo/container/plugin/osgi/ExportPackageParserTest.java index d869b8ec4d9..45f2ef54f7f 100644 --- a/bundle-plugin/src/test/java/com/yahoo/container/plugin/osgi/ExportPackageParserTest.java +++ b/bundle-plugin/src/test/java/com/yahoo/container/plugin/osgi/ExportPackageParserTest.java @@ -5,6 +5,7 @@ import com.yahoo.container.plugin.osgi.ExportPackages.Export; import com.yahoo.container.plugin.osgi.ExportPackages.Parameter; import org.hamcrest.Description; import org.hamcrest.TypeSafeMatcher; +import org.junit.Ignore; import org.junit.Test; import java.util.List; @@ -102,6 +103,8 @@ public class ExportPackageParserTest { assertThat(export.getParameters(), contains(parameterMatching(versionParameter))); } + // TODO: MAVEN_OPTS are not propagated by the maven-surefire-plugin. Either try to fix the underlying problem or set -Xss in plugin config. + @Ignore // Frequently causes StackOverflowError @Test public void require_that_long_string_literals_do_not_cause_stack_overflow_error() { //From jersey-server-1.13.jar diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/SetNodeStateRequest.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/SetNodeStateRequest.java index 4d6738940a8..eecdcc75228 100644 --- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/SetNodeStateRequest.java +++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/SetNodeStateRequest.java @@ -37,6 +37,7 @@ public class SetNodeStateRequest extends Request<SetResponse> { private final SetUnitStateRequest.ResponseWait responseWait; private final WantedStateSetter wantedState; private final TimeBudget timeBudget; + private final boolean probe; public SetNodeStateRequest(Id.Node id, SetUnitStateRequest setUnitStateRequest) { this(id, setUnitStateRequest, SetNodeStateRequest::setWantedState); @@ -51,6 +52,7 @@ public class SetNodeStateRequest extends Request<SetResponse> { this.responseWait = setUnitStateRequest.getResponseWait(); this.wantedState = wantedState; this.timeBudget = setUnitStateRequest.timeBudget(); + this.probe = setUnitStateRequest.isProbe(); } @Override @@ -61,7 +63,8 @@ public class SetNodeStateRequest extends Request<SetResponse> { newStates, id.getNode(), context.nodeStateOrHostInfoChangeHandler, - context.currentConsolidatedState); + context.currentConsolidatedState, + probe); } static NodeState getRequestedNodeState(Map<String, UnitState> newStates, Node n) throws StateRestApiException { @@ -100,7 +103,8 @@ public class SetNodeStateRequest extends Request<SetResponse> { Map<String, UnitState> newStates, Node node, NodeStateOrHostInfoChangeHandler stateListener, - ClusterState currentClusterState) throws StateRestApiException { + ClusterState currentClusterState, + boolean probe) throws StateRestApiException { if ( ! cluster.hasConfiguredNode(node.getIndex())) { throw new MissingIdException(cluster.getName(), node); } @@ -126,7 +130,8 @@ public class SetNodeStateRequest extends Request<SetResponse> { condition, nodeInfo, cluster, - stateListener); + stateListener, + probe); // If the state was successfully set, just return an "ok" message back. String reason = success ? "ok" : result.getReason(); @@ -143,9 +148,10 @@ public class SetNodeStateRequest extends Request<SetResponse> { SetUnitStateRequest.Condition condition, NodeInfo nodeInfo, ContentCluster cluster, - NodeStateOrHostInfoChangeHandler stateListener) { + NodeStateOrHostInfoChangeHandler stateListener, + boolean probe) { if (result.settingWantedStateIsAllowed()) { - setNewWantedState(nodeInfo, newWantedState, stateListener); + setNewWantedState(nodeInfo, newWantedState, stateListener, probe); } // True if the wanted state was or has just been set to newWantedState @@ -156,7 +162,7 @@ public class SetNodeStateRequest extends Request<SetResponse> { // of the distributor. E.g. setting the storage node to maintenance may cause // feeding issues unless distributor is also set down. - setDistributorWantedState(cluster, nodeInfo.getNodeIndex(), newWantedState, stateListener); + setDistributorWantedState(cluster, nodeInfo.getNodeIndex(), newWantedState, stateListener, probe); } return success; @@ -169,7 +175,8 @@ public class SetNodeStateRequest extends Request<SetResponse> { private static void setDistributorWantedState(ContentCluster cluster, int index, NodeState newStorageWantedState, - NodeStateOrHostInfoChangeHandler stateListener) { + NodeStateOrHostInfoChangeHandler stateListener, + boolean probe) { Node distributorNode = new Node(NodeType.DISTRIBUTOR, index); NodeInfo nodeInfo = cluster.getNodeInfo(distributorNode); if (nodeInfo == null) { @@ -200,13 +207,15 @@ public class SetNodeStateRequest extends Request<SetResponse> { if (newWantedState.getState() != currentWantedState.getState() || !Objects.equals(newWantedState.getDescription(), currentWantedState.getDescription())) { - setNewWantedState(nodeInfo, newWantedState, stateListener); + setNewWantedState(nodeInfo, newWantedState, stateListener, probe); } } private static void setNewWantedState(NodeInfo nodeInfo, NodeState newWantedState, - NodeStateOrHostInfoChangeHandler stateListener) { + NodeStateOrHostInfoChangeHandler stateListener, + boolean probe) { + if (probe) return; nodeInfo.setWantedState(newWantedState); stateListener.handleNewWantedNodeState(nodeInfo, newWantedState); } diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/SetNodeStatesForClusterRequest.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/SetNodeStatesForClusterRequest.java index b4d189bcd55..d7820722887 100644 --- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/SetNodeStatesForClusterRequest.java +++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/SetNodeStatesForClusterRequest.java @@ -27,6 +27,7 @@ public class SetNodeStatesForClusterRequest extends Request<SetResponse> { private final Map<String, UnitState> newStates; private final SetUnitStateRequest.Condition condition; private final TimeBudget timeBudget; + private final boolean probe; public SetNodeStatesForClusterRequest(Id.Cluster cluster, SetUnitStateRequest request) { @@ -35,6 +36,7 @@ public class SetNodeStatesForClusterRequest extends Request<SetResponse> { this.newStates = request.getNewState(); this.condition = request.getCondition(); this.timeBudget = request.timeBudget(); + this.probe = request.isProbe(); } @Override @@ -69,7 +71,8 @@ public class SetNodeStatesForClusterRequest extends Request<SetResponse> { newStates, node, context.nodeStateOrHostInfoChangeHandler, - context.currentConsolidatedState); + context.currentConsolidatedState, + probe); if (!setResponse.getWasModified()) { throw new InternalFailure("We have not yet implemented the meaning of " + diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/WantedStateSetter.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/WantedStateSetter.java index 6fa7d536c67..c3090a5e832 100644 --- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/WantedStateSetter.java +++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/WantedStateSetter.java @@ -22,5 +22,6 @@ public interface WantedStateSetter { Map<String, UnitState> newStates, Node node, NodeStateOrHostInfoChangeHandler stateListener, - ClusterState currentClusterState) throws StateRestApiException; + ClusterState currentClusterState, + boolean probe) throws StateRestApiException; } diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/restapiv2/SetNodeStateTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/restapiv2/SetNodeStateTest.java index f3a4be5ac2f..6cf4b7989e7 100644 --- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/restapiv2/SetNodeStateTest.java +++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/restapiv2/SetNodeStateTest.java @@ -33,6 +33,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.mockito.Matchers.any; +import static org.mockito.Matchers.anyBoolean; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -46,6 +47,7 @@ public class SetNodeStateTest extends StateRestApiTest { private Condition condition = Condition.FORCE; private ResponseWait responseWait = ResponseWait.WAIT_UNTIL_CLUSTER_ACKED; private TimeBudget timeBudget = TimeBudget.fromNow(Clock.systemUTC(), Duration.ofSeconds(10)); + private boolean probe = false; public SetUnitStateRequestImpl(String req) { super(req, 0); @@ -98,6 +100,11 @@ public class SetNodeStateTest extends StateRestApiTest { public TimeBudget timeBudget() { return timeBudget; } + + @Override + public boolean isProbe() { + return probe; + } } private void verifyStateSet(String state, String reason) throws Exception { @@ -458,7 +465,7 @@ public class SetNodeStateTest extends StateRestApiTest { new SetUnitStateRequestImpl("music/storage/1").setNewState("user", "maintenance", "whatever reason."), wantedStateSetter); SetResponse response = new SetResponse("some reason", wasModified); - when(wantedStateSetter.set(any(), any(), any(), any(), any(), any())).thenReturn(response); + when(wantedStateSetter.set(any(), any(), any(), any(), any(), any(), anyBoolean())).thenReturn(response); RemoteClusterControllerTask.Context context = mock(RemoteClusterControllerTask.Context.class); MasterInterface masterInterface = mock(MasterInterface.class); diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/SetNodeStateRequestTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/SetNodeStateRequestTest.java index 9239b8774b0..7161fb1be79 100644 --- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/SetNodeStateRequestTest.java +++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/restapiv2/requests/SetNodeStateRequestTest.java @@ -23,20 +23,23 @@ import java.util.Map; import java.util.Optional; import static org.mockito.Matchers.any; +import static org.mockito.Matchers.eq; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; public class SetNodeStateRequestTest { - public static final String REASON = "operator"; - ContentCluster cluster = mock(ContentCluster.class); - SetUnitStateRequest.Condition condition = SetUnitStateRequest.Condition.SAFE; - Map<String, UnitState> newStates = new HashMap<>(); - UnitState unitState = mock(UnitState.class); + private static final String REASON = "operator"; + private ContentCluster cluster = mock(ContentCluster.class); + private SetUnitStateRequest.Condition condition = SetUnitStateRequest.Condition.SAFE; + private Map<String, UnitState> newStates = new HashMap<>(); + private UnitState unitState = mock(UnitState.class); private final int NODE_INDEX = 2; - Node storageNode = new Node(NodeType.STORAGE, NODE_INDEX); - NodeStateOrHostInfoChangeHandler stateListener = mock(NodeStateOrHostInfoChangeHandler.class); - ClusterState currentClusterState = mock(ClusterState.class); + private Node storageNode = new Node(NodeType.STORAGE, NODE_INDEX); + private NodeStateOrHostInfoChangeHandler stateListener = mock(NodeStateOrHostInfoChangeHandler.class); + private ClusterState currentClusterState = mock(ClusterState.class); + private boolean probe = false; @Before public void setUp() { @@ -53,6 +56,16 @@ public class SetNodeStateRequestTest { } @Test + public void testProbingDoesntChangeState() throws StateRestApiException { + probe = true; + testSetStateRequest( + "maintenance", + State.UP, State.UP, + NodeStateChangeChecker.Result.allowSettingOfWantedState(), + Optional.empty(), Optional.empty()); + } + + @Test public void testUpToDown() throws StateRestApiException { testSetStateRequest( "down", @@ -124,6 +137,9 @@ public class SetNodeStateRequestTest { when(cluster.getNodeInfo(distributorNode)).thenReturn(distributorNodeInfo); NodeState distributorNodeState = new NodeState(distributorNode.getType(), distributorWantedState); + if (distributorWantedState != State.UP) { + distributorNodeState.setDescription(REASON); + } when(distributorNodeInfo.getUserWantedState()).thenReturn(distributorNodeState); setWantedState(); @@ -133,6 +149,9 @@ public class SetNodeStateRequestTest { new NodeState(NodeType.STORAGE, expectedNewStorageWantedState.get()); verify(storageNodeInfo).setWantedState(expectedNewStorageNodeState); verify(stateListener).handleNewWantedNodeState(storageNodeInfo, expectedNewStorageNodeState); + } else { + verify(storageNodeInfo, times(0)).setWantedState(any()); + verify(stateListener, times(0)).handleNewWantedNodeState(eq(storageNodeInfo), any()); } if (expectedNewDistributorWantedState.isPresent()) { @@ -140,6 +159,9 @@ public class SetNodeStateRequestTest { new NodeState(NodeType.DISTRIBUTOR, expectedNewDistributorWantedState.get()); verify(distributorNodeInfo).setWantedState(expectedNewDistributorNodeState); verify(stateListener).handleNewWantedNodeState(distributorNodeInfo, expectedNewDistributorNodeState); + } else { + verify(distributorNodeInfo, times(0)).setWantedState(any()); + verify(stateListener, times(0)).handleNewWantedNodeState(eq(distributorNodeInfo), any()); } } @@ -150,6 +172,7 @@ public class SetNodeStateRequestTest { newStates, storageNode, stateListener, - currentClusterState); + currentClusterState, + probe); } }
\ No newline at end of file diff --git a/clustercontroller-utils/src/main/java/com/yahoo/vespa/clustercontroller/utils/staterestapi/requests/SetUnitStateRequest.java b/clustercontroller-utils/src/main/java/com/yahoo/vespa/clustercontroller/utils/staterestapi/requests/SetUnitStateRequest.java index a28ddb3539b..27f18c3664b 100644 --- a/clustercontroller-utils/src/main/java/com/yahoo/vespa/clustercontroller/utils/staterestapi/requests/SetUnitStateRequest.java +++ b/clustercontroller-utils/src/main/java/com/yahoo/vespa/clustercontroller/utils/staterestapi/requests/SetUnitStateRequest.java @@ -64,4 +64,7 @@ public interface SetUnitStateRequest extends UnitRequest { ResponseWait getResponseWait(); TimeBudget timeBudget(); + + /** A probe request is a non-committal request to see if an identical (but non-probe) request would have succeeded. */ + boolean isProbe(); } diff --git a/clustercontroller-utils/src/main/java/com/yahoo/vespa/clustercontroller/utils/staterestapi/server/JsonReader.java b/clustercontroller-utils/src/main/java/com/yahoo/vespa/clustercontroller/utils/staterestapi/server/JsonReader.java index d871a8ed6bc..dab6895cc9d 100644 --- a/clustercontroller-utils/src/main/java/com/yahoo/vespa/clustercontroller/utils/staterestapi/server/JsonReader.java +++ b/clustercontroller-utils/src/main/java/com/yahoo/vespa/clustercontroller/utils/staterestapi/server/JsonReader.java @@ -33,13 +33,16 @@ public class JsonReader { } static class SetRequestData { + final boolean probe; final Map<String, UnitState> stateMap; final SetUnitStateRequest.Condition condition; final SetUnitStateRequest.ResponseWait responseWait; - public SetRequestData(Map<String, UnitState> stateMap, + public SetRequestData(boolean probe, + Map<String, UnitState> stateMap, SetUnitStateRequest.Condition condition, SetUnitStateRequest.ResponseWait responseWait) { + this.probe = probe; this.stateMap = stateMap; this.condition = condition; this.responseWait = responseWait; @@ -49,8 +52,9 @@ public class JsonReader { public SetRequestData getStateRequestData(HttpRequest request) throws Exception { JSONObject json = new JSONObject(request.getPostContent().toString()); - final SetUnitStateRequest.Condition condition; + final boolean probe = json.has("probe") && json.getBoolean("probe"); + final SetUnitStateRequest.Condition condition; if (json.has("condition")) { condition = SetUnitStateRequest.Condition.fromString(json.getString("condition")); } else { @@ -100,6 +104,6 @@ public class JsonReader { stateMap.put(type, new UnitStateImpl(code, reason)); } - return new SetRequestData(stateMap, condition, responseWait); + return new SetRequestData(probe, stateMap, condition, responseWait); } } diff --git a/clustercontroller-utils/src/main/java/com/yahoo/vespa/clustercontroller/utils/staterestapi/server/RestApiHandler.java b/clustercontroller-utils/src/main/java/com/yahoo/vespa/clustercontroller/utils/staterestapi/server/RestApiHandler.java index c38f7aec8c6..46f5d964245 100644 --- a/clustercontroller-utils/src/main/java/com/yahoo/vespa/clustercontroller/utils/staterestapi/server/RestApiHandler.java +++ b/clustercontroller-utils/src/main/java/com/yahoo/vespa/clustercontroller/utils/staterestapi/server/RestApiHandler.java @@ -97,6 +97,8 @@ public class RestApiHandler implements HttpRequestHandler { public ResponseWait getResponseWait() { return setRequestData.responseWait; } @Override public TimeBudget timeBudget() { return TimeBudget.from(clock, start, timeout); } + @Override + public boolean isProbe() { return setRequestData.probe; } }); return new JsonHttpResult().setJson(jsonWriter.createJson(setResponse)); } diff --git a/config-model/src/main/java/com/yahoo/vespa/model/AbstractService.java b/config-model/src/main/java/com/yahoo/vespa/model/AbstractService.java index fcd9df309a4..60a49598c42 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/AbstractService.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/AbstractService.java @@ -47,9 +47,9 @@ public abstract class AbstractService extends AbstractConfigProducer<AbstractCon /** The ports allocated to this Service. */ private List<Integer> ports = new ArrayList<>(); - /** The optional JVM execution args for this Service. */ + /** The optional JVM execution options for this Service. */ // Please keep non-null, as passed to command line in service startup - private String jvmArgs = ""; + private String jvmOptions = ""; /** The optional PRELOAD libraries for this Service. */ // Please keep non-null, as passed to command line in service startup @@ -399,23 +399,23 @@ public abstract class AbstractService extends AbstractConfigProducer<AbstractCon } /** Optional execution args for this service */ - public String getJvmArgs() { - return jvmArgs; + public String getJvmOptions() { + return jvmOptions; } - public void setJvmArgs(String args) { - jvmArgs = (args == null) ? "" : args; + public void setJvmOptions(String args) { + jvmOptions = (args == null) ? "" : args; } - public void appendJvmArgs(String args) { + public void appendJvmOptions(String args) { if ((args != null) && ! "".equals(args)) { - setJvmArgs(jvmArgs + getSeparator(jvmArgs) + args); + setJvmOptions(jvmOptions + getSeparator(jvmOptions) + args); } } private static String getSeparator(String current) { return ("".equals(current)) ? "" : " "; } - public void prependJvmArgs(String args) { + public void prependJvmOptions(String args) { if ((args != null) && ! "".equals(args)) { - setJvmArgs(args + getSeparator(jvmArgs) + jvmArgs); + setJvmOptions(args + getSeparator(jvmOptions) + jvmOptions); } } public String getPreLoad() { return preload; } diff --git a/config-model/src/main/java/com/yahoo/vespa/model/Service.java b/config-model/src/main/java/com/yahoo/vespa/model/Service.java index 29ec26b06d2..d5d33a08b5d 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/Service.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/Service.java @@ -98,8 +98,8 @@ public interface Service extends ConfigProducer { */ String getHostName(); - /** Optional JVM execution args for this service */ - String getJvmArgs(); + /** Optional JVM execution options for this service */ + String getJvmOptions(); /** * Computes and returns the i'th port for this service, based on diff --git a/config-model/src/main/java/com/yahoo/vespa/model/admin/Logserver.java b/config-model/src/main/java/com/yahoo/vespa/model/admin/Logserver.java index 8b98dc9d06a..c354445b690 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/admin/Logserver.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/admin/Logserver.java @@ -29,7 +29,7 @@ public class Logserver extends AbstractService { * @return the startup command for the logserver */ public String getStartupCommand() { - return "exec $ROOT/bin/vespa-logserver-start " + getMyJVMArgs() + " " + getJvmArgs(); + return "exec $ROOT/bin/vespa-logserver-start " + getMyJVMArgs() + " " + getJvmOptions(); } /** diff --git a/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/DomV20ClientsBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/DomV20ClientsBuilder.java index 01dd6495d13..6a02619bdb4 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/DomV20ClientsBuilder.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/DomV20ClientsBuilder.java @@ -75,7 +75,7 @@ public class DomV20ClientsBuilder { int index = clients.getVespaSpoolers().size(); VespaSpoolerService spoolerService = new VespaSpoolerServiceBuilder(index, new VespaSpooler(feederConfig, spoolConfig)). build(deployState, spoolerCfg, e); - if ("".equals(spoolerService.getJvmArgs()) && jvmArgs!=null) spoolerService.setJvmArgs(jvmArgs); + if ("".equals(spoolerService.getJvmOptions()) && jvmArgs!=null) spoolerService.setJvmOptions(jvmArgs); spoolerService.setProp("index", String.valueOf(index)); clients.getVespaSpoolers().add(spoolerService); } else { diff --git a/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/VespaDomBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/VespaDomBuilder.java index 1f6f7ad6c69..a6d3809ff64 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/VespaDomBuilder.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/VespaDomBuilder.java @@ -44,7 +44,8 @@ import java.util.logging.Logger; public class VespaDomBuilder extends VespaModelBuilder { public static final String JVMARGS_ATTRIB_NAME = "jvmargs"; - public static final String GCOPTS_ATTRIB_NAME = "gcopts"; + public static final String JVM_OPTIONS = "jvm-options"; + public static final String JVM_GC_OPTIONS = "jvm-gc-options"; public static final String PRELOAD_ATTRIB_NAME = "preload"; // Intended for vespa engineers public static final String MMAP_NOCORE_LIMIT = "mmap-core-limit"; // Intended for vespa engineers public static final String CORE_ON_OOM = "core-on-oom"; // Intended for vespa engineers @@ -143,8 +144,12 @@ public class VespaDomBuilder extends VespaModelBuilder { { initializeProducer(t, deployState, producerSpec); if (producerSpec != null) { - if (producerSpec.hasAttribute(JVMARGS_ATTRIB_NAME)) { - t.appendJvmArgs(producerSpec.getAttribute(JVMARGS_ATTRIB_NAME)); + if (producerSpec.hasAttribute(JVM_OPTIONS)) { + t.appendJvmOptions(producerSpec.getAttribute(JVM_OPTIONS)); + } else { + if (producerSpec.hasAttribute(JVMARGS_ATTRIB_NAME)) { + t.appendJvmOptions(producerSpec.getAttribute(JVMARGS_ATTRIB_NAME)); + } } if (producerSpec.hasAttribute(PRELOAD_ATTRIB_NAME)) { t.setPreLoad(producerSpec.getAttribute(PRELOAD_ATTRIB_NAME)); diff --git a/config-model/src/main/java/com/yahoo/vespa/model/clients/VespaSpoolerService.java b/config-model/src/main/java/com/yahoo/vespa/model/clients/VespaSpoolerService.java index f4287cf982e..378c85dc325 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/clients/VespaSpoolerService.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/clients/VespaSpoolerService.java @@ -26,7 +26,7 @@ public class VespaSpoolerService extends AbstractService implements SpoolerConfi } public String getStartupCommand() { - return "exec vespaspooler "+getJvmArgs(); + return "exec vespaspooler "+ getJvmOptions(); } @Override diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/Container.java b/config-model/src/main/java/com/yahoo/vespa/model/container/Container.java index ab65c68dea4..e098263119c 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/Container.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/Container.java @@ -295,7 +295,7 @@ public class Container extends AbstractService implements } public String getStartupCommand() { - return "PRELOAD=" + getPreLoad() + " exec vespa-start-container-daemon " + getJvmArgs() + " "; + return "PRELOAD=" + getPreLoad() + " exec vespa-start-container-daemon " + getJvmOptions() + " "; } @Override @@ -315,15 +315,15 @@ public class Container extends AbstractService implements /** Returns the jvm arguments this should start with */ @Override - public String getJvmArgs() { - String jvmArgs = super.getJvmArgs(); + public String getJvmOptions() { + String jvmArgs = super.getJvmOptions(); return isHostedVespa && hasDocproc() ? ("".equals(jvmArgs) ? defaultHostedJVMArgs : defaultHostedJVMArgs + " " + jvmArgs) : jvmArgs; } /** Returns the jvm args set explicitly for this node */ - public String getAssignedJvmArgs() { return super.getJvmArgs(); } + public String getAssignedJvmOptions() { return super.getJvmOptions(); } private String serviceSlobrokId() { return "vespa/service/" + getConfigId(); diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/ContainerCluster.java b/config-model/src/main/java/com/yahoo/vespa/model/container/ContainerCluster.java index ccd828b5f48..29a758fee74 100755 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/ContainerCluster.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/ContainerCluster.java @@ -14,9 +14,6 @@ import com.yahoo.config.docproc.SchemamappingConfig; import com.yahoo.config.model.ApplicationConfigProducerRoot; import com.yahoo.config.model.deploy.DeployState; import com.yahoo.config.model.producer.AbstractConfigProducer; -import com.yahoo.config.provision.Environment; -import com.yahoo.config.provision.RegionName; -import com.yahoo.config.provision.SystemName; import com.yahoo.config.provision.Zone; import com.yahoo.container.BundlesConfig; import com.yahoo.container.ComponentsConfig; @@ -144,7 +141,7 @@ public final class ContainerCluster public static final String STATISTICS_HANDLER_CLASS = "com.yahoo.container.config.StatisticsRequestHandler"; public static final String SIMPLE_LINGUISTICS_PROVIDER = "com.yahoo.language.provider.SimpleLinguisticsProvider"; public static final String CMS = "-XX:+UseConcMarkSweepGC -XX:MaxTenuringThreshold=15 -XX:NewRatio=1"; - static final String G1GC = "-XX:+UseG1GC -XX:MaxTenuringThreshold=15"; + public static final String G1GC = "-XX:+UseG1GC -XX:MaxTenuringThreshold=15"; public static final String ROOT_HANDLER_BINDING = "*://*/"; @@ -186,7 +183,7 @@ public final class ContainerCluster private Zone zone; private String hostClusterId = null; - private String gcopts = null; + private String jvmGCOptions = null; private Integer memoryPercentage = null; private static class AcceptAllVerifier implements ContainerClusterVerifier { @@ -631,20 +628,6 @@ public final class ContainerCluster if (containerSearch!=null) containerSearch.getConfig(builder); } - private String buildGCOpts(Zone zone) { - Optional<String> gcopts = getGCOpts(); - if (gcopts.isPresent()) { - return gcopts.get(); - } else if (zone.system() == SystemName.dev) { - return G1GC; - } else if (isHostedVespa()) { - return ((zone.environment() != Environment.prod) || RegionName.from("us-east-3").equals(zone.region())) - ? G1GC : CMS; - } else { - return CMS; - } - } - @Override public void getConfig(QrStartConfig.Builder builder) { QrStartConfig.Jvm.Builder jvmBuilder = new QrStartConfig.Jvm.Builder(); @@ -656,7 +639,9 @@ public final class ContainerCluster if (containerSearch!=null) { jvmBuilder.directMemorySizeCache(containerSearch.totalCacheSizeMb()); } - jvmBuilder.gcopts(buildGCOpts(getZone())); + if (jvmGCOptions != null) { + jvmBuilder.gcopts(jvmGCOptions); + } builder.jvm(jvmBuilder); } @@ -814,8 +799,8 @@ public final class ContainerCluster public Optional<String> getHostClusterId() { return Optional.ofNullable(hostClusterId); } public void setMemoryPercentage(Integer memoryPercentage) { this.memoryPercentage = memoryPercentage; } - public void setGCOpts(String gcopts) { this.gcopts = gcopts; } - public Optional<String> getGCOpts() { return Optional.ofNullable(gcopts); } + public void setJvmGCOptions(String opts) { this.jvmGCOptions = opts; } + public Optional<String> getJvmGCOptions() { return Optional.ofNullable(jvmGCOptions); } /** * Returns the percentage of host physical memory this application has specified for nodes in this cluster, diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/component/ConfigProducerGroup.java b/config-model/src/main/java/com/yahoo/vespa/model/container/component/ConfigProducerGroup.java index e8142999433..66294d3fcef 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/component/ConfigProducerGroup.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/component/ConfigProducerGroup.java @@ -20,10 +20,10 @@ public class ConfigProducerGroup<CHILD extends AbstractConfigProducer<?>> extend } public void addComponent(ComponentId id, CHILD producer) { - boolean wasAdded = producerById.put(id, producer) == null; - if (!wasAdded) { - throw new IllegalArgumentException("Two entities have the same component id '" + - id + "' in the same scope."); + CHILD existing = producerById.put(id, producer); + if ( existing != null) { + throw new IllegalArgumentException("Both " + producer + " and " + existing + " are configured" + + " with the id '" + id + "'. All components must have a unique id."); } addChild(producer); } diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/component/chain/Chain.java b/config-model/src/main/java/com/yahoo/vespa/model/container/component/chain/Chain.java index f795e481f62..6b4f8c8f8b5 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/component/chain/Chain.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/component/chain/Chain.java @@ -78,4 +78,9 @@ public class Chain<T extends ChainedComponent<?>> extends AbstractConfigProducer return TYPE; } + @Override + public String toString() { + return "chain '" + componentId + "'"; + } + } diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/search/searchchain/SearchChain.java b/config-model/src/main/java/com/yahoo/vespa/model/container/search/searchchain/SearchChain.java index 2605736e23b..ff211264a34 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/search/searchchain/SearchChain.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/search/searchchain/SearchChain.java @@ -28,4 +28,9 @@ public class SearchChain extends Chain<Searcher<?>> { return Collections.emptyList(); } + @Override + public String toString() { + return "search chain '" + getId() + "'"; + } + } 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 ac1f313d983..91df3fee6e8 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 @@ -21,7 +21,9 @@ import com.yahoo.config.provision.ClusterSpec; import com.yahoo.config.provision.Environment; import com.yahoo.config.provision.HostName; import com.yahoo.config.provision.NodeType; +import com.yahoo.config.provision.RegionName; import com.yahoo.config.provision.Rotation; +import com.yahoo.config.provision.SystemName; import com.yahoo.config.provision.Zone; import com.yahoo.container.jdisc.config.MetricDefaultsConfig; import com.yahoo.search.rendering.RendererRegistry; @@ -456,6 +458,37 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> { Pattern cmsArgs = Pattern.compile("-XX:[-+]*CMS"); return (gcAlgorithm.matcher(jvmargs).find() ||cmsArgs.matcher(jvmargs).find()); } + + private static String buildJvmGCOptions(Zone zone, String jvmGCOPtions, boolean isHostedVespa) { + if (jvmGCOPtions != null) { + return jvmGCOPtions; + } else if (zone.system() == SystemName.dev) { + return ContainerCluster.G1GC; + } else if (isHostedVespa) { + return ((zone.environment() != Environment.prod) || RegionName.from("us-east-3").equals(zone.region())) + ? ContainerCluster.G1GC : ContainerCluster.CMS; + } else { + return ContainerCluster.CMS; + } + } + private String getJvmOptions(ContainerCluster cluster, Element nodesElement, DeployLogger deployLogger) { + String jvmOptions = ""; + if (nodesElement.hasAttribute(VespaDomBuilder.JVM_OPTIONS)) { + jvmOptions = nodesElement.getAttribute(VespaDomBuilder.JVM_OPTIONS); + if (nodesElement.hasAttribute(VespaDomBuilder.JVMARGS_ATTRIB_NAME)) { + String jvmArgs = nodesElement.getAttribute(VespaDomBuilder.JVMARGS_ATTRIB_NAME); + throw new IllegalArgumentException("You have specified both jvm-options='" + jvmOptions + "'" + + " and deprecated jvmargs='" + jvmArgs + "'. Merge jvmargs into jvm-options."); + } + } else { + jvmOptions = nodesElement.getAttribute(VespaDomBuilder.JVMARGS_ATTRIB_NAME); + if (incompatibleGCOptions(jvmOptions)) { + deployLogger.log(Level.WARNING, "You need to move out your GC related options from 'jvmargs' to 'jvm-gc-options'"); + cluster.setJvmGCOptions(ContainerCluster.CMS); + } + } + return jvmOptions; + } private void addNodesFromXml(ContainerCluster cluster, Element containerElement, ConfigModelContext context) { Element nodesElement = XML.getChild(containerElement, "nodes"); if (nodesElement == null) { // default single node on localhost @@ -464,19 +497,17 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> { node.setHostResource(host); node.initService(context.getDeployLogger()); cluster.addContainers(Collections.singleton(node)); - } - else { + } else { List<Container> nodes = createNodes(cluster, nodesElement, context); - String jvmArgs = nodesElement.getAttribute(VespaDomBuilder.JVMARGS_ATTRIB_NAME); - String gcopts = nodesElement.hasAttribute(VespaDomBuilder.GCOPTS_ATTRIB_NAME) - ? nodesElement.getAttribute(VespaDomBuilder.GCOPTS_ATTRIB_NAME) - : null; - if (incompatibleGCOptions(jvmArgs)) { - context.getDeployLogger().log(Level.WARNING, "You need to move out your GC related options from 'jvmargs' to 'gcopts'"); - } else { - cluster.setGCOpts(gcopts); + applyNodesTagJvmArgs(nodes, getJvmOptions(cluster, nodesElement, context.getDeployLogger())); + + if ( !cluster.getJvmGCOptions().isPresent()) { + 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())); } - applyNodesTagJvmArgs(nodes, jvmArgs); + applyRoutingAliasProperties(nodes, cluster); applyDefaultPreload(nodes, nodesElement); applyMemoryPercentage(cluster, nodesElement.getAttribute(VespaDomBuilder.Allocated_MEMORY_ATTRIB_NAME)); @@ -677,8 +708,8 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> { private void applyNodesTagJvmArgs(List<Container> containers, String jvmArgs) { for (Container container: containers) { - if (container.getAssignedJvmArgs().isEmpty()) - container.prependJvmArgs(jvmArgs); + if (container.getAssignedJvmOptions().isEmpty()) + container.prependJvmOptions(jvmArgs); } } diff --git a/config-model/src/main/resources/schema/common.rnc b/config-model/src/main/resources/schema/common.rnc index 6a82556f01b..73882da2b01 100644 --- a/config-model/src/main/resources/schema/common.rnc +++ b/config-model/src/main/resources/schema/common.rnc @@ -2,7 +2,8 @@ service.attlist &= attribute hostalias { xsd:NCName } service.attlist &= attribute baseport { xsd:unsignedShort }? service.attlist &= attribute jvmargs { text }? -service.attlist &= attribute gcopts { text }? +service.attlist &= attribute jvm-options { text }? +service.attlist &= attribute jvm-gc-options { text }? # preload is for internal use only service.attlist &= attribute preload { text }? diff --git a/config-model/src/main/resources/schema/containercluster.rnc b/config-model/src/main/resources/schema/containercluster.rnc index 4862fdf7a50..5dbcffce736 100644 --- a/config-model/src/main/resources/schema/containercluster.rnc +++ b/config-model/src/main/resources/schema/containercluster.rnc @@ -211,7 +211,8 @@ DocumentApi = element document-api { NodesOfContainerCluster = element nodes { attribute jvmargs { text }? & - attribute gcopts { text }? & + attribute jvm-options { text }? & + attribute jvm-gc-options { text }? & attribute preload { text }? & attribute allocated-memory { text }? & attribute cpu-socket-affinity { xsd:boolean }? & diff --git a/config-model/src/test/java/com/yahoo/config/model/provision/ModelProvisioningTest.java b/config-model/src/test/java/com/yahoo/config/model/provision/ModelProvisioningTest.java index 8396cac265c..f9585224bd6 100644 --- a/config-model/src/test/java/com/yahoo/config/model/provision/ModelProvisioningTest.java +++ b/config-model/src/test/java/com/yahoo/config/model/provision/ModelProvisioningTest.java @@ -86,7 +86,7 @@ public class ModelProvisioningTest { " <handler id='myHandler'>" + " <component id='injected' />" + " </handler>" + - " <nodes count='2' allocated-memory='45%' gcopts='-XX:+UseParNewGC' jvmargs='-verbosegc' preload='lib/blablamalloc.so'/>" + + " <nodes count='2' allocated-memory='45%' jvm-gc-options='-XX:+UseParNewGC' jvm-options='-verbosegc' preload='lib/blablamalloc.so'/>" + "</jdisc>" + "</services>"; String hosts ="<hosts>" @@ -127,20 +127,20 @@ public class ModelProvisioningTest { assertThat(mydisc2.getContainers().get(1).getConfigId(), is("mydisc2/container.1")); assertTrue(mydisc2.getContainers().get(1).isInitialized()); - assertThat(mydisc.getContainers().get(0).getJvmArgs(), is("")); - assertThat(mydisc.getContainers().get(1).getJvmArgs(), is("")); - assertThat(mydisc.getContainers().get(2).getJvmArgs(), is("")); + assertThat(mydisc.getContainers().get(0).getJvmOptions(), is("")); + assertThat(mydisc.getContainers().get(1).getJvmOptions(), is("")); + assertThat(mydisc.getContainers().get(2).getJvmOptions(), is("")); assertThat(mydisc.getContainers().get(0).getPreLoad(), is(getDefaults().underVespaHome("lib64/vespa/malloc/libvespamalloc.so"))); assertThat(mydisc.getContainers().get(1).getPreLoad(), is(getDefaults().underVespaHome("lib64/vespa/malloc/libvespamalloc.so"))); assertThat(mydisc.getContainers().get(2).getPreLoad(), is(getDefaults().underVespaHome("lib64/vespa/malloc/libvespamalloc.so"))); assertThat(mydisc.getMemoryPercentage(), is(Optional.empty())); - assertThat(mydisc2.getContainers().get(0).getJvmArgs(), is("-verbosegc")); - assertThat(mydisc2.getContainers().get(1).getJvmArgs(), is("-verbosegc")); + assertThat(mydisc2.getContainers().get(0).getJvmOptions(), is("-verbosegc")); + assertThat(mydisc2.getContainers().get(1).getJvmOptions(), is("-verbosegc")); assertThat(mydisc2.getContainers().get(0).getPreLoad(), is("lib/blablamalloc.so")); assertThat(mydisc2.getContainers().get(1).getPreLoad(), is("lib/blablamalloc.so")); assertThat(mydisc2.getMemoryPercentage(), is(Optional.of(45))); - assertThat(mydisc2.getGCOpts(), is(Optional.of("-XX:+UseParNewGC"))); + assertThat(mydisc2.getJvmGCOptions(), is(Optional.of("-XX:+UseParNewGC"))); QrStartConfig.Builder qrStartBuilder = new QrStartConfig.Builder(); mydisc2.getConfig(qrStartBuilder); QrStartConfig qrsStartConfig = new QrStartConfig(qrStartBuilder); @@ -261,13 +261,13 @@ public class ModelProvisioningTest { } @Test - public void testCombinedClusterWithJvmArgs() { + public void testCombinedClusterWithJvmOptions() { String xmlWithNodes = "<?xml version='1.0' encoding='utf-8' ?>" + "<services>" + " <container version='1.0' id='container1'>" + " <document-processing/>" + - " <nodes of='content1' jvmargs='testarg'/>" + + " <nodes of='content1' jvm-options='testoption'/>" + " </container>" + " <content version='1.0' id='content1'>" + " <redundancy>2</redundancy>" + @@ -284,7 +284,7 @@ public class ModelProvisioningTest { assertEquals("Nodes in content1", 2, model.getContentClusters().get("content1").getRootGroup().getNodes().size()); assertEquals("Nodes in container1", 2, model.getContainerClusters().get("container1").getContainers().size()); for (Container container : model.getContainerClusters().get("container1").getContainers()) - assertTrue(container.getJvmArgs().contains("testarg")); + assertTrue(container.getJvmOptions().contains("testoption")); } @Test @@ -1225,6 +1225,58 @@ public class ModelProvisioningTest { } @Test + public void testJvmArgs() { + String services = + "<?xml version='1.0' encoding='utf-8' ?>\n" + + "<jdisc version='1.0'>" + + " <search/>" + + " <nodes jvmargs='xyz' count='3'/>" + + "</jdisc>"; + int numberOfHosts = 3; + VespaModelTester tester = new VespaModelTester(); + tester.addHosts(numberOfHosts); + VespaModel model = tester.createModel(services, true); + assertEquals(numberOfHosts, model.getRoot().getHostSystem().getHosts().size()); + assertEquals("xyz", model.getContainerClusters().get("jdisc").getContainers().get(0).getAssignedJvmOptions()); + } + + @Test + public void testJvmOptions() { + String services = + "<?xml version='1.0' encoding='utf-8' ?>\n" + + "<jdisc version='1.0'>" + + " <search/>" + + " <nodes jvm-options='xyz' count='3'/>" + + "</jdisc>"; + int numberOfHosts = 3; + VespaModelTester tester = new VespaModelTester(); + tester.addHosts(numberOfHosts); + VespaModel model = tester.createModel(services, true); + assertEquals(numberOfHosts, model.getRoot().getHostSystem().getHosts().size()); + assertEquals("xyz", model.getContainerClusters().get("jdisc").getContainers().get(0).getAssignedJvmOptions()); + } + + @Test + public void testJvmOptionsOverridesJvmArgs() { + String services = + "<?xml version='1.0' encoding='utf-8' ?>\n" + + "<jdisc version='1.0'>" + + " <search/>" + + " <nodes jvm-options='xyz' jvmargs='abc' count='3'/>" + + "</jdisc>"; + int numberOfHosts = 3; + VespaModelTester tester = new VespaModelTester(); + tester.addHosts(numberOfHosts); + try { + tester.createModel(services, true); + fail("Expected exception"); + } + catch (IllegalArgumentException e) { + assertEquals("You have specified both jvm-options='xyz' and deprecated jvmargs='abc'. Merge jvmargs into jvm-options.", e.getMessage()); + } + } + + @Test public void testUsingHostaliasWithProvisioner() { String services = "<?xml version='1.0' encoding='utf-8' ?>\n" + @@ -1418,7 +1470,7 @@ public class ModelProvisioningTest { " <document-processing/>\n" + " <document-api/>\n" + " <search/>\n" + - " <nodes jvmargs=\"-Xms512m -Xmx512m\">\n" + + " <nodes jvm-options=\"-Xms512m -Xmx512m\">\n" + " <node hostalias=\"vespa-1\"/>\n" + " </nodes>\n" + " </container>\n" + @@ -1477,7 +1529,7 @@ public class ModelProvisioningTest { " <document-processing/>\n" + " <document-api/>\n" + " <search/>\n" + - " <nodes jvmargs=\"-Xms512m -Xmx512m\">\n" + + " <nodes jvm-options=\"-Xms512m -Xmx512m\">\n" + " <node hostalias=\"vespa-1\"/>\n" + " <node hostalias=\"vespa-2\"/>\n" + " <node hostalias=\"vespa-3\"/>\n" + diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/ContainerClusterTest.java b/config-model/src/test/java/com/yahoo/vespa/model/container/ContainerClusterTest.java index e4fb19010f5..b4dea09010d 100755 --- a/config-model/src/test/java/com/yahoo/vespa/model/container/ContainerClusterTest.java +++ b/config-model/src/test/java/com/yahoo/vespa/model/container/ContainerClusterTest.java @@ -168,45 +168,15 @@ public class ContainerClusterTest { addContainer(root.deployLogger(), cluster, "c1", "host-c1"); assertEquals(1, cluster.getContainers().size()); Container container = cluster.getContainers().get(0); - verifyJvmArgs(isHosted, hasDocProc, "", container.getJvmArgs()); - container.setJvmArgs("initial"); - verifyJvmArgs(isHosted, hasDocProc, "initial", container.getJvmArgs()); - container.prependJvmArgs("ignored"); - verifyJvmArgs(isHosted, hasDocProc, "ignored initial", container.getJvmArgs()); - container.appendJvmArgs("override"); - verifyJvmArgs(isHosted, hasDocProc, "ignored initial override", container.getJvmArgs()); - container.setJvmArgs(null); - verifyJvmArgs(isHosted, hasDocProc, "", container.getJvmArgs()); - } - - private void verifyGCOpts(boolean isHosted, String override, Zone zone, String expected) { - MockRoot root = createRoot(isHosted, zone); - ContainerCluster cluster = createContainerCluster(root, false); - addContainer(root.deployLogger(), cluster, "c1", "host-c1"); - cluster.setGCOpts(override); - assertEquals(1, cluster.getContainers().size()); - QrStartConfig.Builder qsB = new QrStartConfig.Builder(); - cluster.getConfig(qsB); - QrStartConfig qsC= new QrStartConfig(qsB); - assertEquals(expected, qsC.jvm().gcopts()); - } - - private void verifyGCOpts(boolean isHosted, Zone zone, String expected) { - verifyGCOpts(isHosted, null, zone, expected); - verifyGCOpts(isHosted, "-XX:+UseG1GC", zone, "-XX:+UseG1GC"); - Zone DEV = new Zone(SystemName.dev, zone.environment(), zone.region()); - verifyGCOpts(isHosted, null, DEV, ContainerCluster.G1GC); - verifyGCOpts(isHosted, "-XX:+UseConcMarkSweepGC", DEV, "-XX:+UseConcMarkSweepGC"); - - } - - @Test - public void requireThatGCOptsIsHonoured() { - final Zone US_EAST_3 = new Zone(Environment.prod, RegionName.from("us-east-3")); - verifyGCOpts(false, Zone.defaultZone(),ContainerCluster.CMS); - verifyGCOpts(false, US_EAST_3, ContainerCluster.CMS); - verifyGCOpts(true, Zone.defaultZone(), ContainerCluster.CMS); - verifyGCOpts(true, US_EAST_3, ContainerCluster.G1GC); + verifyJvmArgs(isHosted, hasDocProc, "", container.getJvmOptions()); + container.setJvmOptions("initial"); + verifyJvmArgs(isHosted, hasDocProc, "initial", container.getJvmOptions()); + container.prependJvmOptions("ignored"); + verifyJvmArgs(isHosted, hasDocProc, "ignored initial", container.getJvmOptions()); + container.appendJvmOptions("override"); + verifyJvmArgs(isHosted, hasDocProc, "ignored initial override", container.getJvmOptions()); + container.setJvmOptions(null); + verifyJvmArgs(isHosted, hasDocProc, "", container.getJvmOptions()); } @Test @@ -289,10 +259,10 @@ public class ContainerClusterTest { ContainerCluster cluster = createContainerCluster(root, false); addContainer(root.deployLogger(), cluster, "c1", "host-c1"); Container container = cluster.getContainers().get(0); - container.setJvmArgs(""); - String empty = container.getJvmArgs(); - container.setJvmArgs(null); - assertEquals(empty, container.getJvmArgs()); + container.setJvmOptions(""); + String empty = container.getJvmOptions(); + container.setJvmOptions(null); + assertEquals(empty, container.getJvmOptions()); } @Test diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/search/searchchain/SearchChainsTest2.java b/config-model/src/test/java/com/yahoo/vespa/model/container/search/searchchain/SearchChainsTest2.java index 9122e855461..6ba75f1ff05 100644 --- a/config-model/src/test/java/com/yahoo/vespa/model/container/search/searchchain/SearchChainsTest2.java +++ b/config-model/src/test/java/com/yahoo/vespa/model/container/search/searchchain/SearchChainsTest2.java @@ -9,13 +9,11 @@ import org.junit.Before; import org.junit.Test; import org.w3c.dom.Element; -import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.fail; -import static org.hamcrest.Matchers.containsString; /** * @author gjoranv - * @since 5.1.11 */ public class SearchChainsTest2 { @@ -38,7 +36,8 @@ public class SearchChainsTest2 { chains.validate(); fail("Expected exception when inheriting a nonexistent search chain."); } catch (Exception e) { - assertThat(e.getMessage(), containsString("Missing chain 'nonexistent'")); + assertEquals("Missing chain 'nonexistent'.", + e.getMessage()); } } @@ -56,12 +55,13 @@ public class SearchChainsTest2 { ContainerModelBuilderTest.createModel(root, clusterElem); fail("Expected exception when declaring chains with duplicate id."); } catch (Exception e) { - assertThat(e.getMessage(), containsString("Two entities have the same component id 'same'")); + assertEquals("Both search chain 'same' and search chain 'same' are configured with the id 'same'. All components must have a unique id.", + e.getMessage()); } } @Test - public void fail_upon_user_declared_chain_with_same_id_as_builtin_chain() throws Exception { + public void fail_upon_user_declared_chain_with_same_id_as_builtin_chain() { final Element clusterElem = DomBuilderTest.parse( "<jdisc id='cluster1' version='1.0'>", ContainerModelBuilderTest.nodesXml, @@ -73,7 +73,8 @@ public class SearchChainsTest2 { ContainerModelBuilderTest.createModel(root, clusterElem); fail("Expected exception when taking the id from a builtin chain."); } catch (Exception e) { - assertThat(e.getMessage(), containsString("Two entities have the same component id 'vespa'")); + assertEquals("Both search chain 'vespa' and search chain 'vespa' are configured with the id 'vespa'. All components must have a unique id.", + e.getMessage()); } } } diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilderTest.java b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilderTest.java index a3e24b8a520..396fe3e0af5 100644 --- a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilderTest.java +++ b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilderTest.java @@ -14,6 +14,7 @@ import com.yahoo.config.model.provision.InMemoryProvisioner; import com.yahoo.config.model.test.MockApplicationPackage; import com.yahoo.config.provision.Environment; import com.yahoo.config.provision.RegionName; +import com.yahoo.config.provision.SystemName; import com.yahoo.config.provision.Zone; import com.yahoo.container.ComponentsConfig; import com.yahoo.container.QrConfig; @@ -69,7 +70,7 @@ import static org.junit.Assert.fail; public class ContainerModelBuilderTest extends ContainerModelBuilderTestBase { @Test - public void detect_conflicting_gcoptions_in_jvmargs() { + public void detect_conflicting_jvmgcoptions_in_jvmargs() { assertFalse(ContainerModelBuilder.incompatibleGCOptions("")); assertFalse(ContainerModelBuilder.incompatibleGCOptions("UseG1GC")); assertTrue(ContainerModelBuilder.incompatibleGCOptions("-XX:+UseG1GC")); @@ -78,11 +79,11 @@ public class ContainerModelBuilderTest extends ContainerModelBuilderTestBase { } @Test - public void honours_gcopts() { + public void honours_jvm_gc_options() { Element clusterElem = DomBuilderTest.parse( "<jdisc version='1.0'>", " <search/>", - " <nodes gcopts='-XX:+UseG1GC'>", + " <nodes jvm-gc-options='-XX:+UseG1GC'>", " <node hostalias='mockhost'/>", " </nodes>", "</jdisc>" ); @@ -93,23 +94,84 @@ public class ContainerModelBuilderTest extends ContainerModelBuilderTestBase { assertEquals("-XX:+UseG1GC", qrStartConfig.jvm().gcopts()); } + private static void verifyIgnoreJvmGCOptions(boolean isHosted) throws IOException, SAXException { + verifyIgnoreJvmGCOptionsIfJvmArgs(isHosted, "jvmargs", ContainerCluster.CMS); + verifyIgnoreJvmGCOptionsIfJvmArgs(isHosted, "jvm-options", "-XX:+UseG1GC"); + + } + private static void verifyIgnoreJvmGCOptionsIfJvmArgs(boolean isHosted, String jvmOptionsName, String expectedGC) throws IOException, SAXException { + String servicesXml = + "<jdisc version='1.0'>" + + " <nodes jvm-gc-options='-XX:+UseG1GC' " + jvmOptionsName + "='-XX:+UseParNewGC'>" + + " <node hostalias='mockhost'/>" + + " </nodes>" + + "</jdisc>"; + ApplicationPackage applicationPackage = new MockApplicationPackage.Builder().withServices(servicesXml).build(); + // Need to create VespaModel to make deploy properties have effect + final MyLogger logger = new MyLogger(); + 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 DeployProperties.Builder() + .hostedVespa(isHosted) + .build()) + .build()); + QrStartConfig.Builder qrStartBuilder = new QrStartConfig.Builder(); + model.getConfig(qrStartBuilder, "jdisc/container.0"); + QrStartConfig qrStartConfig = new QrStartConfig(qrStartBuilder); + assertEquals(expectedGC, qrStartConfig.jvm().gcopts()); + } + @Test - public void ignores_gcopts_on_conflicting_jvargs() { - Element clusterElem = DomBuilderTest.parse( - "<jdisc version='1.0'>", - " <nodes gcopts='-XX:+UseG1GC' jvmargs='-XX:+UseParNewGC'>", - " <node hostalias='mockhost'/>", - " </nodes>", - "</jdisc>" ); - createModel(root, clusterElem); + public void ignores_jvmgcoptions_on_conflicting_jvmargs() throws IOException, SAXException { + verifyIgnoreJvmGCOptions(false); + verifyIgnoreJvmGCOptions(true); + } + + private void verifyJvmGCOptions(boolean isHosted, String override, Zone zone, String expected) throws IOException, SAXException { + String servicesXml = + "<jdisc version='1.0'>" + + " <nodes " + ((override == null) ? ">" : ("jvm-gc-options='" + override + "'>")) + + " <node hostalias='mockhost'/>" + + " </nodes>" + + "</jdisc>"; + ApplicationPackage applicationPackage = new MockApplicationPackage.Builder().withServices(servicesXml).build(); + // Need to create VespaModel to make deploy properties have effect + final MyLogger logger = new MyLogger(); + VespaModel model = new VespaModel(new NullConfigModelRegistry(), new DeployState.Builder() + .applicationPackage(applicationPackage) + .deployLogger(logger) + .zone(zone) + .properties(new DeployProperties.Builder() + .hostedVespa(isHosted) + .build()) + .build()); QrStartConfig.Builder qrStartBuilder = new QrStartConfig.Builder(); - root.getConfig(qrStartBuilder, "jdisc/container.0"); + model.getConfig(qrStartBuilder, "jdisc/container.0"); QrStartConfig qrStartConfig = new QrStartConfig(qrStartBuilder); - assertEquals(ContainerCluster.CMS, qrStartConfig.jvm().gcopts()); + 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 { + final Zone US_EAST_3 = new Zone(Environment.prod, RegionName.from("us-east-3")); + verifyJvmGCOptions(false, Zone.defaultZone(),ContainerCluster.CMS); + verifyJvmGCOptions(false, US_EAST_3, ContainerCluster.CMS); + verifyJvmGCOptions(true, Zone.defaultZone(), ContainerCluster.CMS); + verifyJvmGCOptions(true, US_EAST_3, ContainerCluster.G1GC); } @Test - public void default_port_is_4080() throws Exception { + public void default_port_is_4080() { Element clusterElem = DomBuilderTest.parse( "<jdisc version='1.0'>", nodesXml, @@ -120,7 +182,7 @@ public class ContainerModelBuilderTest extends ContainerModelBuilderTestBase { } @Test - public void http_server_port_is_configurable_and_does_not_affect_other_ports() throws Exception { + public void http_server_port_is_configurable_and_does_not_affect_other_ports() { Element clusterElem = DomBuilderTest.parse( "<jdisc version='1.0'>", " <http>", @@ -162,7 +224,7 @@ public class ContainerModelBuilderTest extends ContainerModelBuilderTestBase { assertThat(logger.msgs.get(0).getSecond(), containsString(String.format("You cannot set port to anything else than %d", Container.BASEPORT))); } - private class MyLogger implements DeployLogger { + private static class MyLogger implements DeployLogger { List<Pair<Level, String>> msgs = new ArrayList<>(); @Override public void log(Level level, String message) { @@ -171,7 +233,7 @@ public class ContainerModelBuilderTest extends ContainerModelBuilderTestBase { } @Test - public void one_cluster_with_explicit_port_and_one_without_is_ok() throws Exception { + public void one_cluster_with_explicit_port_and_one_without_is_ok() { Element cluster1Elem = DomBuilderTest.parse( "<jdisc id='cluster1' version='1.0' />"); Element cluster2Elem = DomBuilderTest.parse( @@ -184,7 +246,7 @@ public class ContainerModelBuilderTest extends ContainerModelBuilderTestBase { } @Test - public void two_clusters_without_explicit_port_throws_exception() throws SAXException, IOException { + public void two_clusters_without_explicit_port_throws_exception() { Element cluster1Elem = DomBuilderTest.parse( "<jdisc id='cluster1' version='1.0'>", nodesXml, @@ -202,7 +264,7 @@ public class ContainerModelBuilderTest extends ContainerModelBuilderTestBase { } @Test - public void verify_bindings_for_builtin_handlers() throws Exception { + public void verify_bindings_for_builtin_handlers() { Element clusterElem = DomBuilderTest.parse( "<jdisc id='default' version='1.0' />" ); @@ -226,7 +288,7 @@ public class ContainerModelBuilderTest extends ContainerModelBuilderTestBase { } @Test - public void default_root_handler_is_disabled_when_user_adds_a_handler_with_same_binding() throws Exception { + public void default_root_handler_is_disabled_when_user_adds_a_handler_with_same_binding() { Element clusterElem = DomBuilderTest.parse( "<jdisc id='default' version='1.0'>" + " <handler id='userRootHandler'>" + @@ -240,7 +302,7 @@ public class ContainerModelBuilderTest extends ContainerModelBuilderTestBase { } @Test - public void handler_bindings_are_included_in_discBindings_config() throws Exception { + public void handler_bindings_are_included_in_discBindings_config() { createClusterWithJDiscHandler(); String discBindingsConfig = root.getConfig(JdiscBindingsConfig.class, "default").toString(); assertThat(discBindingsConfig, containsString("{discHandler}")); @@ -250,12 +312,12 @@ public class ContainerModelBuilderTest extends ContainerModelBuilderTestBase { } @Test - public void handlers_are_included_in_components_config() throws Exception { + public void handlers_are_included_in_components_config() { createClusterWithJDiscHandler(); assertThat(componentsConfig().toString(), containsString(".id \"discHandler\"")); } - private void createClusterWithJDiscHandler() throws SAXException, IOException { + private void createClusterWithJDiscHandler() { Element clusterElem = DomBuilderTest.parse( "<jdisc id='default' version='1.0'>", " <handler id='discHandler'>", @@ -269,14 +331,14 @@ public class ContainerModelBuilderTest extends ContainerModelBuilderTestBase { } @Test - public void servlets_are_included_in_ServletPathConfig() throws Exception { + public void servlets_are_included_in_ServletPathConfig() { createClusterWithServlet(); ServletPathsConfig servletPathsConfig = root.getConfig(ServletPathsConfig.class, "default"); assertThat(servletPathsConfig.servlets().values().iterator().next().path(), is("p/a/t/h")); } @Test - public void servletconfig_is_produced() throws Exception { + public void servletconfig_is_produced() { createClusterWithServlet(); String configId = getContainerCluster("default").getServletMap(). @@ -287,7 +349,7 @@ public class ContainerModelBuilderTest extends ContainerModelBuilderTestBase { assertThat(servletConfig.map().get("myKey"), is("myValue")); } - private void createClusterWithServlet() throws SAXException, IOException { + private void createClusterWithServlet() { Element clusterElem = DomBuilderTest.parse( "<jdisc id='default' version='1.0'>", " <servlet id='myServlet' class='myClass' bundle='myBundle'>", @@ -303,7 +365,7 @@ public class ContainerModelBuilderTest extends ContainerModelBuilderTestBase { @Test - public void processing_handler_bindings_can_be_overridden() throws Exception { + public void processing_handler_bindings_can_be_overridden() { Element clusterElem = DomBuilderTest.parse( "<jdisc id='default' version='1.0'>", " <processing>", @@ -321,7 +383,7 @@ public class ContainerModelBuilderTest extends ContainerModelBuilderTestBase { } @Test - public void clientProvider_bindings_are_included_in_discBindings_config() throws Exception { + public void clientProvider_bindings_are_included_in_discBindings_config() { createModelWithClientProvider(); String discBindingsConfig = root.getConfig(JdiscBindingsConfig.class, "default").toString(); assertThat(discBindingsConfig, containsString("{discClient}")); @@ -331,12 +393,12 @@ public class ContainerModelBuilderTest extends ContainerModelBuilderTestBase { } @Test - public void clientProviders_are_included_in_components_config() throws Exception { + public void clientProviders_are_included_in_components_config() { createModelWithClientProvider(); assertThat(componentsConfig().toString(), containsString(".id \"discClient\"")); } - private void createModelWithClientProvider() throws SAXException, IOException { + private void createModelWithClientProvider() { Element clusterElem = DomBuilderTest.parse( "<jdisc id='default' version='1.0'>" + " <client id='discClient'>" + @@ -350,7 +412,7 @@ public class ContainerModelBuilderTest extends ContainerModelBuilderTestBase { } @Test - public void serverProviders_are_included_in_components_config() throws Exception { + public void serverProviders_are_included_in_components_config() { Element clusterElem = DomBuilderTest.parse( "<jdisc id='default' version='1.0'>" + " <server id='discServer' />" + @@ -367,7 +429,7 @@ public class ContainerModelBuilderTest extends ContainerModelBuilderTestBase { } @Test - public void searchHandler_gets_only_search_chains_in_chains_config() throws Exception { + public void searchHandler_gets_only_search_chains_in_chains_config() { createClusterWithProcessingAndSearchChains(); String searchHandlerConfigId = "default/component/com.yahoo.search.handler.SearchHandler"; String chainsConfig = getChainsConfig(searchHandlerConfigId); @@ -376,7 +438,7 @@ public class ContainerModelBuilderTest extends ContainerModelBuilderTestBase { } @Test - public void processingHandler_gets_only_processing_chains_in_chains_config() throws Exception { + public void processingHandler_gets_only_processing_chains_in_chains_config() { createClusterWithProcessingAndSearchChains(); String processingHandlerConfigId = "default/component/com.yahoo.processing.handler.ProcessingHandler"; String chainsConfig = getChainsConfig(processingHandlerConfigId); @@ -384,7 +446,7 @@ public class ContainerModelBuilderTest extends ContainerModelBuilderTestBase { assertThat(chainsConfig, not(containsLineWithPattern(".*\\.id \"testSearcher@default\"$"))); } - private void createClusterWithProcessingAndSearchChains() throws SAXException, IOException { + private void createClusterWithProcessingAndSearchChains() { Element clusterElem = DomBuilderTest.parse( "<jdisc id='default' version='1.0'>" + " <search>" + @@ -404,7 +466,7 @@ public class ContainerModelBuilderTest extends ContainerModelBuilderTestBase { } @Test - public void user_config_can_be_overridden_on_node() throws Exception { + public void user_config_can_be_overridden_on_node() { Element containerElem = DomBuilderTest.parse( "<jdisc id='default' version='1.0'>", " <config name=\"prelude.cluster.qr-monitor\">" + @@ -429,7 +491,7 @@ public class ContainerModelBuilderTest extends ContainerModelBuilderTestBase { } @Test - public void legacy_yca_filter_and_its_config_provider_are_included_in_components_config() throws Exception { + public void legacy_yca_filter_and_its_config_provider_are_included_in_components_config() { Element clusterElem = DomBuilderTest.parse( "<jdisc id='default' version='1.0'>", " <filter id='YcaFilter' /> ", @@ -443,7 +505,7 @@ public class ContainerModelBuilderTest extends ContainerModelBuilderTestBase { } @Test - public void nested_components_are_injected_to_handlers() throws Exception { + public void nested_components_are_injected_to_handlers() { Element clusterElem = DomBuilderTest.parse( "<jdisc id='default' version='1.0'>", " <handler id='myHandler'>", @@ -474,7 +536,7 @@ public class ContainerModelBuilderTest extends ContainerModelBuilderTestBase { } @Test - public void affinity_is_set() throws IOException, SAXException { + public void affinity_is_set() { Element clusterElem = DomBuilderTest.parse( "<jdisc id='default' version='1.0'>", " <http>", @@ -516,7 +578,7 @@ public class ContainerModelBuilderTest extends ContainerModelBuilderTestBase { } @Test - public void http_aliases_are_stored_on_cluster_and_on_service_properties() throws SAXException, IOException { + public void http_aliases_are_stored_on_cluster_and_on_service_properties() { Element clusterElem = DomBuilderTest.parse( "<jdisc id='default' version='1.0'>", " <aliases>", @@ -541,7 +603,7 @@ public class ContainerModelBuilderTest extends ContainerModelBuilderTestBase { } @Test - public void http_aliases_are_only_honored_in_prod_environment() throws SAXException, IOException { + public void http_aliases_are_only_honored_in_prod_environment() { Element clusterElem = DomBuilderTest.parse( "<jdisc id='default' version='1.0'>", " <aliases>", @@ -578,17 +640,17 @@ public class ContainerModelBuilderTest extends ContainerModelBuilderTestBase { } @Test(expected = IllegalArgumentException.class) - public void renderers_named_JsonRenderer_are_not_allowed() throws IOException, SAXException { + public void renderers_named_JsonRenderer_are_not_allowed() { createModel(root, generateContainerElementWithRenderer("JsonRenderer")); } @Test(expected = IllegalArgumentException.class) - public void renderers_named_DefaultRenderer_are_not_allowed() throws IOException, SAXException { + public void renderers_named_DefaultRenderer_are_not_allowed() { createModel(root, generateContainerElementWithRenderer("DefaultRenderer")); } @Test - public void renderers_named_something_else_are_allowed() throws IOException, SAXException { + public void renderers_named_something_else_are_allowed() { createModel(root, generateContainerElementWithRenderer("my-little-renderer")); } @@ -642,15 +704,15 @@ public class ContainerModelBuilderTest extends ContainerModelBuilderTestBase { assertEquals("default.container.0", config.discriminator()); assertEquals(19102, config.rpc().port()); assertEquals("vespa/service/default/container.0", config.rpc().slobrokId()); - assertEquals(true, config.rpc().enabled()); + assertTrue(config.rpc().enabled()); assertEquals("", config.rpc().host()); - assertEquals(false, config.restartOnDeploy()); - assertEquals(false, config.coveragereports()); + assertFalse(config.restartOnDeploy()); + assertFalse(config.coveragereports()); assertEquals("filedistribution/" + hostname, config.filedistributor().configid()); } @Test - public void secret_store_can_be_set_up() throws IOException, SAXException { + public void secret_store_can_be_set_up() { Element clusterElem = DomBuilderTest.parse( "<jdisc version='1.0'>", " <secret-store>", diff --git a/config-model/src/test/java/com/yahoo/vespa/model/content/StorageGroupTest.java b/config-model/src/test/java/com/yahoo/vespa/model/content/StorageGroupTest.java index 69e2dba556a..31c1e250183 100644 --- a/config-model/src/test/java/com/yahoo/vespa/model/content/StorageGroupTest.java +++ b/config-model/src/test/java/com/yahoo/vespa/model/content/StorageGroupTest.java @@ -26,7 +26,7 @@ public class StorageGroupTest { "<content id=\"storage\">\n" + " <documents/>" + " <group>\n" + - " <node jvmargs=\"foo\" hostalias=\"mockhost\" distribution-key=\"0\"/>\n" + + " <node hostalias=\"mockhost\" distribution-key=\"0\"/>\n" + " <node hostalias=\"mockhost\" distribution-key=\"1\"/>\n" + " </group>\n" + "</content>" diff --git a/config-model/src/test/schema-test-files/services.xml b/config-model/src/test/schema-test-files/services.xml index 632abe68ab7..43a8d31e2ee 100644 --- a/config-model/src/test/schema-test-files/services.xml +++ b/config-model/src/test/schema-test-files/services.xml @@ -209,7 +209,7 @@ </server> - <nodes jvmargs="-XX:+PrintGCDetails -XX:+PrintGCTimeStamps"> + <nodes jvm-options="-XX:+PrintGCDetails -XX:+PrintGCTimeStamps"> <node hostalias="host1" /> <node hostalias="host1"> <server-port id="myServer" port="4090" /> diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/application/OrchestratorMock.java b/configserver/src/test/java/com/yahoo/vespa/config/server/application/OrchestratorMock.java index e61d3710fac..73abd70a5ae 100644 --- a/configserver/src/test/java/com/yahoo/vespa/config/server/application/OrchestratorMock.java +++ b/configserver/src/test/java/com/yahoo/vespa/config/server/application/OrchestratorMock.java @@ -47,11 +47,6 @@ public class OrchestratorMock implements Orchestrator { } @Override - public void suspendGroup(NodeGroup nodeGroup) { - nodeGroup.getHostNames().forEach(this::suspend); - } - - @Override public ApplicationInstanceStatus getApplicationInstanceStatus(ApplicationId appId) { return suspendedApplications.contains(appId) ? ApplicationInstanceStatus.ALLOWED_TO_BE_DOWN : ApplicationInstanceStatus.NO_REMARKS; diff --git a/container-search/src/main/java/com/yahoo/prelude/fastsearch/FS4InvokerFactory.java b/container-search/src/main/java/com/yahoo/prelude/fastsearch/FS4InvokerFactory.java index f68bb718c8d..51048db3cb7 100644 --- a/container-search/src/main/java/com/yahoo/prelude/fastsearch/FS4InvokerFactory.java +++ b/container-search/src/main/java/com/yahoo/prelude/fastsearch/FS4InvokerFactory.java @@ -8,10 +8,12 @@ import com.yahoo.search.Result; import com.yahoo.search.dispatch.FillInvoker; import com.yahoo.search.dispatch.InterleavedFillInvoker; import com.yahoo.search.dispatch.InterleavedSearchInvoker; -import com.yahoo.search.dispatch.SearchCluster; import com.yahoo.search.dispatch.SearchInvoker; +import com.yahoo.search.dispatch.searchcluster.Node; +import com.yahoo.search.dispatch.searchcluster.SearchCluster; import com.yahoo.search.result.Hit; +import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; import java.util.HashSet; @@ -30,18 +32,20 @@ import java.util.Set; public class FS4InvokerFactory { private final FS4ResourcePool fs4ResourcePool; private final VespaBackEndSearcher searcher; - private final ImmutableMap<Integer, SearchCluster.Node> nodesByKey; + private final SearchCluster searchCluster; + private final ImmutableMap<Integer, Node> nodesByKey; public FS4InvokerFactory(FS4ResourcePool fs4ResourcePool, SearchCluster searchCluster, VespaBackEndSearcher searcher) { this.fs4ResourcePool = fs4ResourcePool; this.searcher = searcher; + this.searchCluster = searchCluster; - ImmutableMap.Builder<Integer, SearchCluster.Node> builder = ImmutableMap.builder(); + ImmutableMap.Builder<Integer, Node> builder = ImmutableMap.builder(); searchCluster.groups().values().forEach(group -> group.nodes().forEach(node -> builder.put(node.key(), node))); this.nodesByKey = builder.build(); } - public SearchInvoker getSearchInvoker(Query query, SearchCluster.Node node) { + public SearchInvoker getSearchInvoker(Query query, Node node) { Backend backend = fs4ResourcePool.getBackend(node.hostname(), node.fs4port()); return new FS4SearchInvoker(searcher, query, backend.openChannel(), node); } @@ -49,22 +53,46 @@ public class FS4InvokerFactory { /** * Create a {@link SearchInvoker} for a list of content nodes. * - * @param query the search query being processed - * @param nodes pre-selected list of content nodes - * @return Optional containing the SearchInvoker or <i>empty</i> if some node in the list is invalid + * @param query + * the search query being processed + * @param groupId + * the id of the node group to which the nodes belong + * @param nodes + * pre-selected list of content nodes + * @param acceptIncompleteCoverage + * if some of the nodes are unavailable and this parameter is + * <b>false</b>, verify that the remaining set of nodes has enough + * coverage + * @return Optional containing the SearchInvoker or <i>empty</i> if some node in the + * list is invalid and the remaining coverage is not sufficient */ - public Optional<SearchInvoker> getSearchInvoker(Query query, List<SearchCluster.Node> nodes) { + public Optional<SearchInvoker> getSearchInvoker(Query query, int groupId, List<Node> nodes, boolean acceptIncompleteCoverage) { Map<Integer, SearchInvoker> invokers = new HashMap<>(); - for (SearchCluster.Node node : nodes) { + Set<Integer> failed = null; + for (Node node : nodes) { if (node.isWorking()) { Backend backend = fs4ResourcePool.getBackend(node.hostname(), node.fs4port()); if (backend.probeConnection()) { invokers.put(node.key(), new FS4SearchInvoker(searcher, query, backend.openChannel(), node)); } else { - return Optional.empty(); + if(failed == null) { + failed = new HashSet<>(); + } + failed.add(node.key()); } } } + if (failed != null && ! acceptIncompleteCoverage) { + List<Node> success = new ArrayList<>(nodes.size() - failed.size()); + for (Node node : nodes) { + if (!failed.contains(node.key())) { + success.add(node); + } + } + if (!searchCluster.isPartialGroupCoverageSufficient(groupId, success)) { + return Optional.empty(); + } + } if (invokers.size() == 1) { return Optional.of(invokers.values().iterator().next()); } else { @@ -72,7 +100,7 @@ public class FS4InvokerFactory { } } - public FillInvoker getFillInvoker(Query query, SearchCluster.Node node) { + public FillInvoker getFillInvoker(Query query, Node node) { return new FS4FillInvoker(searcher, query, fs4ResourcePool, node.hostname(), node.fs4port(), node.key()); } @@ -88,7 +116,7 @@ public class FS4InvokerFactory { Query query = result.getQuery(); Map<Integer, FillInvoker> invokers = new HashMap<>(); for (Integer distKey : requiredNodes) { - SearchCluster.Node node = nodesByKey.get(distKey); + Node node = nodesByKey.get(distKey); if (node == null) { return Optional.empty(); } diff --git a/container-search/src/main/java/com/yahoo/prelude/fastsearch/FS4SearchInvoker.java b/container-search/src/main/java/com/yahoo/prelude/fastsearch/FS4SearchInvoker.java index dc8cd53e638..98676890bdf 100644 --- a/container-search/src/main/java/com/yahoo/prelude/fastsearch/FS4SearchInvoker.java +++ b/container-search/src/main/java/com/yahoo/prelude/fastsearch/FS4SearchInvoker.java @@ -11,8 +11,8 @@ import com.yahoo.fs4.mplex.FS4Channel; import com.yahoo.fs4.mplex.InvalidChannelException; import com.yahoo.search.Query; import com.yahoo.search.Result; -import com.yahoo.search.dispatch.SearchCluster; import com.yahoo.search.dispatch.SearchInvoker; +import com.yahoo.search.dispatch.searchcluster.Node; import com.yahoo.search.result.Coverage; import com.yahoo.search.result.ErrorMessage; @@ -33,13 +33,13 @@ import static java.util.Arrays.asList; public class FS4SearchInvoker extends SearchInvoker { private final VespaBackEndSearcher searcher; private FS4Channel channel; - private final Optional<SearchCluster.Node> node; + private final Optional<Node> node; private ErrorMessage pendingSearchError = null; private Query query = null; private QueryPacket queryPacket = null; - public FS4SearchInvoker(VespaBackEndSearcher searcher, Query query, FS4Channel channel, SearchCluster.Node node) { + public FS4SearchInvoker(VespaBackEndSearcher searcher, Query query, FS4Channel channel, Node node) { this.searcher = searcher; this.node = Optional.of(node); this.channel = channel; @@ -115,7 +115,7 @@ public class FS4SearchInvoker extends SearchInvoker { searcher.addMetaInfo(query, queryPacket.getQueryPacketData(), resultPacket, result); - searcher.addUnfilledHits(result, resultPacket.getDocuments(), false, queryPacket.getQueryPacketData(), cacheKey, node.map(SearchCluster.Node::key)); + searcher.addUnfilledHits(result, resultPacket.getDocuments(), false, queryPacket.getQueryPacketData(), cacheKey, node.map(Node::key)); Packet[] packets; CacheControl cacheControl = searcher.getCacheControl(); PacketWrapper packetWrapper = cacheControl.lookup(cacheKey, query); @@ -130,7 +130,7 @@ public class FS4SearchInvoker extends SearchInvoker { } else { packets = new Packet[1]; packets[0] = resultPacket; - cacheControl.cache(cacheKey, query, new DocsumPacketKey[0], packets, node.map(SearchCluster.Node::key)); + cacheControl.cache(cacheKey, query, new DocsumPacketKey[0], packets, node.map(Node::key)); } } return asList(result); diff --git a/container-search/src/main/java/com/yahoo/prelude/fastsearch/FastSearcher.java b/container-search/src/main/java/com/yahoo/prelude/fastsearch/FastSearcher.java index 36d283040a2..11b71c2c159 100644 --- a/container-search/src/main/java/com/yahoo/prelude/fastsearch/FastSearcher.java +++ b/container-search/src/main/java/com/yahoo/prelude/fastsearch/FastSearcher.java @@ -17,8 +17,8 @@ import com.yahoo.search.Query; import com.yahoo.search.Result; import com.yahoo.search.dispatch.Dispatcher; import com.yahoo.search.dispatch.FillInvoker; -import com.yahoo.search.dispatch.SearchCluster; import com.yahoo.search.dispatch.SearchInvoker; +import com.yahoo.search.dispatch.searchcluster.Node; import com.yahoo.search.grouping.GroupingRequest; import com.yahoo.search.grouping.request.GroupingOperation; import com.yahoo.search.query.Ranking; @@ -218,7 +218,7 @@ public class FastSearcher extends VespaBackEndSearcher { return invoker.get(); } - Optional<SearchCluster.Node> direct = getDirectNode(query); + Optional<Node> direct = getDirectNode(query); if(direct.isPresent()) { return fs4InvokerFactory.getSearchInvoker(query, direct.get()); } @@ -237,7 +237,7 @@ public class FastSearcher extends VespaBackEndSearcher { return invoker.get(); } - Optional<SearchCluster.Node> direct = getDirectNode(query); + Optional<Node> direct = getDirectNode(query); if (direct.isPresent()) { return fs4InvokerFactory.getFillInvoker(query, direct.get()); } @@ -248,18 +248,18 @@ public class FastSearcher extends VespaBackEndSearcher { * If the query can be directed to a single local content node, returns that node. Otherwise, * returns an empty value. */ - private Optional<SearchCluster.Node> getDirectNode(Query query) { + private Optional<Node> getDirectNode(Query query) { if (!query.properties().getBoolean(dispatchDirect, true)) return Optional.empty(); if (query.properties().getBoolean(com.yahoo.search.query.Model.ESTIMATE)) return Optional.empty(); - Optional<SearchCluster.Node> directDispatchRecipient = dispatcher.searchCluster().directDispatchTarget(); + Optional<Node> directDispatchRecipient = dispatcher.searchCluster().directDispatchTarget(); if (!directDispatchRecipient.isPresent()) return Optional.empty(); // Dispatch directly to the single, local search node - SearchCluster.Node local = directDispatchRecipient.get(); + Node local = directDispatchRecipient.get(); query.trace(false, 2, "Dispatching directly to ", local); return Optional.of(local); } diff --git a/container-search/src/main/java/com/yahoo/search/cluster/ClusterMonitor.java b/container-search/src/main/java/com/yahoo/search/cluster/ClusterMonitor.java index 4878691742c..5de0c5eff74 100644 --- a/container-search/src/main/java/com/yahoo/search/cluster/ClusterMonitor.java +++ b/container-search/src/main/java/com/yahoo/search/cluster/ClusterMonitor.java @@ -1,8 +1,6 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.search.cluster; - -import com.yahoo.concurrent.DaemonThreadFactory; import com.yahoo.concurrent.ThreadFactoryFactory; import com.yahoo.search.result.ErrorMessage; @@ -43,7 +41,7 @@ public class ClusterMonitor<T> { public ClusterMonitor(NodeManager<T> manager, String ignored) { this(manager); } - + public ClusterMonitor(NodeManager<T> manager) { nodeManager = manager; monitorThread = new MonitorThread("search.clustermonitor"); diff --git a/container-search/src/main/java/com/yahoo/search/cluster/TrafficNodeMonitor.java b/container-search/src/main/java/com/yahoo/search/cluster/TrafficNodeMonitor.java index c1c955cb03a..ea881ad8b48 100644 --- a/container-search/src/main/java/com/yahoo/search/cluster/TrafficNodeMonitor.java +++ b/container-search/src/main/java/com/yahoo/search/cluster/TrafficNodeMonitor.java @@ -1,6 +1,7 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.search.cluster; +import com.yahoo.container.protect.Error; import com.yahoo.search.result.ErrorMessage; /** @@ -36,20 +37,14 @@ public class TrafficNodeMonitor<T> extends BaseNodeMonitor<T> { public void failed(ErrorMessage error) { respondedAt = now(); - switch (error.getCode()) { - // TODO: Remove hard coded error messages. - // Refer to docs/errormessages - case 10: - case 11: - // Only count not being able to talk to backend at all - // as errors we care about - if ((respondedAt-succeededAt) > 10000) { - setWorking(false, "Not working for 10 s: " + error.toString()); - } - break; - default: - succeededAt = respondedAt; - break; + if (error.getCode() == Error.BACKEND_COMMUNICATION_ERROR.code) { + setWorking(false, "Connection failure: " + error.toString()); + } else if (error.getCode() == Error.NO_ANSWER_WHEN_PINGING_NODE.code) { + if ((respondedAt - succeededAt) > 10000) { + setWorking(false, "Not working for 10 s: " + error.toString()); + } + } else { + succeededAt = respondedAt; } } diff --git a/container-search/src/main/java/com/yahoo/search/dispatch/Dispatcher.java b/container-search/src/main/java/com/yahoo/search/dispatch/Dispatcher.java index 286eee004c5..0dd682dee0e 100644 --- a/container-search/src/main/java/com/yahoo/search/dispatch/Dispatcher.java +++ b/container-search/src/main/java/com/yahoo/search/dispatch/Dispatcher.java @@ -11,6 +11,9 @@ import com.yahoo.processing.request.CompoundName; import com.yahoo.search.Query; import com.yahoo.search.Result; import com.yahoo.search.dispatch.SearchPath.InvalidSearchPathException; +import com.yahoo.search.dispatch.searchcluster.Group; +import com.yahoo.search.dispatch.searchcluster.Node; +import com.yahoo.search.dispatch.searchcluster.SearchCluster; import com.yahoo.vespa.config.search.DispatchConfig; import java.util.Arrays; @@ -33,6 +36,8 @@ import java.util.Set; * @author ollvir */ public class Dispatcher extends AbstractComponent { + private static final int MAX_GROUP_SELECTION_ATTEMPTS = 3; + /** If enabled, this internal dispatcher will be preferred over fdispatch whenever possible */ private static final CompoundName dispatchInternal = new CompoundName("dispatch.internal"); @@ -66,11 +71,6 @@ public class Dispatcher extends AbstractComponent { rpcResourcePool.release(); } - @FunctionalInterface - private interface SearchInvokerSupplier { - Optional<SearchInvoker> supply(Query query, List<SearchCluster.Node> nodes); - } - public Optional<FillInvoker> getFillInvoker(Result result, VespaBackEndSearcher searcher, DocumentDatabase documentDb, FS4InvokerFactory fs4InvokerFactory) { Optional<FillInvoker> rpcInvoker = rpcResourcePool.getFillInvoker(result.getQuery(), searcher, documentDb); @@ -102,6 +102,11 @@ public class Dispatcher extends AbstractComponent { return Optional.empty(); } + @FunctionalInterface + private interface SearchInvokerSupplier { + Optional<SearchInvoker> supply(Query query, int groupId, List<Node> nodes, boolean acceptIncompleteCoverage); + } + // build invoker based on searchpath private Optional<SearchInvoker> getSearchPathInvoker(Query query, SearchInvokerSupplier invokerFactory) { String searchPath = query.getModel().getSearchPath(); @@ -109,11 +114,11 @@ public class Dispatcher extends AbstractComponent { return Optional.empty(); } try { - List<SearchCluster.Node> nodes = SearchPath.selectNodes(searchPath, searchCluster); + List<Node> nodes = SearchPath.selectNodes(searchPath, searchCluster); if (nodes.isEmpty()) { return Optional.empty(); } else { - return invokerFactory.supply(query, nodes); + return invokerFactory.supply(query, -1, nodes, true); } } catch (InvalidSearchPathException e) { return Optional.of(new SearchErrorInvoker(e.getMessage())); @@ -121,41 +126,34 @@ public class Dispatcher extends AbstractComponent { } private Optional<SearchInvoker> getInternalInvoker(Query query, SearchInvokerSupplier invokerFactory) { - Optional<SearchCluster.Node> directNode = searchCluster.directDispatchTarget(); + Optional<Node> directNode = searchCluster.directDispatchTarget(); if (directNode.isPresent()) { - SearchCluster.Node node = directNode.get(); + Node node = directNode.get(); query.trace(false, 2, "Dispatching directly to ", node); - return invokerFactory.supply(query, Arrays.asList(node)); + return invokerFactory.supply(query, -1, Arrays.asList(node), true); } - Set<Integer> tried = null; - int max = searchCluster.groups().size(); - for (int attempt = 0; attempt < max; attempt++) { - Optional<SearchCluster.Group> groupInCluster = loadBalancer.takeGroupForQuery(query); - if (! groupInCluster.isPresent()) { + int max = Integer.min(searchCluster.orderedGroups().size(), MAX_GROUP_SELECTION_ATTEMPTS); + Set<Integer> rejected = null; + for (int i = 0; i < max; i++) { + Optional<Group> groupInCluster = loadBalancer.takeGroupForQuery(query, rejected); + if (!groupInCluster.isPresent()) { // No groups available break; } - SearchCluster.Group group = groupInCluster.get(); - if (tried != null && tried.contains(group.id())) { - // bail out: LB is offering a previously discarded group - loadBalancer.releaseGroup(group); - break; - } - - Optional<SearchInvoker> invoker = invokerFactory.supply(query, group.nodes()); + Group group = groupInCluster.get(); + boolean acceptIncompleteCoverage = (i == max - 1); + Optional<SearchInvoker> invoker = invokerFactory.supply(query, group.id(), group.nodes(), acceptIncompleteCoverage); if (invoker.isPresent()) { query.trace(false, 2, "Dispatching internally to ", group); invoker.get().teardown(() -> loadBalancer.releaseGroup(group)); return invoker; } else { - // invoker could not be produced (likely connectivity issue) - searchCluster.groupConnectionFailure(group); loadBalancer.releaseGroup(group); - if (tried == null) { - tried = new HashSet<>(); + if (rejected == null) { + rejected = new HashSet<>(); } - tried.add(group.id()); + rejected.add(group.id()); } } diff --git a/container-search/src/main/java/com/yahoo/search/dispatch/LoadBalancer.java b/container-search/src/main/java/com/yahoo/search/dispatch/LoadBalancer.java index 64e38a488ab..22573fac2d9 100644 --- a/container-search/src/main/java/com/yahoo/search/dispatch/LoadBalancer.java +++ b/container-search/src/main/java/com/yahoo/search/dispatch/LoadBalancer.java @@ -2,12 +2,14 @@ package com.yahoo.search.dispatch; import com.yahoo.search.Query; -import com.yahoo.search.dispatch.SearchCluster.Group; +import com.yahoo.search.dispatch.searchcluster.Group; +import com.yahoo.search.dispatch.searchcluster.SearchCluster; import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.Optional; +import java.util.Set; import java.util.logging.Level; import java.util.logging.Logger; @@ -47,18 +49,19 @@ public class LoadBalancer { * {@link #releaseGroup} symmetrically for each taken allocation. * * @param query the query for which this allocation is made + * @param rejectedGroups if not null, the load balancer will only return groups with IDs not in the set * @return The node group to target, or <i>empty</i> if the internal dispatch logic cannot be used */ - public Optional<Group> takeGroupForQuery(Query query) { + public Optional<Group> takeGroupForQuery(Query query, Set<Integer> rejectedGroups) { if (scoreboard == null) { return Optional.empty(); } - return allocateNextGroup(); + return allocateNextGroup(rejectedGroups); } /** - * Release an allocation given by {@link #takeGroupForQuery(Query)}. The release must be done exactly once for each allocation. + * Release an allocation given by {@link #takeGroupForQuery}. The release must be done exactly once for each allocation. * * @param group * previously allocated group @@ -74,7 +77,7 @@ public class LoadBalancer { } } - private Optional<Group> allocateNextGroup() { + private Optional<Group> allocateNextGroup(Set<Integer> rejectedGroups) { synchronized (this) { GroupSchedule bestSchedule = null; int bestIndex = needle; @@ -82,9 +85,11 @@ public class LoadBalancer { int index = needle; for (int i = 0; i < scoreboard.size(); i++) { GroupSchedule sched = scoreboard.get(index); - if (sched.isPreferredOver(bestSchedule)) { - bestSchedule = sched; - bestIndex = index; + if (rejectedGroups == null || !rejectedGroups.contains(sched.group.id())) { + if (sched.isPreferredOver(bestSchedule)) { + bestSchedule = sched; + bestIndex = index; + } } index = nextScoreboardIndex(index); } diff --git a/container-search/src/main/java/com/yahoo/search/dispatch/SearchPath.java b/container-search/src/main/java/com/yahoo/search/dispatch/SearchPath.java index 57f06225d27..6800a80b78f 100644 --- a/container-search/src/main/java/com/yahoo/search/dispatch/SearchPath.java +++ b/container-search/src/main/java/com/yahoo/search/dispatch/SearchPath.java @@ -3,7 +3,9 @@ package com.yahoo.search.dispatch; import com.google.common.collect.ImmutableCollection; import com.yahoo.collections.Pair; -import com.yahoo.search.dispatch.SearchCluster.Group; +import com.yahoo.search.dispatch.searchcluster.Group; +import com.yahoo.search.dispatch.searchcluster.Node; +import com.yahoo.search.dispatch.searchcluster.SearchCluster; import java.util.ArrayList; import java.util.Collection; @@ -37,7 +39,7 @@ public class SearchPath { * @return list of nodes chosen with the search path, or an empty list in which * case some other node selection logic should be used */ - public static List<SearchCluster.Node> selectNodes(String searchPath, SearchCluster cluster) { + public static List<Node> selectNodes(String searchPath, SearchCluster cluster) { Optional<SearchPath> sp = SearchPath.fromString(searchPath); if (sp.isPresent()) { return sp.get().mapToNodes(cluster); @@ -46,7 +48,7 @@ public class SearchPath { } } - public static Optional<SearchPath> fromString(String path) { + static Optional<SearchPath> fromString(String path) { if (path == null || path.isEmpty()) { return Optional.empty(); } @@ -73,23 +75,23 @@ public class SearchPath { this.group = group; } - private List<SearchCluster.Node> mapToNodes(SearchCluster cluster) { + private List<Node> mapToNodes(SearchCluster cluster) { if (cluster.groups().isEmpty()) { return Collections.emptyList(); } - SearchCluster.Group selectedGroup = selectGroup(cluster); + Group selectedGroup = selectGroup(cluster); if (nodes.isEmpty()) { return selectedGroup.nodes(); } - List<SearchCluster.Node> groupNodes = selectedGroup.nodes(); + List<Node> groupNodes = selectedGroup.nodes(); Set<Integer> wanted = new HashSet<>(); int max = groupNodes.size(); for (NodeSelection node : nodes) { wanted.addAll(node.matches(max)); } - List<SearchCluster.Node> ret = new ArrayList<>(); + List<Node> ret = new ArrayList<>(); for (int idx : wanted) { ret.add(groupNodes.get(idx)); } diff --git a/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/Group.java b/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/Group.java new file mode 100644 index 00000000000..01cbc5cd307 --- /dev/null +++ b/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/Group.java @@ -0,0 +1,75 @@ +// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.search.dispatch.searchcluster; + +import com.google.common.collect.ImmutableList; + +import java.util.List; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicLong; + +/** + * A group in a search cluster. This class is multithread safe. + * + * @author bratseth + * @author ollivir + */ +public class Group { + + private final int id; + private final ImmutableList<Node> nodes; + + private final AtomicBoolean hasSufficientCoverage = new AtomicBoolean(true); + private final AtomicLong activeDocuments = new AtomicLong(0); + + public Group(int id, List<Node> nodes) { + this.id = id; + this.nodes = ImmutableList.copyOf(nodes); + } + + /** Returns the unique identity of this group */ + public int id() { return id; } + + /** Returns the nodes in this group as an immutable list */ + public ImmutableList<Node> nodes() { return nodes; } + + /** + * Returns whether this group has sufficient active documents + * (compared to other groups) that is should receive traffic + */ + public boolean hasSufficientCoverage() { + return hasSufficientCoverage.get(); + } + + void setHasSufficientCoverage(boolean sufficientCoverage) { + hasSufficientCoverage.lazySet(sufficientCoverage); + } + + void aggregateActiveDocuments() { + long activeDocumentsInGroup = 0; + for (Node node : nodes) { + if (node.isWorking()) { + activeDocumentsInGroup += node.getActiveDocuments(); + } + } + activeDocuments.set(activeDocumentsInGroup); + + } + + /** Returns the active documents on this node. If unknown, 0 is returned. */ + long getActiveDocuments() { + return this.activeDocuments.get(); + } + + @Override + public String toString() { return "search group " + id; } + + @Override + public int hashCode() { return id; } + + @Override + public boolean equals(Object other) { + if (other == this) return true; + if (!(other instanceof Group)) return false; + return ((Group) other).id == this.id; + } +} diff --git a/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/Node.java b/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/Node.java new file mode 100644 index 00000000000..98deb9e3199 --- /dev/null +++ b/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/Node.java @@ -0,0 +1,73 @@ +// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.search.dispatch.searchcluster; + +import java.util.Objects; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicLong; + +/** + * A node in a search cluster. This class is multithread safe. + * + * @author bratseth + * @author ollivir + */ +public class Node { + + private final int key; + private final String hostname; + private final int fs4port; + final int group; + + private final AtomicBoolean working = new AtomicBoolean(true); + private final AtomicLong activeDocuments = new AtomicLong(0); + + public Node(int key, String hostname, int fs4port, int group) { + this.key = key; + this.hostname = hostname; + this.fs4port = fs4port; + this.group = group; + } + + /** Returns the unique and stable distribution key of this node */ + public int key() { return key; } + + public String hostname() { return hostname; } + + public int fs4port() { return fs4port; } + + /** Returns the id of this group this node belongs to */ + public int group() { return group; } + + public void setWorking(boolean working) { + this.working.lazySet(working); + } + + /** Returns whether this node is currently responding to requests */ + public boolean isWorking() { return working.get(); } + + /** Updates the active documents on this node */ + void setActiveDocuments(long activeDocuments) { + this.activeDocuments.set(activeDocuments); + } + + /** Returns the active documents on this node. If unknown, 0 is returned. */ + public long getActiveDocuments() { + return this.activeDocuments.get(); + } + + @Override + public int hashCode() { return Objects.hash(hostname, fs4port); } + + @Override + public boolean equals(Object o) { + if (o == this) return true; + if ( ! (o instanceof Node)) return false; + Node other = (Node)o; + if ( ! Objects.equals(this.hostname, other.hostname)) return false; + if ( ! Objects.equals(this.fs4port, other.fs4port)) return false; + return true; + } + + @Override + public String toString() { return "search node " + hostname + ":" + fs4port + " in group " + group; } +} diff --git a/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/Pinger.java b/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/Pinger.java new file mode 100644 index 00000000000..7c7a9cb1d1c --- /dev/null +++ b/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/Pinger.java @@ -0,0 +1,42 @@ +// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.search.dispatch.searchcluster; + +import com.yahoo.prelude.Ping; +import com.yahoo.prelude.Pong; +import com.yahoo.prelude.fastsearch.FS4ResourcePool; +import com.yahoo.prelude.fastsearch.FastSearcher; +import com.yahoo.search.cluster.ClusterMonitor; +import com.yahoo.search.result.ErrorMessage; +import com.yahoo.yolean.Exceptions; + +import java.util.concurrent.Callable; + +/** + * @author bratseth + * @author ollivir + */ +class Pinger implements Callable<Pong> { + private final Node node; + private final ClusterMonitor<Node> clusterMonitor; + private final FS4ResourcePool fs4ResourcePool; + + public Pinger(Node node, ClusterMonitor<Node> clusterMonitor, FS4ResourcePool fs4ResourcePool) { + this.node = node; + this.clusterMonitor = clusterMonitor; + this.fs4ResourcePool = fs4ResourcePool; + } + + public Pong call() { + try { + Pong pong = FastSearcher.ping(new Ping(clusterMonitor.getConfiguration().getRequestTimeout()), + fs4ResourcePool.getBackend(node.hostname(), node.fs4port()), node.toString()); + if (pong.activeDocuments().isPresent()) + node.setActiveDocuments(pong.activeDocuments().get()); + return pong; + } catch (RuntimeException e) { + return new Pong(ErrorMessage.createBackendCommunicationError("Exception when pinging " + node + ": " + + Exceptions.toMessageString(e))); + } + } + +} diff --git a/container-search/src/main/java/com/yahoo/search/dispatch/SearchCluster.java b/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/SearchCluster.java index e26dd5648eb..b0e63d20931 100644 --- a/container-search/src/main/java/com/yahoo/search/dispatch/SearchCluster.java +++ b/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/SearchCluster.java @@ -1,5 +1,5 @@ -// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -package com.yahoo.search.dispatch; +// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.search.dispatch.searchcluster; import com.google.common.collect.ImmutableCollection; import com.google.common.collect.ImmutableList; @@ -11,27 +11,18 @@ import com.yahoo.search.cluster.ClusterMonitor; import com.yahoo.search.cluster.NodeManager; import com.yahoo.search.result.ErrorMessage; import com.yahoo.vespa.config.search.DispatchConfig; - -// Only needed until query requests are moved to rpc -import com.yahoo.prelude.Ping; -import com.yahoo.prelude.fastsearch.FastSearcher; -import com.yahoo.yolean.Exceptions; import com.yahoo.prelude.Pong; import com.yahoo.prelude.fastsearch.FS4ResourcePool; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; -import java.util.Objects; import java.util.Optional; -import java.util.concurrent.Callable; import java.util.concurrent.ExecutionException; import java.util.concurrent.Executor; import java.util.concurrent.FutureTask; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; -import java.util.concurrent.atomic.AtomicBoolean; -import java.util.concurrent.atomic.AtomicLong; import java.util.logging.Level; import java.util.logging.Logger; import java.util.stream.Collectors; @@ -41,7 +32,7 @@ import java.util.stream.Collectors; * * @author bratseth */ -public class SearchCluster implements NodeManager<SearchCluster.Node> { +public class SearchCluster implements NodeManager<Node> { private static final Logger log = Logger.getLogger(SearchCluster.class.getName()); @@ -128,8 +119,8 @@ public class SearchCluster implements NodeManager<SearchCluster.Node> { // Only use direct dispatch if we have exactly 1 search node on the same machine: if (localSearchNodes.size() != 1) return Optional.empty(); - SearchCluster.Node localSearchNode = localSearchNodes.iterator().next(); - SearchCluster.Group localSearchGroup = groups.get(localSearchNode.group()); + Node localSearchNode = localSearchNodes.iterator().next(); + Group localSearchGroup = groups.get(localSearchNode.group()); // Only use direct dispatch if the local search node has the entire corpus if (localSearchGroup.nodes().size() != 1) return Optional.empty(); @@ -188,7 +179,7 @@ public class SearchCluster implements NodeManager<SearchCluster.Node> { if ( ! directDispatchTarget.isPresent()) return Optional.empty(); // Only use direct dispatch if the local group has sufficient coverage - SearchCluster.Group localSearchGroup = groups.get(directDispatchTarget.get().group()); + Group localSearchGroup = groups.get(directDispatchTarget.get().group()); if ( ! localSearchGroup.hasSufficientCoverage()) return Optional.empty(); // Only use direct dispatch if the local search node is up @@ -216,10 +207,6 @@ public class SearchCluster implements NodeManager<SearchCluster.Node> { vipStatus.removeFromRotation(this); } - public void groupConnectionFailure(Group group) { - group.setHasSufficientCoverage(false); // will be reset after next ping iteration - } - private void updateSufficientCoverage(Group group, boolean sufficientCoverage) { // update VIP status if we direct dispatch to this group and coverage status changed if (usesDirectDispatchTo(group) && sufficientCoverage != group.hasSufficientCoverage()) { @@ -245,7 +232,7 @@ public class SearchCluster implements NodeManager<SearchCluster.Node> { /** Used by the cluster monitor to manage node status */ @Override public void ping(Node node, Executor executor) { - Pinger pinger = new Pinger(node); + Pinger pinger = new Pinger(node, clusterMonitor, fs4ResourcePool); FutureTask<Pong> futurePong = new FutureTask<>(pinger); executor.execute(futurePong); Pong pong = getPong(futurePong, node); @@ -287,29 +274,34 @@ public class SearchCluster implements NodeManager<SearchCluster.Node> { Group group = orderedGroups.get(i); long activeDocuments = activeDocumentsInGroup[i]; long averageDocumentsInOtherGroups = (sumOfActiveDocuments - activeDocuments) / (numGroups - 1); - boolean sufficientCoverage = true; - - if (averageDocumentsInOtherGroups > 0) { - double coverage = 100.0 * (double) activeDocuments / averageDocumentsInOtherGroups; - sufficientCoverage = coverage >= minActivedocsCoveragePercentage; - } - if (sufficientCoverage) { - sufficientCoverage = isNodeCoverageSufficient(group); - } + boolean sufficientCoverage = isGroupCoverageSufficient(group.nodes(), activeDocuments, averageDocumentsInOtherGroups); updateSufficientCoverage(group, sufficientCoverage); } } - private boolean isNodeCoverageSufficient(Group group) { + private boolean isGroupCoverageSufficient(List<Node> nodes, long activeDocuments, long averageDocumentsInOtherGroups) { + boolean sufficientCoverage = true; + + if (averageDocumentsInOtherGroups > 0) { + double coverage = 100.0 * (double) activeDocuments / averageDocumentsInOtherGroups; + sufficientCoverage = coverage >= minActivedocsCoveragePercentage; + } + if (sufficientCoverage) { + sufficientCoverage = isGroupNodeCoverageSufficient(nodes); + } + return sufficientCoverage; + } + + private boolean isGroupNodeCoverageSufficient(List<Node> nodes) { int nodesUp = 0; - for (Node node : group.nodes()) { + for (Node node : nodes) { if (node.isWorking()) { nodesUp++; } } - int nodes = group.nodes().size(); - int nodesAllowedDown = maxNodesDownPerGroup + (int) (((double) nodes * (100.0 - minGroupCoverage)) / 100.0); - return nodesUp + nodesAllowedDown >= nodes; + int numNodes = nodes.size(); + int nodesAllowedDown = maxNodesDownPerGroup + (int) (((double) numNodes * (100.0 - minGroupCoverage)) / 100.0); + return nodesUp + nodesAllowedDown >= numNodes; } private Pong getPong(FutureTask<Pong> futurePong, Node node) { @@ -326,153 +318,26 @@ public class SearchCluster implements NodeManager<SearchCluster.Node> { } } - private class Pinger implements Callable<Pong> { - - private final Node node; - - public Pinger(Node node) { - this.node = node; - } - - public Pong call() { - try { - Pong pong = FastSearcher.ping(new Ping(clusterMonitor.getConfiguration().getRequestTimeout()), - fs4ResourcePool.getBackend(node.hostname(), node.fs4port()), node.toString()); - if (pong.activeDocuments().isPresent()) - node.setActiveDocuments(pong.activeDocuments().get()); - return pong; - } catch (RuntimeException e) { - return new Pong(ErrorMessage.createBackendCommunicationError("Exception when pinging " + node + ": " - + Exceptions.toMessageString(e))); - } - } - - } - - /** A group in a search cluster. This class is multithread safe. */ - public static class Group { - - private final int id; - private final ImmutableList<Node> nodes; - - private final AtomicBoolean hasSufficientCoverage = new AtomicBoolean(true); - private final AtomicLong activeDocuments = new AtomicLong(0); - - public Group(int id, List<Node> nodes) { - this.id = id; - this.nodes = ImmutableList.copyOf(nodes); - } - - /** Returns the unique identity of this group */ - public int id() { return id; } - - /** Returns the nodes in this group as an immutable list */ - public ImmutableList<Node> nodes() { return nodes; } - - /** - * Returns whether this group has sufficient active documents - * (compared to other groups) that is should receive traffic - */ - public boolean hasSufficientCoverage() { - return hasSufficientCoverage.get(); - } - - void setHasSufficientCoverage(boolean sufficientCoverage) { - hasSufficientCoverage.lazySet(sufficientCoverage); + /** + * Calculate whether a subset of nodes in a group has enough coverage + */ + public boolean isPartialGroupCoverageSufficient(int groupId, List<Node> nodes) { + if (orderedGroups.size() == 1) { + return true; } - - void aggregateActiveDocuments() { - long activeDocumentsInGroup = 0; - for (Node node : nodes) { - if (node.isWorking()) { - activeDocumentsInGroup += node.getActiveDocuments(); - } + long sumOfActiveDocuments = 0; + int otherGroups = 0; + for (Group g : orderedGroups) { + if (g.id() != groupId) { + sumOfActiveDocuments += g.getActiveDocuments(); + otherGroups++; } - activeDocuments.set(activeDocumentsInGroup); - } - - /** Returns the active documents on this node. If unknown, 0 is returned. */ - long getActiveDocuments() { - return this.activeDocuments.get(); + long activeDocuments = 0; + for (Node n : nodes) { + activeDocuments += n.getActiveDocuments(); } - - @Override - public String toString() { return "search group " + id; } - - @Override - public int hashCode() { return id; } - - @Override - public boolean equals(Object other) { - if (other == this) return true; - if (!(other instanceof Group)) return false; - return ((Group) other).id == this.id; - } - + long averageDocumentsInOtherGroups = sumOfActiveDocuments / otherGroups; + return isGroupCoverageSufficient(nodes, activeDocuments, averageDocumentsInOtherGroups); } - - /** A node in a search cluster. This class is multithread safe. */ - public static class Node { - - private final int key; - private final String hostname; - private final int fs4port; - private final int group; - - private final AtomicBoolean working = new AtomicBoolean(true); - private final AtomicLong activeDocuments = new AtomicLong(0); - - public Node(int key, String hostname, int fs4port, int group) { - this.key = key; - this.hostname = hostname; - this.fs4port = fs4port; - this.group = group; - } - - /** Returns the unique and stable distribution key of this node */ - public int key() { return key; } - - public String hostname() { return hostname; } - - public int fs4port() { return fs4port; } - - /** Returns the id of this group this node belongs to */ - public int group() { return group; } - - void setWorking(boolean working) { - this.working.lazySet(working); - } - - /** Returns whether this node is currently responding to requests */ - public boolean isWorking() { return working.get(); } - - /** Updates the active documents on this node */ - void setActiveDocuments(long activeDocuments) { - this.activeDocuments.set(activeDocuments); - } - - /** Returns the active documents on this node. If unknown, 0 is returned. */ - public long getActiveDocuments() { - return this.activeDocuments.get(); - } - - @Override - public int hashCode() { return Objects.hash(hostname, fs4port); } - - @Override - public boolean equals(Object o) { - if (o == this) return true; - if ( ! (o instanceof Node)) return false; - Node other = (Node)o; - if ( ! Objects.equals(this.hostname, other.hostname)) return false; - if ( ! Objects.equals(this.fs4port, other.fs4port)) return false; - return true; - } - - @Override - public String toString() { return "search node " + hostname + ":" + fs4port + " in group " + group; } - - } - } diff --git a/container-search/src/main/java/com/yahoo/search/query/ranking/SoftTimeout.java b/container-search/src/main/java/com/yahoo/search/query/ranking/SoftTimeout.java index 1fad45a99e3..ca6fd44af50 100644 --- a/container-search/src/main/java/com/yahoo/search/query/ranking/SoftTimeout.java +++ b/container-search/src/main/java/com/yahoo/search/query/ranking/SoftTimeout.java @@ -8,7 +8,7 @@ import com.yahoo.search.query.profile.types.QueryProfileType; import java.util.Objects; /** - * Holds the settings for the soft-timeout feature. + * Settings for the soft-timeout feature. * * @author baldersheim */ @@ -41,22 +41,24 @@ public class SoftTimeout implements Cloneable { public void setFactor(double factor) { if ((factor < 0.0) || (factor > 1.0)) { - throw new IllegalArgumentException("factor must be in the range [0.0, 1.0]. It is " + factor); + throw new IllegalArgumentException("factor must be in the range [0.0, 1.0], got " + factor); } this.factor = factor; } + public Double getFactor() { return factor; } + public void setTailcost(double tailcost) { if ((tailcost < 0.0) || (tailcost > 1.0)) { - throw new IllegalArgumentException("tailcost must be in the range [0.0, 1.0]. It is " + tailcost); + throw new IllegalArgumentException("tailcost must be in the range [0.0, 1.0], got " + tailcost); } this.tailcost = tailcost; } + public Double getTailcost() { return tailcost; } /** Internal operation - DO NOT USE */ public void prepare(RankProperties rankProperties) { - if (enable != null) { rankProperties.put("vespa.softtimeout.enable", String.valueOf(enable)); } diff --git a/container-search/src/test/java/com/yahoo/prelude/fastsearch/test/FastSearcherTestCase.java b/container-search/src/test/java/com/yahoo/prelude/fastsearch/test/FastSearcherTestCase.java index b08aef6ecb1..79b43563c6a 100644 --- a/container-search/src/test/java/com/yahoo/prelude/fastsearch/test/FastSearcherTestCase.java +++ b/container-search/src/test/java/com/yahoo/prelude/fastsearch/test/FastSearcherTestCase.java @@ -5,26 +5,37 @@ import com.google.common.collect.ImmutableList; import com.yahoo.component.chain.Chain; import com.yahoo.config.subscription.ConfigGetter; import com.yahoo.container.handler.VipStatus; +import com.yahoo.container.protect.Error; import com.yahoo.container.search.Fs4Config; import com.yahoo.document.GlobalId; -import com.yahoo.fs4.mplex.*; +import com.yahoo.fs4.BasicPacket; +import com.yahoo.fs4.Packet; +import com.yahoo.fs4.QueryPacket; +import com.yahoo.fs4.mplex.Backend; +import com.yahoo.fs4.mplex.BackendTestCase; import com.yahoo.fs4.test.QueryTestCase; import com.yahoo.language.simple.SimpleLinguistics; import com.yahoo.prelude.Ping; import com.yahoo.prelude.Pong; +import com.yahoo.prelude.fastsearch.CacheControl; +import com.yahoo.prelude.fastsearch.CacheKey; +import com.yahoo.prelude.fastsearch.CacheParams; +import com.yahoo.prelude.fastsearch.ClusterParams; import com.yahoo.prelude.fastsearch.DocumentdbInfoConfig; -import com.yahoo.container.protect.Error; -import com.yahoo.fs4.*; +import com.yahoo.prelude.fastsearch.FS4ResourcePool; +import com.yahoo.prelude.fastsearch.FastHit; +import com.yahoo.prelude.fastsearch.FastSearcher; +import com.yahoo.prelude.fastsearch.PacketWrapper; +import com.yahoo.prelude.fastsearch.SummaryParameters; import com.yahoo.prelude.fastsearch.test.fs4mock.MockBackend; import com.yahoo.prelude.fastsearch.test.fs4mock.MockFS4ResourcePool; import com.yahoo.prelude.fastsearch.test.fs4mock.MockFSChannel; +import com.yahoo.prelude.query.WordItem; import com.yahoo.processing.execution.Execution.Trace; import com.yahoo.search.Query; import com.yahoo.search.Result; import com.yahoo.search.Searcher; -import com.yahoo.prelude.fastsearch.*; -import com.yahoo.prelude.query.WordItem; -import com.yahoo.search.dispatch.SearchCluster; +import com.yahoo.search.dispatch.searchcluster.Node; import com.yahoo.search.grouping.GroupingRequest; import com.yahoo.search.grouping.request.AllOperation; import com.yahoo.search.grouping.request.EachOperation; @@ -48,9 +59,13 @@ import java.util.logging.Level; import java.util.logging.Logger; import static org.hamcrest.CoreMatchers.containsString; -import static org.junit.Assert.*; import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertTrue; /** * Tests the Fast searcher @@ -87,7 +102,7 @@ public class FastSearcherTestCase { @Test public void testNullQuery() { Logger.getLogger(FastSearcher.class.getName()).setLevel(Level.ALL); - FastSearcher fastSearcher = new FastSearcher(new MockBackend(), + FastSearcher fastSearcher = new FastSearcher(new MockBackend(), new FS4ResourcePool(1), new MockDispatcher(Collections.emptyList()), new SummaryParameters(null), @@ -115,9 +130,9 @@ public class FastSearcherTestCase { .rankprofile(new DocumentdbInfoConfig.Documentdb.Rankprofile.Builder() .name("simpler").hasRankFeatures(false).hasSummaryFeatures(false)))); - List<SearchCluster.Node> nodes = new ArrayList<>(); - nodes.add(new SearchCluster.Node(0, "host1", 5000, 0)); - nodes.add(new SearchCluster.Node(2, "host2", 5000, 0)); + List<Node> nodes = new ArrayList<>(); + nodes.add(new Node(0, "host1", 5000, 0)); + nodes.add(new Node(2, "host2", 5000, 0)); MockFS4ResourcePool mockFs4ResourcePool = new MockFS4ResourcePool(); FastSearcher fastSearcher = new FastSearcher(new MockBackend(), @@ -162,15 +177,15 @@ public class FastSearcherTestCase { new DocumentdbInfoConfig(new DocumentdbInfoConfig.Builder().documentdb(new DocumentdbInfoConfig.Documentdb.Builder().name("testDb"))); FastSearcher fastSearcher = new FastSearcher(mockBackend, new FS4ResourcePool(1), - new MockDispatcher(Collections.emptyList()), + new MockDispatcher(Collections.emptyList()), new SummaryParameters(null), new ClusterParams("testhittype"), - new CacheParams(100, 1e64), + new CacheParams(100, 1e64), documentdbConfigWithOneDb); Query query = new Query("?query=foo&model.restrict=testDb"); query.prepare(); - Result result = doSearch(fastSearcher, query, 0, 10); + doSearch(fastSearcher, query, 0, 10); Packet receivedPacket = mockBackend.getChannel().getLastQueryPacket(); byte[] encoded = QueryTestCase.packetToBytes(receivedPacket); @@ -354,10 +369,10 @@ public class FastSearcherTestCase { Logger.getLogger(FastSearcher.class.getName()).setLevel(Level.ALL); return new FastSearcher(mockBackend, new FS4ResourcePool(1), - new MockDispatcher(Collections.emptyList()), + new MockDispatcher(Collections.emptyList()), new SummaryParameters(null), - new ClusterParams("testhittype"), - new CacheParams(100, 1e64), + new ClusterParams("testhittype"), + new CacheParams(100, 1e64), config); } @@ -436,12 +451,12 @@ public class FastSearcherTestCase { } } - + @Test public void testSinglePassGroupingIsForcedWithSingleNodeGroups() { FastSearcher fastSearcher = new FastSearcher(new MockBackend(), new FS4ResourcePool(1), - new MockDispatcher(new SearchCluster.Node(0, "host0", 123, 0)), + new MockDispatcher(new Node(0, "host0", 123, 0)), new SummaryParameters(null), new ClusterParams("testhittype"), new CacheParams(100, 1e64), @@ -455,17 +470,17 @@ public class FastSearcherTestCase { all.addChild(new EachOperation()); all.addChild(new EachOperation()); request2.setRootOperation(all); - + assertForceSinglePassIs(false, q); fastSearcher.search(q, new Execution(Execution.Context.createContextStub())); - assertForceSinglePassIs(true, q); + assertForceSinglePassIs(true, q); } @Test public void testSinglePassGroupingIsNotForcedWithSingleNodeGroups() { - MockDispatcher dispatcher = - new MockDispatcher(ImmutableList.of(new SearchCluster.Node(0, "host0", 123, 0), - new SearchCluster.Node(2, "host1", 123, 0))); + MockDispatcher dispatcher = + new MockDispatcher(ImmutableList.of(new Node(0, "host0", 123, 0), + new Node(2, "host1", 123, 0))); FastSearcher fastSearcher = new FastSearcher(new MockBackend(), new FS4ResourcePool(1), @@ -495,7 +510,7 @@ public class FastSearcherTestCase { } private void assertForceSinglePassIs(boolean expected, GroupingOperation operation) { - assertEquals("Force single pass is " + expected + " in " + operation, + assertEquals("Force single pass is " + expected + " in " + operation, expected, operation.getForceSinglePass()); for (GroupingOperation child : operation.getChildren()) assertForceSinglePassIs(expected, child); diff --git a/container-search/src/test/java/com/yahoo/prelude/fastsearch/test/FastSearcherTester.java b/container-search/src/test/java/com/yahoo/prelude/fastsearch/test/FastSearcherTester.java index 4f6d2d88917..12c313dbfe3 100644 --- a/container-search/src/test/java/com/yahoo/prelude/fastsearch/test/FastSearcherTester.java +++ b/container-search/src/test/java/com/yahoo/prelude/fastsearch/test/FastSearcherTester.java @@ -12,16 +12,14 @@ import com.yahoo.prelude.fastsearch.FastSearcher; import com.yahoo.prelude.fastsearch.SummaryParameters; import com.yahoo.prelude.fastsearch.test.fs4mock.MockBackend; import com.yahoo.prelude.fastsearch.test.fs4mock.MockFS4ResourcePool; -import com.yahoo.prelude.fastsearch.test.fs4mock.MockFSChannel; import com.yahoo.search.Query; import com.yahoo.search.Result; -import com.yahoo.search.dispatch.SearchCluster; +import com.yahoo.search.dispatch.searchcluster.Node; import com.yahoo.search.searchchain.Execution; import java.util.ArrayList; import java.util.Collections; import java.util.List; -import java.util.Optional; import static org.junit.Assert.assertEquals; import static org.junit.Assert.fail; @@ -38,7 +36,7 @@ class FastSearcherTester { private final MockDispatcher mockDispatcher; private final VipStatus vipStatus; - public FastSearcherTester(int containerClusterSize, SearchCluster.Node searchNode) { + public FastSearcherTester(int containerClusterSize, Node searchNode) { this(containerClusterSize, Collections.singletonList(searchNode)); } @@ -46,7 +44,7 @@ class FastSearcherTester { this(containerClusterSize, toNodes(hostAndPortAndGroupStrings)); } - public FastSearcherTester(int containerClusterSize, List<SearchCluster.Node> searchNodes) { + public FastSearcherTester(int containerClusterSize, List<Node> searchNodes) { ClustersStatus clustersStatus = new ClustersStatus(); clustersStatus.setContainerHasClusters(true); vipStatus = new VipStatus(clustersStatus); @@ -61,12 +59,12 @@ class FastSearcherTester { new DocumentdbInfoConfig(new DocumentdbInfoConfig.Builder())); } - private static List<SearchCluster.Node> toNodes(String... hostAndPortAndGroupStrings) { - List<SearchCluster.Node> nodes = new ArrayList<>(); + private static List<Node> toNodes(String... hostAndPortAndGroupStrings) { + List<Node> nodes = new ArrayList<>(); int key = 0; for (String s : hostAndPortAndGroupStrings) { String[] parts = s.split(":"); - nodes.add(new SearchCluster.Node(key++, parts[0], Integer.parseInt(parts[1]), Integer.parseInt(parts[2]))); + nodes.add(new Node(key++, parts[0], Integer.parseInt(parts[1]), Integer.parseInt(parts[2]))); } return nodes; } @@ -90,7 +88,7 @@ class FastSearcherTester { mockFS4ResourcePool.setResponding(hostname, responding); // Make the search cluster monitor notice right now in this thread - SearchCluster.Node node = mockDispatcher.searchCluster().nodesByHost().get(hostname).iterator().next(); + Node node = mockDispatcher.searchCluster().nodesByHost().get(hostname).iterator().next(); mockDispatcher.searchCluster().ping(node, MoreExecutors.directExecutor()); } @@ -99,7 +97,7 @@ class FastSearcherTester { mockFS4ResourcePool.setActiveDocuments(hostname, activeDocuments); // Make the search cluster monitor notice right now in this thread - SearchCluster.Node node = mockDispatcher.searchCluster().nodesByHost().get(hostname).iterator().next(); + Node node = mockDispatcher.searchCluster().nodesByHost().get(hostname).iterator().next(); mockDispatcher.searchCluster().ping(node, MoreExecutors.directExecutor()); mockDispatcher.searchCluster().pingIterationCompleted(); } diff --git a/container-search/src/test/java/com/yahoo/prelude/fastsearch/test/MockDispatcher.java b/container-search/src/test/java/com/yahoo/prelude/fastsearch/test/MockDispatcher.java index 2b25b2a3796..800b1bc21f0 100644 --- a/container-search/src/test/java/com/yahoo/prelude/fastsearch/test/MockDispatcher.java +++ b/container-search/src/test/java/com/yahoo/prelude/fastsearch/test/MockDispatcher.java @@ -5,7 +5,7 @@ import com.yahoo.container.handler.VipStatus; import com.yahoo.prelude.fastsearch.FS4ResourcePool; import com.yahoo.search.Result; import com.yahoo.search.dispatch.Dispatcher; -import com.yahoo.search.dispatch.SearchCluster; +import com.yahoo.search.dispatch.searchcluster.Node; import com.yahoo.vespa.config.search.DispatchConfig; import java.util.Collections; @@ -13,27 +13,27 @@ import java.util.List; class MockDispatcher extends Dispatcher { - public MockDispatcher(SearchCluster.Node node) { + public MockDispatcher(Node node) { this(Collections.singletonList(node)); } - public MockDispatcher(List<SearchCluster.Node> nodes) { + public MockDispatcher(List<Node> nodes) { super(toDispatchConfig(nodes), new FS4ResourcePool(1), 1, new VipStatus()); } - public MockDispatcher(List<SearchCluster.Node> nodes, VipStatus vipStatus) { + public MockDispatcher(List<Node> nodes, VipStatus vipStatus) { super(toDispatchConfig(nodes), new FS4ResourcePool(1), 1, vipStatus); } - public MockDispatcher(List<SearchCluster.Node> nodes, FS4ResourcePool fs4ResourcePool, + public MockDispatcher(List<Node> nodes, FS4ResourcePool fs4ResourcePool, int containerClusterSize, VipStatus vipStatus) { super(toDispatchConfig(nodes), fs4ResourcePool, containerClusterSize, vipStatus); } - private static DispatchConfig toDispatchConfig(List<SearchCluster.Node> nodes) { + private static DispatchConfig toDispatchConfig(List<Node> nodes) { DispatchConfig.Builder dispatchConfigBuilder = new DispatchConfig.Builder(); int key = 0; - for (SearchCluster.Node node : nodes) { + for (Node node : nodes) { DispatchConfig.Node.Builder dispatchConfigNodeBuilder = new DispatchConfig.Node.Builder(); dispatchConfigNodeBuilder.host(node.hostname()); dispatchConfigNodeBuilder.fs4port(node.fs4port()); @@ -44,7 +44,7 @@ class MockDispatcher extends Dispatcher { } return new DispatchConfig(dispatchConfigBuilder); } - + public void fill(Result result, String summaryClass) { } diff --git a/container-search/src/test/java/com/yahoo/search/dispatch/LoadBalancerTest.java b/container-search/src/test/java/com/yahoo/search/dispatch/LoadBalancerTest.java index 9311ddab3c6..698cee743e4 100644 --- a/container-search/src/test/java/com/yahoo/search/dispatch/LoadBalancerTest.java +++ b/container-search/src/test/java/com/yahoo/search/dispatch/LoadBalancerTest.java @@ -2,8 +2,9 @@ package com.yahoo.search.dispatch; import com.yahoo.search.Query; -import com.yahoo.search.dispatch.SearchCluster.Group; -import com.yahoo.search.dispatch.SearchCluster.Node; +import com.yahoo.search.dispatch.searchcluster.Group; +import com.yahoo.search.dispatch.searchcluster.Node; +import com.yahoo.search.dispatch.searchcluster.SearchCluster; import junit.framework.AssertionFailedError; import org.junit.Test; @@ -21,11 +22,11 @@ import static org.junit.Assert.assertThat; public class LoadBalancerTest { @Test public void requreThatLoadBalancerServesSingleNodeSetups() { - Node n1 = new SearchCluster.Node(0, "test-node1", 0, 0); + Node n1 = new Node(0, "test-node1", 0, 0); SearchCluster cluster = new SearchCluster(88.0, 99.0, 0, Arrays.asList(n1), null, 1, null); LoadBalancer lb = new LoadBalancer(cluster, true); - Optional<Group> grp = lb.takeGroupForQuery(new Query()); + Optional<Group> grp = lb.takeGroupForQuery(new Query(), null); Group group = grp.orElseGet(() -> { throw new AssertionFailedError("Expected a SearchCluster.Group"); }); @@ -34,12 +35,12 @@ public class LoadBalancerTest { @Test public void requreThatLoadBalancerServesMultiGroupSetups() { - Node n1 = new SearchCluster.Node(0, "test-node1", 0, 0); - Node n2 = new SearchCluster.Node(1, "test-node2", 1, 1); + Node n1 = new Node(0, "test-node1", 0, 0); + Node n2 = new Node(1, "test-node2", 1, 1); SearchCluster cluster = new SearchCluster(88.0, 99.0, 0, Arrays.asList(n1, n2), null, 1, null); LoadBalancer lb = new LoadBalancer(cluster, true); - Optional<Group> grp = lb.takeGroupForQuery(new Query()); + Optional<Group> grp = lb.takeGroupForQuery(new Query(), null); Group group = grp.orElseGet(() -> { throw new AssertionFailedError("Expected a SearchCluster.Group"); }); @@ -48,51 +49,51 @@ public class LoadBalancerTest { @Test public void requreThatLoadBalancerServesClusteredGroups() { - Node n1 = new SearchCluster.Node(0, "test-node1", 0, 0); - Node n2 = new SearchCluster.Node(1, "test-node2", 1, 0); - Node n3 = new SearchCluster.Node(0, "test-node3", 0, 1); - Node n4 = new SearchCluster.Node(1, "test-node4", 1, 1); + Node n1 = new Node(0, "test-node1", 0, 0); + Node n2 = new Node(1, "test-node2", 1, 0); + Node n3 = new Node(0, "test-node3", 0, 1); + Node n4 = new Node(1, "test-node4", 1, 1); SearchCluster cluster = new SearchCluster(88.0, 99.0, 0, Arrays.asList(n1, n2, n3, n4), null, 2, null); LoadBalancer lb = new LoadBalancer(cluster, true); - Optional<Group> grp = lb.takeGroupForQuery(new Query()); + Optional<Group> grp = lb.takeGroupForQuery(new Query(), null); assertThat(grp.isPresent(), is(true)); } @Test public void requreThatLoadBalancerReturnsDifferentGroups() { - Node n1 = new SearchCluster.Node(0, "test-node1", 0, 0); - Node n2 = new SearchCluster.Node(1, "test-node2", 1, 1); + Node n1 = new Node(0, "test-node1", 0, 0); + Node n2 = new Node(1, "test-node2", 1, 1); SearchCluster cluster = new SearchCluster(88.0, 99.0, 0, Arrays.asList(n1, n2), null, 1, null); LoadBalancer lb = new LoadBalancer(cluster, true); // get first group - Optional<Group> grp = lb.takeGroupForQuery(new Query()); + Optional<Group> grp = lb.takeGroupForQuery(new Query(), null); Group group = grp.get(); int id1 = group.id(); // release allocation lb.releaseGroup(group); // get second group - grp = lb.takeGroupForQuery(new Query()); + grp = lb.takeGroupForQuery(new Query(), null); group = grp.get(); assertThat(group.id(), not(equalTo(id1))); } @Test public void requreThatLoadBalancerReturnsGroupWithShortestQueue() { - Node n1 = new SearchCluster.Node(0, "test-node1", 0, 0); - Node n2 = new SearchCluster.Node(1, "test-node2", 1, 1); + Node n1 = new Node(0, "test-node1", 0, 0); + Node n2 = new Node(1, "test-node2", 1, 1); SearchCluster cluster = new SearchCluster(88.0, 99.0, 0, Arrays.asList(n1, n2), null, 1, null); LoadBalancer lb = new LoadBalancer(cluster, true); // get first group - Optional<Group> grp = lb.takeGroupForQuery(new Query()); + Optional<Group> grp = lb.takeGroupForQuery(new Query(), null); Group group = grp.get(); int id1 = group.id(); // get second group - grp = lb.takeGroupForQuery(new Query()); + grp = lb.takeGroupForQuery(new Query(), null); group = grp.get(); int id2 = group.id(); assertThat(id2, not(equalTo(id1))); @@ -100,7 +101,7 @@ public class LoadBalancerTest { lb.releaseGroup(group); // get third group - grp = lb.takeGroupForQuery(new Query()); + grp = lb.takeGroupForQuery(new Query(), null); group = grp.get(); assertThat(group.id(), equalTo(id2)); } diff --git a/container-search/src/test/java/com/yahoo/search/dispatch/MockSearchCluster.java b/container-search/src/test/java/com/yahoo/search/dispatch/MockSearchCluster.java index ee903fd3fa0..0c0a65ded17 100644 --- a/container-search/src/test/java/com/yahoo/search/dispatch/MockSearchCluster.java +++ b/container-search/src/test/java/com/yahoo/search/dispatch/MockSearchCluster.java @@ -3,6 +3,9 @@ package com.yahoo.search.dispatch; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableMultimap; +import com.yahoo.search.dispatch.searchcluster.Group; +import com.yahoo.search.dispatch.searchcluster.Node; +import com.yahoo.search.dispatch.searchcluster.SearchCluster; import java.util.ArrayList; import java.util.Collections; diff --git a/container-search/src/test/java/com/yahoo/search/dispatch/SearchPathTest.java b/container-search/src/test/java/com/yahoo/search/dispatch/SearchPathTest.java index 77cb8d5c353..a1f926d3201 100644 --- a/container-search/src/test/java/com/yahoo/search/dispatch/SearchPathTest.java +++ b/container-search/src/test/java/com/yahoo/search/dispatch/SearchPathTest.java @@ -2,6 +2,7 @@ package com.yahoo.search.dispatch; import com.yahoo.search.dispatch.SearchPath.InvalidSearchPathException; +import com.yahoo.search.dispatch.searchcluster.Node; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; @@ -77,7 +78,7 @@ public class SearchPathTest { assertThat(distKeysAsString(SearchPath.selectNodes("[1,88>/1", cluster)), equalTo("5,6")); } - private static String distKeysAsString(Collection<SearchCluster.Node> nodes) { - return nodes.stream().map(SearchCluster.Node::key).map(Object::toString).collect(Collectors.joining(",")); + private static String distKeysAsString(Collection<Node> nodes) { + return nodes.stream().map(Node::key).map(Object::toString).collect(Collectors.joining(",")); } } diff --git a/container-search/src/test/java/com/yahoo/search/query/SoftTimeoutTestCase.java b/container-search/src/test/java/com/yahoo/search/query/SoftTimeoutTestCase.java index 01501674ae8..83ef955a6d9 100644 --- a/container-search/src/test/java/com/yahoo/search/query/SoftTimeoutTestCase.java +++ b/container-search/src/test/java/com/yahoo/search/query/SoftTimeoutTestCase.java @@ -37,7 +37,7 @@ public class SoftTimeoutTestCase { } catch (QueryException e) { assertEquals("Invalid request parameter", e.getMessage()); assertEquals("Could not set 'ranking.softtimeout." + key + "' to '" + value +"'", e.getCause().getMessage()); - assertEquals(key + " must be in the range [0.0, 1.0]. It is " + value, e.getCause().getCause().getMessage()); + assertEquals(key + " must be in the range [0.0, 1.0], got " + value, e.getCause().getCause().getMessage()); } } @Test diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainer.java index b89cbaa4c82..22d738ff98c 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainer.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainer.java @@ -13,7 +13,7 @@ import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneId; import com.yahoo.vespa.hosted.controller.application.Deployment; import com.yahoo.vespa.hosted.controller.application.RotationStatus; import com.yahoo.vespa.hosted.controller.authority.config.ApiAuthorityConfig; -import com.yahoo.yolean.Exceptions; +import org.apache.http.HttpResponse; import org.apache.http.client.methods.HttpPost; import org.apache.http.entity.ByteArrayEntity; import org.apache.http.impl.client.CloseableHttpClient; @@ -155,11 +155,14 @@ public class DeploymentMetricsMaintainer extends Maintainer { } private void feedMetrics(Slime slime) throws IOException { - String uri = baseUris.get(0) + "/metricforwarding/v1/deploymentmetrics/"; // For now, we only feed to one controller + String uri = baseUris.get(0) + "/metricforwarding/v1/deploymentmetrics"; // For now, we only feed to one controller CloseableHttpClient httpClient = HttpClientBuilder.create().build(); HttpPost httpPost = new HttpPost(uri); httpPost.setEntity(new ByteArrayEntity(SlimeUtils.toJsonBytes(slime))); - httpClient.execute(httpPost); + HttpResponse response = httpClient.execute(httpPost); + if (response.getStatusLine().getStatusCode() != 200) { + log.log(Level.WARNING, "Could not feed metrics. Reason: " + response.getStatusLine().getReasonPhrase()); + } } private static RotationStatus from(com.yahoo.vespa.hosted.controller.api.integration.routing.RotationStatus status) { diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/ControllerAuthorizationFilter.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/ControllerAuthorizationFilter.java index b1e3f8799d6..ccd943e7f0a 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/ControllerAuthorizationFilter.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/ControllerAuthorizationFilter.java @@ -81,10 +81,10 @@ public class ControllerAuthorizationFilter extends CorsRequestFilterBase { @Override public Optional<ErrorResponse> filterRequest(DiscFilterRequest request) { Method method = getMethod(request); - if (isWhiteListedMethod(method)) return Optional.empty(); + Path path = new Path(request.getRequestURI()); + if (isWhiteListed(method, path)) return Optional.empty(); try { - Path path = new Path(request.getRequestURI()); AthenzPrincipal principal = getPrincipalOrThrow(request); if (isWhiteListedOperation(path, method)) { // no authz check @@ -106,8 +106,10 @@ public class ControllerAuthorizationFilter extends CorsRequestFilterBase { } } - private static boolean isWhiteListedMethod(Method method) { - return WHITELISTED_METHODS.contains(method); + private static boolean isWhiteListed(Method method, Path path) { + return WHITELISTED_METHODS.contains(method) || + path.matches("/metricforwarding/v1/{*}") && method == POST || + path.matches("/contactinfo/v1/{*}") && method == POST; } private static boolean isWhiteListedOperation(Path path, Method method) { diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/metrics/MetricForwardingApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/metrics/MetricForwardingApiHandler.java index 91c847d10f1..f3112b09173 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/metrics/MetricForwardingApiHandler.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/metrics/MetricForwardingApiHandler.java @@ -26,6 +26,7 @@ import com.yahoo.vespa.hosted.controller.restapi.StringResponse; import java.io.IOException; import java.util.HashMap; import java.util.Map; +import java.util.logging.Level; /** * This implements the metricforwarding/v1 API which allows feeding @@ -72,7 +73,8 @@ public class MetricForwardingApiHandler extends LoggingRequestHandler { }); }); } catch (IOException e) { - ErrorResponse.badRequest("Unable to parse request for metrics - " + e.getMessage()); + log.log(Level.WARNING, "Unable to parse request for cluster utilization metrics", e); + return ErrorResponse.badRequest("Unable to parse request for cluster utilization metrics - " + e.getMessage()); } return new StringResponse("Added cluster utilization metrics"); } @@ -99,7 +101,8 @@ public class MetricForwardingApiHandler extends LoggingRequestHandler { } }); } catch (IOException e) { - ErrorResponse.badRequest("Unable to parse request for metrics - " + e.getMessage()); + log.log(Level.WARNING, "Unable to parse request for deployment metrics", e); + return ErrorResponse.badRequest("Unable to parse request for deployment metrics - " + e.getMessage()); } return new StringResponse("Added deployment metrics"); } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainerTest.java index 5014f796933..fc43a7f9411 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainerTest.java @@ -72,13 +72,13 @@ public class DeploymentMetricsMaintainerTest { metricsService.setZoneIn(assignedRotation, "proxy.prod.us-west-1.vip.test"); metricsService.setZoneOut(assignedRotation,"proxy.prod.us-east-3.vip.test"); - wireMockRule.stubFor(post(urlEqualTo("/metricforwarding/v1/deploymentmetrics/")) + wireMockRule.stubFor(post(urlEqualTo("/metricforwarding/v1/deploymentmetrics")) .willReturn(aResponse().withStatus(200))); maintainer.maintain(); List<ServeEvent> allServeEvents = getAllServeEvents(); assertEquals(1, allServeEvents.size()); - LoggedRequest request = findAll(postRequestedFor(urlEqualTo("/metricforwarding/v1/deploymentmetrics/"))).get(0); + LoggedRequest request = findAll(postRequestedFor(urlEqualTo("/metricforwarding/v1/deploymentmetrics"))).get(0); Slime slime = SlimeUtils.jsonToSlime(request.getBody()); Inspector inspector = slime.get().entry(0); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/filter/ControllerAuthorizationFilterTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/filter/ControllerAuthorizationFilterTest.java index 19aa247edb4..6dce576e8a4 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/filter/ControllerAuthorizationFilterTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/filter/ControllerAuthorizationFilterTest.java @@ -58,6 +58,8 @@ public class ControllerAuthorizationFilterTest { ControllerAuthorizationFilter filter = createFilter(new ControllerTester()); assertIsAllowed(invokeFilter(filter, createRequest(PUT, "/application/v4/user", USER))); assertIsAllowed(invokeFilter(filter, createRequest(POST, "/application/v4/tenant/john", USER))); + assertIsAllowed(invokeFilter(filter, createRequest(POST, "/metricforwarding/v1/deploymentmetrics", USER))); + assertIsAllowed(invokeFilter(filter, createRequest(POST, "/contactinfo/v1/tenant/john/etc", USER))); } @Test diff --git a/document/src/vespa/document/fieldvalue/mapfieldvalue.cpp b/document/src/vespa/document/fieldvalue/mapfieldvalue.cpp index ebd51c82794..9c9c10c1a79 100644 --- a/document/src/vespa/document/fieldvalue/mapfieldvalue.cpp +++ b/document/src/vespa/document/fieldvalue/mapfieldvalue.cpp @@ -37,19 +37,26 @@ const MapDataType *verifyMapType(const DataType& type) { struct Hasher { Hasher(const MapFieldValue::IArray * keys) : _keys(keys) {} - uint32_t operator () (uint32_t index) const { return (*_keys)[index].hash(); } - const MapFieldValue::IArray * _keys; -}; - -struct Extract { - Extract(const MapFieldValue::IArray * keys) : _keys(keys) {} - const FieldValue & operator () (uint32_t index) const { return (*_keys)[index]; } + uint32_t operator () (uint32_t index) const { + return (*_keys)[index].hash(); + } + uint32_t operator () (const FieldValue & fv) const { + return fv.hash(); + } const MapFieldValue::IArray * _keys; }; struct Equal { Equal(const MapFieldValue::IArray * keys) : _keys(keys) {} - bool operator () (uint32_t a, uint32_t b) const { return (*_keys)[a].fastCompare((*_keys)[b]) == 0; } + bool operator () (uint32_t a, uint32_t b) const { + return (*_keys)[a].fastCompare((*_keys)[b]) == 0; + } + bool operator () (const FieldValue & a, uint32_t b) const { + return a.fastCompare((*_keys)[b]) == 0; + } + bool operator () (uint32_t a, const FieldValue & b) const { + return (*_keys)[a].fastCompare(b) == 0; + } const MapFieldValue::IArray * _keys; }; @@ -387,8 +394,7 @@ MapFieldValue::findIndex(const FieldValue& key) const { if ((size() > 0) && (key.getClass().id() == (*_keys)[0].getClass().id())) { ensureLookupMap(); - Extract extract(_keys.get()); - auto found = _lookupMap->find<FieldValue, Extract, vespalib::hash<FieldValue>, std::equal_to<FieldValue>>(key, extract); + auto found = _lookupMap->find(key); if (found != _lookupMap->end()) { uint32_t index = *found; assert(_present[index]); diff --git a/jaxrs_client_utils/pom.xml b/jaxrs_client_utils/pom.xml index e3de5b8163d..43fbc66a9e6 100644 --- a/jaxrs_client_utils/pom.xml +++ b/jaxrs_client_utils/pom.xml @@ -18,6 +18,12 @@ <dependencies> <dependency> <groupId>com.yahoo.vespa</groupId> + <artifactId>vespajlib</artifactId> + <version>${project.version}</version> + <scope>provided</scope> + </dependency> + <dependency> + <groupId>com.yahoo.vespa</groupId> <artifactId>application-model</artifactId> <version>${project.version}</version> <scope>provided</scope> diff --git a/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JaxRsClientFactory.java b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JaxRsClientFactory.java index d004ac3af45..27d7024b9bd 100644 --- a/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JaxRsClientFactory.java +++ b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JaxRsClientFactory.java @@ -3,11 +3,55 @@ package com.yahoo.vespa.jaxrs.client; import com.yahoo.vespa.applicationmodel.HostName; +import java.net.URI; +import java.time.Duration; + /** * Interface for creating a JAX-RS client API instance for a single server endpoint. * * @author bakksjo */ public interface JaxRsClientFactory { + class Params<T> { + private final Class<T> apiClass; + private final URI uri; + + private Duration connectTimeout = Duration.ofSeconds(30); + private Duration readTimeout = Duration.ofSeconds(30); + + public Params(Class<T> apiClass, URI uri) { + this.apiClass = apiClass; + this.uri = uri; + } + + public Class<T> apiClass() { + return apiClass; + } + + public URI uri() { + return uri; + } + + public void setConnectTimeout(Duration timeout) { + this.connectTimeout = timeout; + } + + public Duration connectTimeout() { + return connectTimeout; + } + + public void setReadTimeout(Duration timeout) { + readTimeout = timeout; + } + + public Duration readTimeout() { + return readTimeout; + } + } + + default <T> T createClient(Params<T> params) { + return createClient(params.apiClass, new HostName(params.uri.getHost()), params.uri.getPort(), params.uri.getPath(), params.uri.getScheme()); + } + <T> T createClient(Class<T> apiClass, HostName hostName, int port, String pathPrefix, String scheme); } diff --git a/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JaxRsStrategy.java b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JaxRsStrategy.java index 72af76fe54c..cd7d8684cbc 100644 --- a/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JaxRsStrategy.java +++ b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JaxRsStrategy.java @@ -11,4 +11,8 @@ import java.util.function.Function; */ public interface JaxRsStrategy<T> { <R> R apply(final Function<T, R> function) throws IOException; + + default <R> R apply(final Function<T, R> function, JaxRsTimeouts timeouts) throws IOException { + return apply(function); + } } diff --git a/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JaxRsTimeouts.java b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JaxRsTimeouts.java new file mode 100644 index 00000000000..914a9d7b42c --- /dev/null +++ b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JaxRsTimeouts.java @@ -0,0 +1,22 @@ +package com.yahoo.vespa.jaxrs.client; + +import java.time.Duration; + +/** + * @author hakonhall + */ +public interface JaxRsTimeouts { + /** + * The connect timeout, which must be at least 1ms. Called once per real REST call. + * + * Throws com.google.common.util.concurrent.UncheckedTimeoutException on timeout. + */ + Duration getConnectTimeoutOrThrow(); + + /** + * The read timeout, which must be at least 1ms. Called once per real REST call. + * + * Throws com.google.common.util.concurrent.UncheckedTimeoutException on timeout. + */ + Duration getReadTimeoutOrThrow(); +} diff --git a/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JerseyJaxRsClientFactory.java b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JerseyJaxRsClientFactory.java index 9321f8e290d..8aa880fb0e4 100644 --- a/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JerseyJaxRsClientFactory.java +++ b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/JerseyJaxRsClientFactory.java @@ -23,34 +23,20 @@ import java.util.Collections; */ public class JerseyJaxRsClientFactory implements JaxRsClientFactory { - private static final int DEFAULT_CONNECT_TIMEOUT_MS = 30000; - private static final int DEFAULT_READ_TIMEOUT_MS = 30000; - // Client is a heavy-weight object with a finalizer so we create only one and re-use it private final Client client; public JerseyJaxRsClientFactory() { - this(DEFAULT_CONNECT_TIMEOUT_MS, DEFAULT_READ_TIMEOUT_MS); + this(null, null, null); } public JerseyJaxRsClientFactory(SSLContext sslContext, HostnameVerifier hostnameVerifier, String userAgent) { - this(DEFAULT_CONNECT_TIMEOUT_MS, DEFAULT_READ_TIMEOUT_MS, sslContext, hostnameVerifier, userAgent); - } - - public JerseyJaxRsClientFactory(int connectTimeoutMs, int readTimeoutMs) { - this(connectTimeoutMs, readTimeoutMs, null, null, null); - } - - public JerseyJaxRsClientFactory(int connectTimeoutMs, int readTimeoutMs, SSLContext sslContext, - HostnameVerifier hostnameVerifier, String userAgent) { /* * Configure client with some workarounds for HTTP/JAX-RS/Jersey issues. See: * https://jersey.java.net/apidocs/latest/jersey/org/glassfish/jersey/client/ClientProperties.html#SUPPRESS_HTTP_COMPLIANCE_VALIDATION * https://jersey.java.net/apidocs/latest/jersey/org/glassfish/jersey/client/HttpUrlConnectorProvider.html#SET_METHOD_WORKAROUND */ ClientBuilder builder = ClientBuilder.newBuilder() - .property(ClientProperties.CONNECT_TIMEOUT, connectTimeoutMs) - .property(ClientProperties.READ_TIMEOUT, readTimeoutMs) .property(ClientProperties.SUPPRESS_HTTP_COMPLIANCE_VALIDATION, true) // Allow empty PUT. TODO: Fix API. .property(HttpUrlConnectorProvider.SET_METHOD_WORKAROUND, true) // Allow e.g. PATCH method. .property(ClientProperties.FOLLOW_REDIRECTS, true); @@ -67,10 +53,16 @@ public class JerseyJaxRsClientFactory implements JaxRsClientFactory { } @Override + public <T> T createClient(Params<T> params) { + WebTarget target = client.target(params.uri()); + target.property(ClientProperties.CONNECT_TIMEOUT, (int) params.connectTimeout().toMillis()); + target.property(ClientProperties.READ_TIMEOUT, (int) params.readTimeout().toMillis()); + return WebResourceFactory.newResource(params.apiClass(), target); + } + + @Override public <T> T createClient(Class<T> apiClass, HostName hostName, int port, String pathPrefix, String scheme) { UriBuilder uriBuilder = UriBuilder.fromPath(pathPrefix).host(hostName.s()).port(port).scheme(scheme); - WebTarget target = client.target(uriBuilder); - return WebResourceFactory.newResource(apiClass, target); + return createClient(new Params<>(apiClass, uriBuilder.build())); } - } diff --git a/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/LegacyJaxRsTimeouts.java b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/LegacyJaxRsTimeouts.java new file mode 100644 index 00000000000..3f2139f6bf0 --- /dev/null +++ b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/LegacyJaxRsTimeouts.java @@ -0,0 +1,26 @@ +package com.yahoo.vespa.jaxrs.client; + +import java.time.Duration; + +/** + * Legacy defaults for timeouts. + * + * Clients should instead define their own JaxRsTimeouts tailored to their use-case. + * + * @author hakonhall + */ +// Immutable +public class LegacyJaxRsTimeouts implements JaxRsTimeouts { + private static final Duration CONNECT_TIMEOUT = Duration.ofSeconds(30); + private static final Duration READ_TIMEOUT = Duration.ofSeconds(30); + + @Override + public Duration getConnectTimeoutOrThrow() { + return CONNECT_TIMEOUT; + } + + @Override + public Duration getReadTimeoutOrThrow() { + return READ_TIMEOUT; + } +} diff --git a/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/RetryingJaxRsStrategy.java b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/RetryingJaxRsStrategy.java index 65b302ef4ff..c964dfce2c7 100644 --- a/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/RetryingJaxRsStrategy.java +++ b/jaxrs_client_utils/src/main/java/com/yahoo/vespa/jaxrs/client/RetryingJaxRsStrategy.java @@ -4,7 +4,9 @@ package com.yahoo.vespa.jaxrs.client; import com.yahoo.vespa.applicationmodel.HostName; import javax.ws.rs.ProcessingException; +import javax.ws.rs.core.UriBuilder; import java.io.IOException; +import java.net.URI; import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -65,14 +67,25 @@ public class RetryingJaxRsStrategy<T> implements JaxRsStrategy<T> { @Override public <R> R apply(final Function<T, R> function) throws IOException { + return apply(function, new LegacyJaxRsTimeouts()); + } + + + @Override + public <R> R apply(final Function<T, R> function, JaxRsTimeouts timeouts) throws IOException { ProcessingException sampleException = null; for (int i = 0; i < maxIterations; ++i) { for (final HostName hostName : hostNames) { - final T jaxRsClient = jaxRsClientFactory.createClient(apiClass, hostName, port, pathPrefix, scheme); + URI uri = UriBuilder.fromPath(pathPrefix).port(port).scheme(scheme).host(hostName.s()).build(); + JaxRsClientFactory.Params<T> params = new JaxRsClientFactory.Params<>(apiClass, uri); + params.setConnectTimeout(timeouts.getConnectTimeoutOrThrow()); + params.setReadTimeout(timeouts.getReadTimeoutOrThrow()); + final T jaxRsClient = jaxRsClientFactory.createClient(params); try { return function.apply(jaxRsClient); } catch (ProcessingException e) { + // E.g. java.net.SocketTimeoutException thrown on read timeout is wrapped as a ProcessingException sampleException = e; logger.log(Level.INFO, "Failed REST API call to " + hostName + ":" + port + pathPrefix + " (in retry loop): " diff --git a/jaxrs_client_utils/src/test/java/com/yahoo/vespa/jaxrs/client/HttpPatchTest.java b/jaxrs_client_utils/src/test/java/com/yahoo/vespa/jaxrs/client/HttpPatchTest.java index 63e2b814c24..8161602cdac 100644 --- a/jaxrs_client_utils/src/test/java/com/yahoo/vespa/jaxrs/client/HttpPatchTest.java +++ b/jaxrs_client_utils/src/test/java/com/yahoo/vespa/jaxrs/client/HttpPatchTest.java @@ -74,8 +74,7 @@ public class HttpPatchTest extends JerseyTest { final JaxRsStrategy<TestResourceApi> client = factory.apiNoRetries(TestResourceApi.class, apiPath); final String responseBody; - responseBody = client.apply(api -> - api.doPatch(REQUEST_BODY)); + responseBody = client.apply(api -> api.doPatch(REQUEST_BODY)); assertThat(testResourceSingleton.invocation.get(60, TimeUnit.SECONDS), is(REQUEST_BODY)); assertThat(responseBody, is(REQUEST_BODY)); diff --git a/jaxrs_client_utils/src/test/java/com/yahoo/vespa/jaxrs/client/RetryingJaxRsStrategyTest.java b/jaxrs_client_utils/src/test/java/com/yahoo/vespa/jaxrs/client/RetryingJaxRsStrategyTest.java index e31920febd6..dbe886b7896 100644 --- a/jaxrs_client_utils/src/test/java/com/yahoo/vespa/jaxrs/client/RetryingJaxRsStrategyTest.java +++ b/jaxrs_client_utils/src/test/java/com/yahoo/vespa/jaxrs/client/RetryingJaxRsStrategyTest.java @@ -5,7 +5,10 @@ import com.yahoo.vespa.applicationmodel.HostName; import com.yahoo.vespa.defaults.Defaults; import org.junit.Before; import org.junit.Test; +import org.junit.runner.RunWith; import org.mockito.ArgumentCaptor; +import org.mockito.Captor; +import org.mockito.runners.MockitoJUnitRunner; import org.mockito.stubbing.OngoingStubbing; import javax.ws.rs.GET; @@ -15,24 +18,26 @@ import java.io.IOException; import java.util.Arrays; import java.util.HashSet; import java.util.Set; +import java.util.stream.Collectors; -import static org.hamcrest.CoreMatchers.equalTo; -import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.CoreMatchers.hasItem; import static org.hamcrest.MatcherAssert.assertThat; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.fail; import static org.mockito.Matchers.any; -import static org.mockito.Matchers.anyInt; -import static org.mockito.Matchers.anyString; -import static org.mockito.Matchers.eq; import static org.mockito.Mockito.atLeast; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +@RunWith(MockitoJUnitRunner.class) public class RetryingJaxRsStrategyTest { private static final String API_PATH = "/"; + @Captor + ArgumentCaptor<JaxRsClientFactory.Params<TestJaxRsApi>> paramsCaptor; + @Path(API_PATH) private interface TestJaxRsApi { @GET @@ -53,8 +58,7 @@ public class RetryingJaxRsStrategyTest { @Before public void setup() { - when(jaxRsClientFactory.createClient(eq(TestJaxRsApi.class), any(HostName.class), anyInt(), anyString(), anyString())) - .thenReturn(mockApi); + when(jaxRsClientFactory.createClient(any())).thenReturn(mockApi); } @Test @@ -63,11 +67,12 @@ public class RetryingJaxRsStrategyTest { verify(mockApi, times(1)).doSomething(); - // Check that one of the supplied hosts is contacted. - final ArgumentCaptor<HostName> hostNameCaptor = ArgumentCaptor.forClass(HostName.class); - verify(jaxRsClientFactory, times(1)) - .createClient(eq(TestJaxRsApi.class), hostNameCaptor.capture(), eq(REST_PORT), eq(API_PATH), eq("http")); - assertThat(SERVER_HOSTS.contains(hostNameCaptor.getValue()), is(true)); + verify(jaxRsClientFactory, times(1)).createClient(paramsCaptor.capture()); + JaxRsClientFactory.Params<TestJaxRsApi> params = paramsCaptor.getValue(); + assertEquals(REST_PORT, params.uri().getPort()); + assertEquals(API_PATH, params.uri().getPath()); + assertEquals("http", params.uri().getScheme()); + assertThat(SERVER_HOSTS, hasItem(new HostName(params.uri().getHost()))); } @Test @@ -99,10 +104,10 @@ public class RetryingJaxRsStrategyTest { @Test public void testRetryLoopsOverAvailableServers() throws Exception { when(mockApi.doSomething()) - .thenThrow(new ProcessingException("Fake timeout 1 induced by test")) - .thenThrow(new ProcessingException("Fake timeout 2 induced by test")) - .thenThrow(new ProcessingException("Fake timeout 3 induced by test")) - .thenThrow(new ProcessingException("Fake timeout 4 induced by test")) + .thenThrow(new ProcessingException("Fake socket timeout 1 induced by test")) + .thenThrow(new ProcessingException("Fake socket timeout 2 induced by test")) + .thenThrow(new ProcessingException("Fake socket timeout 3 induced by test")) + .thenThrow(new ProcessingException("Fake socket timeout 4 induced by test")) .thenReturn("a response"); jaxRsStrategy.apply(TestJaxRsApi::doSomething); @@ -142,12 +147,9 @@ public class RetryingJaxRsStrategyTest { verifyAllServersContacted(jaxRsClientFactory); } - private static void verifyAllServersContacted( - final JaxRsClientFactory jaxRsClientFactory) { - final ArgumentCaptor<HostName> hostNameCaptor = ArgumentCaptor.forClass(HostName.class); - verify(jaxRsClientFactory, atLeast(SERVER_HOSTS.size())) - .createClient(eq(TestJaxRsApi.class), hostNameCaptor.capture(), eq(REST_PORT), eq(API_PATH), eq("http")); - final Set<HostName> actualServerHostsContacted = new HashSet<>(hostNameCaptor.getAllValues()); - assertThat(actualServerHostsContacted, equalTo(SERVER_HOSTS)); + private void verifyAllServersContacted(final JaxRsClientFactory jaxRsClientFactory) { + verify(jaxRsClientFactory, atLeast(SERVER_HOSTS.size())).createClient(paramsCaptor.capture()); + final Set<JaxRsClientFactory.Params<TestJaxRsApi>> actualServerHostsContacted = new HashSet<>(paramsCaptor.getAllValues()); + assertEquals(actualServerHostsContacted.stream().map(x -> new HostName(x.uri().getHost())).collect(Collectors.toSet()), SERVER_HOSTS); } } diff --git a/juniper/src/vespa/juniper/stringmap.cpp b/juniper/src/vespa/juniper/stringmap.cpp index 13d7bdee980..f673f22e29c 100644 --- a/juniper/src/vespa/juniper/stringmap.cpp +++ b/juniper/src/vespa/juniper/stringmap.cpp @@ -1,6 +1,7 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. #include "stringmap.h" +#include <vespa/vespalib/stllike/hashtable.hpp> void Fast_StringMap::Insert(const char* key, const char* value) { @@ -9,7 +10,7 @@ void Fast_StringMap::Insert(const char* key, const char* value) const char * -Fast_StringMap::Lookup(const char *key, const char *defval) +Fast_StringMap::Lookup(const char *key, const char *defval) const { Map::const_iterator found(_backing.find(key)); return (found != _backing.end()) ? found->second.c_str() : defval; diff --git a/juniper/src/vespa/juniper/stringmap.h b/juniper/src/vespa/juniper/stringmap.h index c202ecb5888..068eeaf3d16 100644 --- a/juniper/src/vespa/juniper/stringmap.h +++ b/juniper/src/vespa/juniper/stringmap.h @@ -10,10 +10,10 @@ class Fast_StringMap { private: - typedef vespalib::hash_map<vespalib::string, vespalib::string> Map; + using Map = vespalib::hash_map<vespalib::string, vespalib::string>; Map _backing; public: void Insert(const char* key, const char* value); - const char *Lookup(const char* key, const char* defval); + const char *Lookup(const char* key, const char* defval) const; }; diff --git a/metrics/src/vespa/metrics/CMakeLists.txt b/metrics/src/vespa/metrics/CMakeLists.txt index 147e88cd61c..96156dc84b0 100644 --- a/metrics/src/vespa/metrics/CMakeLists.txt +++ b/metrics/src/vespa/metrics/CMakeLists.txt @@ -12,7 +12,6 @@ vespa_add_library(metrics metricsnapshot.cpp metrictimer.cpp metricvalueset.cpp - namehash.cpp printutils.cpp name_repo.cpp state_api_adapter.cpp diff --git a/metrics/src/vespa/metrics/loadmetric.hpp b/metrics/src/vespa/metrics/loadmetric.hpp index ce93c761a05..65098662e04 100644 --- a/metrics/src/vespa/metrics/loadmetric.hpp +++ b/metrics/src/vespa/metrics/loadmetric.hpp @@ -55,7 +55,7 @@ LoadMetric<MetricType>::LoadMetric(const LoadMetric<MetricType>& other, MetricSe } template<typename MetricType> -LoadMetric<MetricType>::~LoadMetric() { } +LoadMetric<MetricType>::~LoadMetric() = default; template<typename MetricType> MetricSet* @@ -74,10 +74,9 @@ MetricType& LoadMetric<MetricType>::getMetric(const LoadType& type) { MetricType* metric; - typename vespalib::hash_map<uint32_t, MetricTypeUP>::iterator it( - _metrics.find(type.getId())); + auto it = _metrics.find(type.getId()); if (it == _metrics.end()) { - it = _metrics.find(0); + it = _metrics.find(0u); assert(it != _metrics.end()); // Default should always exist } metric = it->second.get(); diff --git a/metrics/src/vespa/metrics/memoryconsumption.cpp b/metrics/src/vespa/metrics/memoryconsumption.cpp index 0e69defa558..0391e496ecd 100644 --- a/metrics/src/vespa/metrics/memoryconsumption.cpp +++ b/metrics/src/vespa/metrics/memoryconsumption.cpp @@ -1,11 +1,11 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. #include "memoryconsumption.h" -#include <vespa/vespalib/stllike/hash_set.h> +#include <vespa/vespalib/stllike/hash_set.hpp> #include <sstream> namespace metrics { -struct SeenStrings : public vespalib::hash_set<const void*> { }; +struct SeenStrings : public vespalib::hash_set<const char*> { }; struct SnapShotUsage : public std::vector<std::pair<std::string, uint32_t> > { }; MemoryConsumption::MemoryConsumption() @@ -16,7 +16,7 @@ MemoryConsumption::MemoryConsumption() _seenStrings->resize(1000); } -MemoryConsumption::~MemoryConsumption() { } +MemoryConsumption::~MemoryConsumption() = default; uint32_t MemoryConsumption::getStringMemoryUsage(const std::string& s, uint32_t& uniqueCount) { diff --git a/metrics/src/vespa/metrics/metricmanager.cpp b/metrics/src/vespa/metrics/metricmanager.cpp index 5b716e2698f..7125446c168 100644 --- a/metrics/src/vespa/metrics/metricmanager.cpp +++ b/metrics/src/vespa/metrics/metricmanager.cpp @@ -13,6 +13,7 @@ #include <vespa/vespalib/util/stringfmt.h> #include <vespa/vespalib/util/exceptions.h> #include <vespa/vespalib/stllike/asciistream.h> +#include <vespa/vespalib/stllike/hashtable.hpp> #include <sstream> #include <algorithm> @@ -23,8 +24,8 @@ namespace metrics { typedef MetricsmanagerConfig Config; -MetricManager::ConsumerSpec::ConsumerSpec() : includedMetrics() { } -MetricManager::ConsumerSpec::~ConsumerSpec() { } +MetricManager::ConsumerSpec::ConsumerSpec() = default; +MetricManager::ConsumerSpec::~ConsumerSpec() = default; void MetricManager::assertMetricLockLocked(const MetricLockGuard& g) const { diff --git a/metrics/src/vespa/metrics/namehash.cpp b/metrics/src/vespa/metrics/namehash.cpp deleted file mode 100644 index bd0b3a05697..00000000000 --- a/metrics/src/vespa/metrics/namehash.cpp +++ /dev/null @@ -1,43 +0,0 @@ -// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. - -#include "namehash.h" -#include "memoryconsumption.h" -#include <vespa/vespalib/stllike/hash_set.h> - -namespace metrics { - -struct NameSet : public vespalib::hash_set<std::string> { }; - -NameHash::NameHash() - : _hash(std::make_unique<NameSet>()), - _unifiedCounter(0), - _checkedCounter(0) -{ } - -NameHash::~NameHash() { } - -void -NameHash::updateName(std::string& name) { - ++_checkedCounter; - NameSet::const_iterator it(_hash->find(name)); - if (it != _hash->end()) { - if (name.c_str() != it->c_str()) { - name = *it; - ++_unifiedCounter; - } - } else { - _hash->insert(name); - } -} - -void -NameHash::addMemoryUsage(MemoryConsumption& mc) const { - mc._nameHash += sizeof(NameHash) - + _hash->getMemoryConsumption() - - sizeof(NameSet); - for (const std::string & name : *_hash) { - mc._nameHashStrings += mc.getStringMemoryUsage(name, mc._nameHashUnique); - } -} - -} // metrics diff --git a/metrics/src/vespa/metrics/namehash.h b/metrics/src/vespa/metrics/namehash.h deleted file mode 100644 index 94b4c984f6b..00000000000 --- a/metrics/src/vespa/metrics/namehash.h +++ /dev/null @@ -1,48 +0,0 @@ -// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -/** - * \class metrics::NameHash - * \ingroup metrics - * - * \brief Simple class to enable string reference counting to work better. - * - * When creating metrics, it is easy to use const char references from code, - * for instance having a for loop setting up metrics for each thread, this will - * not actually generate ref counted strings, but rather unique strings. - * - * Also, with ref counted strings, it is easy to screw it up if you access the - * string in a way requiring copy. - * - * This class is used to just keep a set of strings, and having a class for - * users to input their strings and get the "master" string with that content. - * - * Metrics use this after having registered metrics, to ensure we dont keep more - * copies of non-unique strings than needed. - */ -#pragma once - -#include "memoryconsumption.h" - -namespace metrics { - -class NameSet; - -class NameHash { - std::unique_ptr<NameSet> _hash; - uint32_t _unifiedCounter; - uint32_t _checkedCounter; - -public: - NameHash(const NameHash &) = delete; - NameHash & operator = (const NameHash &) = delete; - NameHash(); - ~NameHash(); - - void updateName(std::string& name); - - uint32_t getUnifiedStringCount() const { return _unifiedCounter; } - uint32_t getCheckedStringCount() const { return _checkedCounter; } - void resetCounts() { _unifiedCounter = 0; _checkedCounter = 0; } - void addMemoryUsage(MemoryConsumption& mc) const; -}; - -} // metrics diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/OrchestratorMock.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/OrchestratorMock.java index c71b0783d69..70750dd6672 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/OrchestratorMock.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/OrchestratorMock.java @@ -5,7 +5,6 @@ import com.yahoo.config.provision.ApplicationId; import com.yahoo.vespa.applicationmodel.HostName; import com.yahoo.vespa.orchestrator.Host; import com.yahoo.vespa.orchestrator.Orchestrator; -import com.yahoo.vespa.orchestrator.model.NodeGroup; import com.yahoo.vespa.orchestrator.status.ApplicationInstanceStatus; import com.yahoo.vespa.orchestrator.status.HostStatus; @@ -46,11 +45,6 @@ public class OrchestratorMock implements Orchestrator { } @Override - public void suspendGroup(NodeGroup nodeGroup) { - nodeGroup.getHostNames().forEach(this::suspend); - } - - @Override public ApplicationInstanceStatus getApplicationInstanceStatus(ApplicationId appId) { return suspendedApplications.contains(appId) ? ApplicationInstanceStatus.ALLOWED_TO_BE_DOWN : ApplicationInstanceStatus.NO_REMARKS; diff --git a/orchestrator-restapi/src/main/java/com/yahoo/vespa/orchestrator/restapi/HostSuspensionApi.java b/orchestrator-restapi/src/main/java/com/yahoo/vespa/orchestrator/restapi/HostSuspensionApi.java index 603d6a1adac..9535096af4f 100644 --- a/orchestrator-restapi/src/main/java/com/yahoo/vespa/orchestrator/restapi/HostSuspensionApi.java +++ b/orchestrator-restapi/src/main/java/com/yahoo/vespa/orchestrator/restapi/HostSuspensionApi.java @@ -22,13 +22,9 @@ public interface HostSuspensionApi { String PATH_PREFIX = "/v1/suspensions/hosts"; /** - * Ask for permission to temporarily suspend all services on a set of hosts. + * Ask for permission to temporarily suspend all services on a set of hosts (nodes). * - * See HostApi::suspend for semantics of suspending a host. - * - * On failure, it tries to resume ALL hosts. It needs to try to resume all hosts because any or all hosts - * may have been suspended in an earlier attempt. Ending with resumption of all hosts makes sure other - * batch-requests for suspension of hosts succeed. + * See HostApi::suspend for semantics of suspending a node. */ @PUT @Path("/{hostname}") diff --git a/orchestrator/pom.xml b/orchestrator/pom.xml index ae05a1908c9..37d308111ae 100644 --- a/orchestrator/pom.xml +++ b/orchestrator/pom.xml @@ -117,6 +117,12 @@ <scope>provided</scope> </dependency> <dependency> + <groupId>com.yahoo.vespa</groupId> + <artifactId>testutil</artifactId> + <version>${project.version}</version> + <scope>test</scope> + </dependency> + <dependency> <groupId>org.apache.curator</groupId> <artifactId>curator-test</artifactId> <scope>test</scope> diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/Orchestrator.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/Orchestrator.java index b2be4fe52ec..df124f2f690 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/Orchestrator.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/Orchestrator.java @@ -75,15 +75,6 @@ public interface Orchestrator { void acquirePermissionToRemove(HostName hostName) throws OrchestrationException; /** - * Suspend normal operations for a group of nodes in the same application. - * - * @param nodeGroup The group of nodes in an application. - * @throws HostStateChangeDeniedException if the request cannot be meet due to policy constraints. - * @throws HostNameNotFoundException if any hostnames in the node group is not recognized - */ - void suspendGroup(NodeGroup nodeGroup) throws HostStateChangeDeniedException, HostNameNotFoundException; - - /** * Suspend several hosts. On failure, all hosts are resumed before exiting the method with an exception. */ void suspendAll(HostName parentHostname, List<HostName> hostNames) diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorContext.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorContext.java index 6577b4b96cc..f1f572621ce 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorContext.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorContext.java @@ -1,40 +1,85 @@ // Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.orchestrator; -import com.google.common.util.concurrent.UncheckedTimeoutException; import com.yahoo.time.TimeBudget; +import com.yahoo.vespa.orchestrator.controller.ClusterControllerClientTimeouts; import java.time.Clock; import java.time.Duration; +import java.time.Instant; +import java.util.Optional; /** - * Context for the Orchestrator, e.g. timeout management. + * Context for an operation (or suboperation) of the Orchestrator that needs to pass through to the backend, + * e.g. timeout management and probing. * - * @author hakon + * @author hakonhall */ public class OrchestratorContext { - private static final Duration DEFAULT_TIMEOUT = Duration.ofSeconds(10); + private static final Duration DEFAULT_TIMEOUT_FOR_SINGLE_OP = Duration.ofSeconds(10); + private static final Duration DEFAULT_TIMEOUT_FOR_BATCH_OP = Duration.ofSeconds(60); + private static final Duration TIMEOUT_OVERHEAD = Duration.ofMillis(500); - private TimeBudget timeBudget; + private final Clock clock; + private final TimeBudget timeBudget; + private boolean probe; - public OrchestratorContext(Clock clock) { - this.timeBudget = TimeBudget.fromNow(clock, DEFAULT_TIMEOUT); + /** Create an OrchestratorContext for operations on multiple applications. */ + public static OrchestratorContext createContextForMultiAppOp(Clock clock) { + return new OrchestratorContext(clock, TimeBudget.fromNow(clock, DEFAULT_TIMEOUT_FOR_BATCH_OP), true); } - /** Get the original timeout in seconds. */ - public long getOriginalTimeoutInSeconds() { - return timeBudget.originalTimeout().get().getSeconds(); + /** Create an OrchestratorContext for an operation on a single application. */ + public static OrchestratorContext createContextForSingleAppOp(Clock clock) { + return new OrchestratorContext(clock, TimeBudget.fromNow(clock, DEFAULT_TIMEOUT_FOR_SINGLE_OP), true); } - /** - * Get timeout for a suboperation that should take up {@code shareOfRemaining} of the - * remaining time, or throw an {@link UncheckedTimeoutException} if timed out. - */ - public float getSuboperationTimeoutInSeconds(float shareOfRemaining) { - if (!(0f <= shareOfRemaining && shareOfRemaining <= 1.0f)) { - throw new IllegalArgumentException("Share of remaining time must be between 0 and 1: " + shareOfRemaining); + private OrchestratorContext(Clock clock, TimeBudget timeBudget, boolean probe) { + this.clock = clock; + this.timeBudget = timeBudget; + this.probe = probe; + } + + public Duration getTimeLeft() { + return timeBudget.timeLeftOrThrow().get(); + } + + public ClusterControllerClientTimeouts getClusterControllerTimeouts() { + return new ClusterControllerClientTimeouts(timeBudget.timeLeftAsTimeBudget()); + } + + + /** Mark this operation as a non-committal probe. */ + public OrchestratorContext markAsProbe() { + this.probe = true; + return this; + } + + /** Whether the operation is a no-op probe to test whether it would have succeeded, if it had been committal. */ + public boolean isProbe() { + return probe; + } + + /** Create OrchestratorContext to use within an application lock. */ + public OrchestratorContext createSubcontextWithinLock() { + // Move deadline towards past by a fixed amount to ensure there's time to process exceptions and + // access ZooKeeper before the lock times out. + TimeBudget subTimeBudget = timeBudget.withDeadline(timeBudget.deadline().get().minus(TIMEOUT_OVERHEAD)); + return new OrchestratorContext(clock, subTimeBudget, probe); + } + + /** Create an OrchestratorContext for an operation on a single application, but limited to current timeout. */ + public OrchestratorContext createSubcontextForSingleAppOp() { + Instant now = clock.instant(); + Instant deadline = timeBudget.deadline().get(); + Instant maxDeadline = now.plus(DEFAULT_TIMEOUT_FOR_SINGLE_OP); + if (maxDeadline.compareTo(deadline) < 0) { + deadline = maxDeadline; } - return shareOfRemaining * timeBudget.timeLeftOrThrow().get().toMillis() / 1000.0f; + return new OrchestratorContext( + clock, + TimeBudget.from(clock, now, Optional.of(Duration.between(now, deadline))), + probe); } } diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorImpl.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorImpl.java index ad8a35312e4..6811788ffb7 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorImpl.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorImpl.java @@ -105,7 +105,9 @@ public class OrchestratorImpl implements Orchestrator { @Override public void setNodeStatus(HostName hostName, HostStatus status) throws OrchestrationException { ApplicationInstanceReference reference = getApplicationInstance(hostName).reference(); - try (MutableStatusRegistry statusRegistry = statusService.lockApplicationInstance_forCurrentThreadOnly(reference)) { + OrchestratorContext context = OrchestratorContext.createContextForSingleAppOp(clock); + try (MutableStatusRegistry statusRegistry = statusService + .lockApplicationInstance_forCurrentThreadOnly(reference, context.getTimeLeft())) { statusRegistry.setHostState(hostName, status); } } @@ -127,10 +129,10 @@ public class OrchestratorImpl implements Orchestrator { ApplicationInstance appInstance = getApplicationInstance(hostName); - OrchestratorContext context = new OrchestratorContext(clock); + OrchestratorContext context = OrchestratorContext.createContextForSingleAppOp(clock); try (MutableStatusRegistry statusRegistry = statusService.lockApplicationInstance_forCurrentThreadOnly( appInstance.reference(), - context.getOriginalTimeoutInSeconds())) { + context.getTimeLeft())) { final HostStatus currentHostState = statusRegistry.getHostStatus(hostName); if (HostStatus.NO_REMARKS == currentHostState) { @@ -139,7 +141,7 @@ public class OrchestratorImpl implements Orchestrator { ApplicationInstanceStatus appStatus = statusService.forApplicationInstance(appInstance.reference()).getApplicationInstanceStatus(); if (appStatus == ApplicationInstanceStatus.NO_REMARKS) { - policy.releaseSuspensionGrant(context, appInstance, hostName, statusRegistry); + policy.releaseSuspensionGrant(context.createSubcontextWithinLock(), appInstance, hostName, statusRegistry); } } } @@ -148,7 +150,7 @@ public class OrchestratorImpl implements Orchestrator { public void suspend(HostName hostName) throws HostStateChangeDeniedException, HostNameNotFoundException { ApplicationInstance appInstance = getApplicationInstance(hostName); NodeGroup nodeGroup = new NodeGroup(appInstance, hostName); - suspendGroup(nodeGroup); + suspendGroup(OrchestratorContext.createContextForSingleAppOp(clock), nodeGroup); } @Override @@ -156,29 +158,32 @@ public class OrchestratorImpl implements Orchestrator { ApplicationInstance appInstance = getApplicationInstance(hostName); NodeGroup nodeGroup = new NodeGroup(appInstance, hostName); - OrchestratorContext context = new OrchestratorContext(clock); + OrchestratorContext context = OrchestratorContext.createContextForSingleAppOp(clock); try (MutableStatusRegistry statusRegistry = statusService.lockApplicationInstance_forCurrentThreadOnly( appInstance.reference(), - context.getOriginalTimeoutInSeconds())) { + context.getTimeLeft())) { ApplicationApi applicationApi = new ApplicationApiImpl( nodeGroup, statusRegistry, clusterControllerClientFactory); - policy.acquirePermissionToRemove(context, applicationApi); + policy.acquirePermissionToRemove(context.createSubcontextWithinLock(), applicationApi); } } - // Public for testing purposes - @Override - public void suspendGroup(NodeGroup nodeGroup) throws HostStateChangeDeniedException, HostNameNotFoundException { + /** + * Suspend normal operations for a group of nodes in the same application. + * + * @param nodeGroup The group of nodes in an application. + * @throws HostStateChangeDeniedException if the request cannot be meet due to policy constraints. + */ + void suspendGroup(OrchestratorContext context, NodeGroup nodeGroup) throws HostStateChangeDeniedException { ApplicationInstanceReference applicationReference = nodeGroup.getApplicationReference(); - OrchestratorContext context = new OrchestratorContext(clock); try (MutableStatusRegistry hostStatusRegistry = statusService.lockApplicationInstance_forCurrentThreadOnly( applicationReference, - context.getOriginalTimeoutInSeconds())) { + context.getTimeLeft())) { ApplicationInstanceStatus appStatus = statusService.forApplicationInstance(applicationReference).getApplicationInstanceStatus(); if (appStatus == ApplicationInstanceStatus.ALLOWED_TO_BE_DOWN) { return; @@ -188,7 +193,7 @@ public class OrchestratorImpl implements Orchestrator { nodeGroup, hostStatusRegistry, clusterControllerClientFactory); - policy.grantSuspensionRequest(context, applicationApi); + policy.grantSuspensionRequest(context.createSubcontextWithinLock(), applicationApi); } } @@ -224,14 +229,12 @@ public class OrchestratorImpl implements Orchestrator { throw new BatchHostNameNotFoundException(parentHostname, hostNames, e); } + OrchestratorContext context = OrchestratorContext.createContextForMultiAppOp(clock); for (NodeGroup nodeGroup : nodeGroupsOrderedByApplication) { try { - suspendGroup(nodeGroup); + suspendGroup(context.createSubcontextForSingleAppOp(), nodeGroup); } catch (HostStateChangeDeniedException e) { throw new BatchHostStateChangeDeniedException(parentHostname, nodeGroup, e); - } catch (HostNameNotFoundException e) { - // Should never get here since since we would have received HostNameNotFoundException earlier. - throw new BatchHostNameNotFoundException(parentHostname, hostNames, e); } catch (RuntimeException e) { throw new BatchInternalErrorException(parentHostname, nodeGroup, e); } @@ -301,12 +304,12 @@ public class OrchestratorImpl implements Orchestrator { private void setApplicationStatus(ApplicationId appId, ApplicationInstanceStatus status) throws ApplicationStateChangeDeniedException, ApplicationIdNotFoundException{ - OrchestratorContext context = new OrchestratorContext(clock); + OrchestratorContext context = OrchestratorContext.createContextForSingleAppOp(clock); ApplicationInstanceReference appRef = OrchestratorUtil.toApplicationInstanceReference(appId, instanceLookupService); try (MutableStatusRegistry statusRegistry = statusService.lockApplicationInstance_forCurrentThreadOnly( appRef, - context.getOriginalTimeoutInSeconds())) { + context.getTimeLeft())) { // Short-circuit if already in wanted state if (status == statusRegistry.getApplicationInstanceStatus()) return; @@ -321,7 +324,7 @@ public class OrchestratorImpl implements Orchestrator { // If the clustercontroller throws an error the nodes will be marked as allowed to be down // and be set back up on next resume invocation. - setClusterStateInController(context, application, ClusterControllerNodeState.MAINTENANCE); + setClusterStateInController(context.createSubcontextWithinLock(), application, ClusterControllerNodeState.MAINTENANCE); } statusRegistry.setApplicationInstanceStatus(status); diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientImpl.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientImpl.java index 467b534f809..50ffe13b437 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientImpl.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientImpl.java @@ -1,6 +1,7 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.orchestrator.controller; +import com.google.common.util.concurrent.UncheckedTimeoutException; import com.yahoo.vespa.jaxrs.client.JaxRsStrategy; import com.yahoo.vespa.orchestrator.OrchestratorContext; @@ -15,23 +16,6 @@ public class ClusterControllerClientImpl implements ClusterControllerClient{ public static final String REQUEST_REASON = "Orchestrator"; - // On setNodeState calls against the CC ensemble. - // - // We'd like to set a timeout for the request to the first CC such that if the first - // CC is faulty, there's sufficient time to send the request to the second and third CC. - // The timeouts would be: - // timeout(1. request) = SHARE_REMAINING_TIME * T - // timeout(2. request) = SHARE_REMAINING_TIME * T * (1 - SHARE_REMAINING_TIME) - // timeout(3. request) = SHARE_REMAINING_TIME * T * (1 - SHARE_REMAINING_TIME)^2 - // - // Using a share of 50% gives approximately: - // timeout(1. request) = T * 0.5 - // timeout(2. request) = T * 0.25 - // timeout(3. request) = T * 0.125 - // - // which seems fine - public static final float SHARE_REMAINING_TIME = 0.5f; - private final JaxRsStrategy<ClusterControllerJaxRsApi> clusterControllerApi; private final String clusterName; @@ -52,20 +36,22 @@ public class ClusterControllerClientImpl implements ClusterControllerClient{ ClusterControllerNodeState wantedState) throws IOException { ClusterControllerStateRequest.State state = new ClusterControllerStateRequest.State(wantedState, REQUEST_REASON); ClusterControllerStateRequest stateRequest = new ClusterControllerStateRequest(state, ClusterControllerStateRequest.Condition.SAFE); + ClusterControllerClientTimeouts timeouts = context.getClusterControllerTimeouts(); try { return clusterControllerApi.apply(api -> api.setNodeState( clusterName, storageNodeIndex, - context.getSuboperationTimeoutInSeconds(SHARE_REMAINING_TIME), - stateRequest) - ); - } catch (IOException e) { + timeouts.getServerTimeoutOrThrow().toMillis() / 1000.0f, + stateRequest), + timeouts); + } catch (IOException | UncheckedTimeoutException e) { String message = String.format( - "Giving up setting %s for storage node with index %d in cluster %s", + "Giving up setting %s for storage node with index %d in cluster %s: %s", stateRequest, storageNodeIndex, - clusterName); + clusterName, + e.getMessage()); throw new IOException(message, e); } @@ -79,16 +65,18 @@ public class ClusterControllerClientImpl implements ClusterControllerClient{ @Override public ClusterControllerStateResponse setApplicationState( OrchestratorContext context, - final ClusterControllerNodeState wantedState) throws IOException { - final ClusterControllerStateRequest.State state = new ClusterControllerStateRequest.State(wantedState, REQUEST_REASON); - final ClusterControllerStateRequest stateRequest = new ClusterControllerStateRequest(state, ClusterControllerStateRequest.Condition.FORCE); + ClusterControllerNodeState wantedState) throws IOException { + ClusterControllerStateRequest.State state = new ClusterControllerStateRequest.State(wantedState, REQUEST_REASON); + ClusterControllerStateRequest stateRequest = new ClusterControllerStateRequest(state, ClusterControllerStateRequest.Condition.FORCE); + ClusterControllerClientTimeouts timeouts = context.getClusterControllerTimeouts(); try { return clusterControllerApi.apply(api -> api.setClusterState( clusterName, - context.getSuboperationTimeoutInSeconds(SHARE_REMAINING_TIME), - stateRequest)); - } catch (IOException e) { + timeouts.getServerTimeoutOrThrow().toMillis() / 1000.0f, + stateRequest), + timeouts); + } catch (IOException | UncheckedTimeoutException e) { final String message = String.format( "Giving up setting %s for cluster %s", stateRequest, diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientTimeouts.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientTimeouts.java new file mode 100644 index 00000000000..22f2440b408 --- /dev/null +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientTimeouts.java @@ -0,0 +1,74 @@ +package com.yahoo.vespa.orchestrator.controller; + +import com.google.common.util.concurrent.UncheckedTimeoutException; +import com.yahoo.time.TimeBudget; +import com.yahoo.vespa.jaxrs.client.JaxRsTimeouts; + +import java.time.Duration; + +/** + * Calculates various timeouts associated with a REST call from the Orchestrator to the Cluster Controller. + * + * <p>Timeout handling of HTTP messaging is fundamentally flawed in various Java implementations. + * We would like to specify a max time for the whole operation (connect, send request, and receive response). + * Jersey JAX-RS implementation and the Apache HTTP client library provides a way to set the connect timeout C + * and read timeout R. So if the operation takes NR reads, and the writes takes TW time, + * the theoretical max time is: T = C + R * NR + TW. With both NR and TW unknown, there's no way to + * set a proper C and R.</p> + * + * @author hakonhall + */ +public class ClusterControllerClientTimeouts implements JaxRsTimeouts { + static final Duration CONNECT_TIMEOUT = Duration.ofMillis(100); + // Time reserved to guarantee that even though the server application honors a server timeout S, + // some time will pass before the server sees the timeout, and after it has returned. + static final Duration DOWNSTREAM_OVERHEAD = Duration.ofMillis(300); + // Minimum server-side timeout + static final Duration MIN_SERVER_TIMEOUT = Duration.ofMillis(100); + + private final TimeBudget timeBudget; + + /** + * Creates a timeouts instance. + * + * The {@link #timeBudget} SHOULD be the time budget for a single logical call to the Cluster Controller. + * A logical call to CC may in fact call the CC several times, if the first ones are down and/or not + * the master. + * + * @param timeBudget The time budget for a single logical call to the the Cluster Controller. + */ + public ClusterControllerClientTimeouts(TimeBudget timeBudget) { + this.timeBudget = timeBudget; + } + + @Override + public Duration getConnectTimeoutOrThrow() { + return CONNECT_TIMEOUT; + } + + @Override + public Duration getReadTimeoutOrThrow() { + Duration timeLeft = timeBudget.timeLeft().get(); + + // timeLeft = CONNECT_TIMEOUT + readTimeout + Duration readTimeout = timeLeft.minus(CONNECT_TIMEOUT); + + if (readTimeout.toMillis() <= 0) { + throw new UncheckedTimeoutException("Timed out after " + timeBudget.originalTimeout().get()); + } + + return readTimeout; + } + + public Duration getServerTimeoutOrThrow() { + // readTimeout = DOWNSTREAM_OVERHEAD + serverTimeout + Duration serverTimeout = getReadTimeoutOrThrow().minus(DOWNSTREAM_OVERHEAD); + + if (serverTimeout.toMillis() < MIN_SERVER_TIMEOUT.toMillis()) { + throw new UncheckedTimeoutException("Timed out after " + timeBudget.originalTimeout().get()); + } + + return serverTimeout; + } + +} diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerStateErrorResponse.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerStateErrorResponse.java new file mode 100644 index 00000000000..042eca0040e --- /dev/null +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerStateErrorResponse.java @@ -0,0 +1,19 @@ +package com.yahoo.vespa.orchestrator.controller; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; + +/** + * Error response from cluster controller. + * + * @author hakonhall + */ +public class ClusterControllerStateErrorResponse { + @JsonProperty("message") + public final String message; + + @JsonCreator + public ClusterControllerStateErrorResponse(@JsonProperty("message") String message) { + this.message = message; + } +} diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/RetryingClusterControllerClientFactory.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/RetryingClusterControllerClientFactory.java index 5571eedeec6..33e74235862 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/RetryingClusterControllerClientFactory.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/RetryingClusterControllerClientFactory.java @@ -8,9 +8,8 @@ import com.yahoo.vespa.jaxrs.client.JaxRsStrategy; import com.yahoo.vespa.jaxrs.client.JaxRsStrategyFactory; import com.yahoo.vespa.jaxrs.client.JerseyJaxRsClientFactory; +import java.util.HashSet; import java.util.List; -import java.util.Set; -import java.util.stream.Collectors; /** * @author bakksjo @@ -21,14 +20,12 @@ public class RetryingClusterControllerClientFactory implements ClusterController public static final int HARDCODED_CLUSTERCONTROLLER_PORT = 19050; public static final String CLUSTERCONTROLLER_API_PATH = "/"; public static final String CLUSTERCONTROLLER_SCHEME = "http"; - private static final int CLUSTER_CONTROLLER_CONNECT_TIMEOUT_MS = 1000; - private static final int CLUSTER_CONTROLLER_READ_TIMEOUT_MS = 1000; private JaxRsClientFactory jaxRsClientFactory; @Inject public RetryingClusterControllerClientFactory() { - this(new JerseyJaxRsClientFactory(CLUSTER_CONTROLLER_CONNECT_TIMEOUT_MS, CLUSTER_CONTROLLER_READ_TIMEOUT_MS)); + this(new JerseyJaxRsClientFactory()); } public RetryingClusterControllerClientFactory(JaxRsClientFactory jaxRsClientFactory) { @@ -36,19 +33,21 @@ public class RetryingClusterControllerClientFactory implements ClusterController } @Override - public ClusterControllerClient createClient(List<HostName> clusterControllers, - String clusterName) { - Set<HostName> clusterControllerSet = clusterControllers.stream().collect(Collectors.toSet()); - JaxRsStrategy<ClusterControllerJaxRsApi> jaxRsApi - = new JaxRsStrategyFactory(clusterControllerSet, HARDCODED_CLUSTERCONTROLLER_PORT, jaxRsClientFactory, CLUSTERCONTROLLER_SCHEME) - .apiWithRetries(ClusterControllerJaxRsApi.class, CLUSTERCONTROLLER_API_PATH) - // Use max iteration 1. The JaxRsStrategyFactory will try host 1, 2, then 3: - // - If host 1 responds, it will redirect to master if necessary. Otherwise - // - If host 2 responds, it will redirect to master if necessary. Otherwise - // - If host 3 responds, it may redirect to master if necessary (if they're up - // after all), but more likely there's no quorum and this will fail too. - .setMaxIterations(1); + public ClusterControllerClient createClient(List<HostName> clusterControllers, String clusterName) { + JaxRsStrategy<ClusterControllerJaxRsApi> jaxRsApi = + new JaxRsStrategyFactory( + new HashSet<>(clusterControllers), + HARDCODED_CLUSTERCONTROLLER_PORT, + jaxRsClientFactory, + CLUSTERCONTROLLER_SCHEME) + .apiWithRetries(ClusterControllerJaxRsApi.class, CLUSTERCONTROLLER_API_PATH) + // Use max iteration 1: The JaxRsStrategyFactory will try host 1, 2, then 3: + // - If host 1 responds, it will redirect to master if necessary. Otherwise + // - If host 2 responds, it will redirect to master if necessary. Otherwise + // - If host 3 responds, it may redirect to master if necessary (if they're up + // after all), but more likely there's no quorum and this will fail too. + // If there's only 1 CC, we'll try that one twice. + .setMaxIterations(clusterControllers.size() > 1 ? 1 : 2); return new ClusterControllerClientImpl(jaxRsApi, clusterName); } - } diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/SingleInstanceClusterControllerClientFactory.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/SingleInstanceClusterControllerClientFactory.java deleted file mode 100644 index 7459f0a6b11..00000000000 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/controller/SingleInstanceClusterControllerClientFactory.java +++ /dev/null @@ -1,54 +0,0 @@ -// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -package com.yahoo.vespa.orchestrator.controller; - -import com.yahoo.log.LogLevel; -import com.yahoo.vespa.applicationmodel.HostName; -import com.yahoo.vespa.jaxrs.client.JaxRsClientFactory; -import com.yahoo.vespa.jaxrs.client.JaxRsStrategy; -import com.yahoo.vespa.jaxrs.client.NoRetryJaxRsStrategy; - -import java.util.List; -import java.util.logging.Logger; - -/** - * @author bakksjo - */ -public class SingleInstanceClusterControllerClientFactory implements ClusterControllerClientFactory { - - public static final int CLUSTERCONTROLLER_HARDCODED_PORT = 19050; - public static final String CLUSTERCONTROLLER_HARDCODED_SCHEME = "http"; - public static final String CLUSTERCONTROLLER_API_PATH = "/"; - - private static final Logger log = Logger.getLogger(SingleInstanceClusterControllerClientFactory.class.getName()); - - private JaxRsClientFactory jaxRsClientFactory; - - public SingleInstanceClusterControllerClientFactory(JaxRsClientFactory jaxRsClientFactory) { - this.jaxRsClientFactory = jaxRsClientFactory; - } - - @Override - public ClusterControllerClient createClient(List<HostName> clusterControllers, - String clusterName) { - if (clusterControllers.isEmpty()) { - throw new IllegalArgumentException("No cluster controller instances found"); - } - HostName controllerHostName = clusterControllers.iterator().next(); - int port = CLUSTERCONTROLLER_HARDCODED_PORT; // TODO: Get this from service monitor. - - log.log(LogLevel.DEBUG, () -> - "For cluster '" + clusterName + "' with controllers " + clusterControllers - + ", creating api client for " + controllerHostName.s() + ":" + port); - - JaxRsStrategy<ClusterControllerJaxRsApi> strategy = new NoRetryJaxRsStrategy<>( - controllerHostName, - port, - jaxRsClientFactory, - ClusterControllerJaxRsApi.class, - CLUSTERCONTROLLER_API_PATH, - CLUSTERCONTROLLER_HARDCODED_SCHEME); - - return new ClusterControllerClientImpl(strategy, clusterName); - } - -} diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/NodeGroup.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/NodeGroup.java index ed506c82079..74b4b534acc 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/NodeGroup.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/NodeGroup.java @@ -43,7 +43,7 @@ public class NodeGroup { } public List<HostName> getHostNames() { - return hostNames.stream().collect(Collectors.toList()).stream().sorted().collect(Collectors.toList()); + return hostNames.stream().sorted().collect(Collectors.toList()); } public String toCommaSeparatedString() { diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/StorageNodeImpl.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/StorageNodeImpl.java index 3e387012d2c..9900c8de752 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/StorageNodeImpl.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/StorageNodeImpl.java @@ -12,10 +12,13 @@ import com.yahoo.vespa.orchestrator.OrchestratorContext; import com.yahoo.vespa.orchestrator.controller.ClusterControllerClient; import com.yahoo.vespa.orchestrator.controller.ClusterControllerClientFactory; import com.yahoo.vespa.orchestrator.controller.ClusterControllerNodeState; +import com.yahoo.vespa.orchestrator.controller.ClusterControllerStateErrorResponse; import com.yahoo.vespa.orchestrator.controller.ClusterControllerStateResponse; import com.yahoo.vespa.orchestrator.policy.HostStateChangeDeniedException; import com.yahoo.vespa.orchestrator.policy.HostedVespaPolicy; +import javax.ws.rs.WebApplicationException; +import javax.ws.rs.core.Response; import java.io.IOException; import java.util.List; import java.util.Objects; @@ -75,6 +78,17 @@ public class StorageNodeImpl implements StorageNode { HostedVespaPolicy.CLUSTER_CONTROLLER_AVAILABLE_CONSTRAINT, "Failed to communicate with cluster controllers " + clusterControllers + ": " + e, e); + } catch (WebApplicationException e) { + Response webResponse = e.getResponse(); + // Response may contain detail message + ClusterControllerStateErrorResponse errorResponse = webResponse.readEntity(ClusterControllerStateErrorResponse.class); + String detail = errorResponse.message == null ? "" : ": " + errorResponse.message; + throw new HostStateChangeDeniedException( + hostName(), + HostedVespaPolicy.SET_NODE_STATE_CONSTRAINT, + "Failure from cluster controllers " + clusterControllers + " when setting node " + nodeIndex + + " in cluster " + clusterId + " to state " + wantedNodeState + detail, + e); } catch (UncheckedTimeoutException e) { throw new HostStateChangeDeniedException( hostName(), diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/InMemoryStatusService.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/InMemoryStatusService.java index c5ae553a98c..bd5eb6f3e29 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/InMemoryStatusService.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/InMemoryStatusService.java @@ -4,6 +4,7 @@ package com.yahoo.vespa.orchestrator.status; import com.yahoo.vespa.applicationmodel.ApplicationInstanceReference; import com.yahoo.vespa.applicationmodel.HostName; +import java.time.Duration; import java.util.HashMap; import java.util.HashSet; import java.util.Map; @@ -42,15 +43,11 @@ public class InMemoryStatusService implements StatusService { }; } - @Override - public MutableStatusRegistry lockApplicationInstance_forCurrentThreadOnly(ApplicationInstanceReference applicationInstanceReference) { - return lockApplicationInstance_forCurrentThreadOnly(applicationInstanceReference, 10); - } @Override public MutableStatusRegistry lockApplicationInstance_forCurrentThreadOnly( ApplicationInstanceReference applicationInstanceReference, - long timeoutSeconds) { + Duration timeout) { Lock lock = instanceLockService.get(applicationInstanceReference); return new InMemoryMutableStatusRegistry(lock, applicationInstanceReference); } diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/StatusService.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/StatusService.java index c47be096242..76adef72b2b 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/StatusService.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/StatusService.java @@ -3,6 +3,7 @@ package com.yahoo.vespa.orchestrator.status; import com.yahoo.vespa.applicationmodel.ApplicationInstanceReference; +import java.time.Duration; import java.util.Set; /** @@ -24,7 +25,7 @@ public interface StatusService { * possibly inconsistent snapshot values. It is not recommended that this method is used for anything other * than monitoring, logging, debugging, etc. It should never be used for multi-step operations (e.g. * read-then-write) where consistency is required. For those cases, use - * {@link #lockApplicationInstance_forCurrentThreadOnly(ApplicationInstanceReference)}. + * {@link #lockApplicationInstance_forCurrentThreadOnly(ApplicationInstanceReference, Duration)}. */ ReadOnlyStatusRegistry forApplicationInstance(ApplicationInstanceReference applicationInstanceReference); @@ -52,12 +53,9 @@ public interface StatusService { * this case, subsequent mutating operations will fail, but previous mutating operations are NOT rolled back. * This may leave the registry in an inconsistent state (as judged by the client code). */ - MutableStatusRegistry lockApplicationInstance_forCurrentThreadOnly(ApplicationInstanceReference applicationInstanceReference); - - /** Lock application instance with timeout. */ MutableStatusRegistry lockApplicationInstance_forCurrentThreadOnly( ApplicationInstanceReference applicationInstanceReference, - long timeoutSeconds); + Duration timeout); /** * Returns all application instances that are allowed to be down. The intention is to use this diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusService.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusService.java index deece6a4a65..7df29e038c1 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusService.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusService.java @@ -56,21 +56,6 @@ public class ZookeeperStatusService implements StatusService { }; } - /** - * 1) locks the status service for an application instance. - * 2) fails all operations in this thread when the session is lost, - * since session loss might cause the lock to be lost. - * Since it only fails operations in this thread, - * all operations depending on a lock, including the locking itself, must be done in this thread. - * Note that since it is the thread that fails, all status operations in this thread will fail - * even if they're not supposed to be guarded by this lock - * (i.e. the request is for another applicationInstanceReference) - */ - @Override - public MutableStatusRegistry lockApplicationInstance_forCurrentThreadOnly(ApplicationInstanceReference applicationInstanceReference) { - return lockApplicationInstance_forCurrentThreadOnly(applicationInstanceReference, 10); - } - @Override public Set<ApplicationInstanceReference> getAllSuspendedApplications() { try { @@ -93,13 +78,23 @@ public class ZookeeperStatusService implements StatusService { } } + /** + * 1) locks the status service for an application instance. + * 2) fails all operations in this thread when the session is lost, + * since session loss might cause the lock to be lost. + * Since it only fails operations in this thread, + * all operations depending on a lock, including the locking itself, must be done in this thread. + * Note that since it is the thread that fails, all status operations in this thread will fail + * even if they're not supposed to be guarded by this lock + * (i.e. the request is for another applicationInstanceReference) + */ @Override public MutableStatusRegistry lockApplicationInstance_forCurrentThreadOnly( ApplicationInstanceReference applicationInstanceReference, - long timeoutSeconds) { + Duration timeout) { String lockPath = applicationInstanceLock2Path(applicationInstanceReference); Lock lock = new Lock(lockPath, curator); - lock.acquire(Duration.ofSeconds(timeoutSeconds)); + lock.acquire(timeout); try { return new ZkMutableStatusRegistry(lock, applicationInstanceReference); diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java index 76d9398c44e..80174d05a54 100644 --- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java +++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java @@ -43,6 +43,8 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import static org.mockito.Matchers.any; +import static org.mockito.Matchers.eq; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.mock; @@ -245,6 +247,8 @@ public class OrchestratorImplTest { // A spy is preferential because suspendAll() relies on delegating the hard work to suspend() and resume(). OrchestratorImpl orchestrator = spy(this.orchestrator); + OrchestratorContext context = mock(OrchestratorContext.class); + orchestrator.suspendAll( new HostName("parentHostname"), Arrays.asList( @@ -257,9 +261,9 @@ public class OrchestratorImplTest { // TEST6: tenant-id-3:application-instance-3:default // TEST1: test-tenant-id:application:instance InOrder order = inOrder(orchestrator); - order.verify(orchestrator).suspendGroup(DummyInstanceLookupService.TEST3_NODE_GROUP); - order.verify(orchestrator).suspendGroup(DummyInstanceLookupService.TEST6_NODE_GROUP); - order.verify(orchestrator).suspendGroup(DummyInstanceLookupService.TEST1_NODE_GROUP); + order.verify(orchestrator).suspendGroup(any(), eq(DummyInstanceLookupService.TEST3_NODE_GROUP)); + order.verify(orchestrator).suspendGroup(any(), eq(DummyInstanceLookupService.TEST6_NODE_GROUP)); + order.verify(orchestrator).suspendGroup(any(), eq(DummyInstanceLookupService.TEST1_NODE_GROUP)); order.verifyNoMoreInteractions(); } @@ -272,7 +276,7 @@ public class OrchestratorImplTest { DummyInstanceLookupService.TEST6_HOST_NAME, "some-constraint", "error message"); - doThrow(supensionFailure).when(orchestrator).suspendGroup(DummyInstanceLookupService.TEST6_NODE_GROUP); + doThrow(supensionFailure).when(orchestrator).suspendGroup(any(), eq(DummyInstanceLookupService.TEST6_NODE_GROUP)); try { orchestrator.suspendAll( @@ -291,8 +295,8 @@ public class OrchestratorImplTest { } InOrder order = inOrder(orchestrator); - order.verify(orchestrator).suspendGroup(DummyInstanceLookupService.TEST3_NODE_GROUP); - order.verify(orchestrator).suspendGroup(DummyInstanceLookupService.TEST6_NODE_GROUP); + order.verify(orchestrator).suspendGroup(any(), eq(DummyInstanceLookupService.TEST3_NODE_GROUP)); + order.verify(orchestrator).suspendGroup(any(), eq(DummyInstanceLookupService.TEST6_NODE_GROUP)); order.verifyNoMoreInteractions(); } diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientTest.java index 228174a9b3d..35dda403aed 100644 --- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientTest.java +++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientTest.java @@ -6,7 +6,8 @@ import com.yahoo.vespa.jaxrs.client.LocalPassThroughJaxRsStrategy; import com.yahoo.vespa.orchestrator.OrchestratorContext; import org.junit.Test; -import static org.mockito.Matchers.anyFloat; +import java.time.Duration; + import static org.mockito.Matchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; @@ -28,7 +29,9 @@ public class ClusterControllerClientTest { final ClusterControllerNodeState wantedState = ClusterControllerNodeState.MAINTENANCE; OrchestratorContext context = mock(OrchestratorContext.class); - when(context.getSuboperationTimeoutInSeconds(anyFloat())).thenReturn(1.0f); + ClusterControllerClientTimeouts timeouts = mock(ClusterControllerClientTimeouts.class); + when(context.getClusterControllerTimeouts()).thenReturn(timeouts); + when(timeouts.getServerTimeoutOrThrow()).thenReturn(Duration.ofSeconds(1)); clusterControllerClient.setNodeState(context, STORAGE_NODE_INDEX, wantedState); final ClusterControllerStateRequest expectedNodeStateRequest = new ClusterControllerStateRequest( diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientTimeoutsTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientTimeoutsTest.java new file mode 100644 index 00000000000..63b8e498f16 --- /dev/null +++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/ClusterControllerClientTimeoutsTest.java @@ -0,0 +1,91 @@ +package com.yahoo.vespa.orchestrator.controller; + +import com.google.common.util.concurrent.UncheckedTimeoutException; +import com.yahoo.test.ManualClock; +import com.yahoo.time.TimeBudget; +import org.junit.Before; +import org.junit.Test; + +import java.time.Duration; +import java.util.Optional; + +import static com.yahoo.vespa.orchestrator.controller.ClusterControllerClientTimeouts.CONNECT_TIMEOUT; +import static com.yahoo.vespa.orchestrator.controller.ClusterControllerClientTimeouts.DOWNSTREAM_OVERHEAD; +import static com.yahoo.vespa.orchestrator.controller.ClusterControllerClientTimeouts.MIN_SERVER_TIMEOUT; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; + +public class ClusterControllerClientTimeoutsTest { + // The minimum time that allows for a single RPC to CC. + private static final Duration MINIMUM_TIME_LEFT = CONNECT_TIMEOUT + .plus(DOWNSTREAM_OVERHEAD) + .plus(MIN_SERVER_TIMEOUT); + static { + assertEquals(Duration.ofMillis(500), MINIMUM_TIME_LEFT); + } + + private final ManualClock clock = new ManualClock(); + + private Duration originalTimeout; + private TimeBudget timeBudget; + private ClusterControllerClientTimeouts timeouts; + + private void makeTimeouts(Duration originalTimeout) { + this.originalTimeout = originalTimeout; + this.timeBudget = TimeBudget.from(clock, clock.instant(), Optional.of(originalTimeout)); + this.timeouts = new ClusterControllerClientTimeouts(timeBudget); + } + + @Before + public void setUp() { + makeTimeouts(Duration.ofSeconds(3)); + } + + @Test + public void makesManyRequestsWithShortProcessingTime() { + assertEquals(Duration.ofMillis(100), timeouts.getConnectTimeoutOrThrow()); + assertEquals(Duration.ofMillis(2900), timeouts.getReadTimeoutOrThrow()); + assertEquals(Duration.ofMillis(2600), timeouts.getServerTimeoutOrThrow()); + + clock.advance(Duration.ofMillis(100)); + + assertEquals(Duration.ofMillis(100), timeouts.getConnectTimeoutOrThrow()); + assertEquals(Duration.ofMillis(2800), timeouts.getReadTimeoutOrThrow()); + assertEquals(Duration.ofMillis(2500), timeouts.getServerTimeoutOrThrow()); + + clock.advance(Duration.ofMillis(100)); + + assertEquals(Duration.ofMillis(100), timeouts.getConnectTimeoutOrThrow()); + assertEquals(Duration.ofMillis(2700), timeouts.getReadTimeoutOrThrow()); + assertEquals(Duration.ofMillis(2400), timeouts.getServerTimeoutOrThrow()); + } + + @Test + public void alreadyTimedOut() { + clock.advance(Duration.ofSeconds(4)); + + try { + timeouts.getServerTimeoutOrThrow(); + fail(); + } catch (UncheckedTimeoutException e) { + assertEquals("Timed out after PT3S", e.getMessage()); + } + } + + @Test + public void justTooLittleTime() { + clock.advance(originalTimeout.minus(MINIMUM_TIME_LEFT).plus(Duration.ofMillis(1))); + try { + timeouts.getServerTimeoutOrThrow(); + fail(); + } catch (UncheckedTimeoutException e) { + assertEquals("Timed out after PT3S", e.getMessage()); + } + } + + @Test + public void justEnoughTime() { + clock.advance(originalTimeout.minus(MINIMUM_TIME_LEFT)); + timeouts.getServerTimeoutOrThrow(); + } +}
\ No newline at end of file diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/RetryingClusterControllerClientFactoryTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/RetryingClusterControllerClientFactoryTest.java new file mode 100644 index 00000000000..f47b43fa27b --- /dev/null +++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/RetryingClusterControllerClientFactoryTest.java @@ -0,0 +1,54 @@ +package com.yahoo.vespa.orchestrator.controller; + +import com.yahoo.test.ManualClock; +import com.yahoo.vespa.applicationmodel.HostName; +import com.yahoo.vespa.jaxrs.client.JaxRsClientFactory; +import com.yahoo.vespa.orchestrator.OrchestratorContext; +import org.junit.Test; +import org.mockito.ArgumentCaptor; + +import java.io.IOException; +import java.time.Clock; +import java.util.Arrays; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import static org.junit.Assert.assertEquals; +import static org.mockito.Matchers.any; +import static org.mockito.Matchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +public class RetryingClusterControllerClientFactoryTest { + private final Clock clock = new ManualClock(); + + @Test + public void verifyJerseyCallForSetNodeState() throws IOException { + JaxRsClientFactory clientFactory = mock(JaxRsClientFactory.class); + ClusterControllerJaxRsApi api = mock(ClusterControllerJaxRsApi.class); + when(clientFactory.createClient(any())).thenReturn(api); + RetryingClusterControllerClientFactory factory = new RetryingClusterControllerClientFactory(clientFactory); + String clusterName = "clustername"; + HostName host1 = new HostName("host1"); + HostName host2 = new HostName("host2"); + HostName host3 = new HostName("host3"); + List<HostName> clusterControllers = Arrays.asList(host1, host2, host3); + ClusterControllerClient client = factory.createClient(clusterControllers, clusterName); + OrchestratorContext context = OrchestratorContext.createContextForSingleAppOp(clock); + int storageNode = 2; + ClusterControllerNodeState wantedState = ClusterControllerNodeState.MAINTENANCE; + client.setNodeState(context, storageNode, wantedState); + + ArgumentCaptor<ClusterControllerStateRequest> requestCaptor = ArgumentCaptor.forClass(ClusterControllerStateRequest.class); + + verify(api, times(1)).setNodeState(eq(clusterName), eq(storageNode), eq(9.6f), requestCaptor.capture()); + ClusterControllerStateRequest request = requestCaptor.getValue(); + assertEquals(ClusterControllerStateRequest.Condition.SAFE, request.condition); + Map<String, Object> expectedState = new HashMap<>(); + expectedState.put("user", new ClusterControllerStateRequest.State(wantedState, "Orchestrator")); + assertEquals(expectedState, request.state); + } +}
\ No newline at end of file diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/SingleInstanceClusterControllerClientFactoryTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/SingleInstanceClusterControllerClientFactoryTest.java deleted file mode 100644 index 4dabae14add..00000000000 --- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/controller/SingleInstanceClusterControllerClientFactoryTest.java +++ /dev/null @@ -1,116 +0,0 @@ -// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -package com.yahoo.vespa.orchestrator.controller; - -import com.yahoo.vespa.applicationmodel.ConfigId; -import com.yahoo.vespa.applicationmodel.HostName; -import com.yahoo.vespa.jaxrs.client.JaxRsClientFactory; -import com.yahoo.vespa.orchestrator.OrchestratorContext; -import org.junit.Before; -import org.junit.Test; - -import java.util.Arrays; -import java.util.List; - -import static org.hamcrest.CoreMatchers.anyOf; -import static org.hamcrest.CoreMatchers.equalTo; -import static org.hamcrest.CoreMatchers.is; -import static org.junit.Assert.fail; -import static org.mockito.Matchers.any; -import static org.mockito.Matchers.anyFloat; -import static org.mockito.Matchers.anyInt; -import static org.mockito.Matchers.anyString; -import static org.mockito.Matchers.argThat; -import static org.mockito.Matchers.eq; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; - -public class SingleInstanceClusterControllerClientFactoryTest { - private static final int PORT = SingleInstanceClusterControllerClientFactory.CLUSTERCONTROLLER_HARDCODED_PORT; - private static final String PATH = SingleInstanceClusterControllerClientFactory.CLUSTERCONTROLLER_API_PATH; - - private static final HostName HOST_NAME_1 = new HostName("host1"); - private static final HostName HOST_NAME_2 = new HostName("host2"); - private static final HostName HOST_NAME_3 = new HostName("host3"); - - OrchestratorContext context = mock(OrchestratorContext.class); - private final ClusterControllerJaxRsApi mockApi = mock(ClusterControllerJaxRsApi.class); - private final JaxRsClientFactory jaxRsClientFactory = mock(JaxRsClientFactory.class); - private final ClusterControllerClientFactory clientFactory - = new SingleInstanceClusterControllerClientFactory(jaxRsClientFactory); - - @Before - public void setup() { - when( - jaxRsClientFactory.createClient( - eq(ClusterControllerJaxRsApi.class), - any(HostName.class), - anyInt(), - anyString(), - anyString())) - .thenReturn(mockApi); - } - - @Test - public void testCreateClientWithNoClusterControllerInstances() throws Exception { - final List<HostName> clusterControllers = Arrays.asList(); - - try { - clientFactory.createClient(clusterControllers, "clusterName"); - fail(); - } catch (IllegalArgumentException e) { - // As expected. - } - } - - @Test - public void testCreateClientWithSingleClusterControllerInstance() throws Exception { - final List<HostName> clusterControllers = Arrays.asList(HOST_NAME_1); - - when(context.getSuboperationTimeoutInSeconds(anyFloat())).thenReturn(1.0f); - clientFactory.createClient(clusterControllers, "clusterName") - .setNodeState(context, 0, ClusterControllerNodeState.MAINTENANCE); - - verify(jaxRsClientFactory).createClient( - ClusterControllerJaxRsApi.class, - HOST_NAME_1, - PORT, - PATH, - "http"); - } - - @Test - public void testCreateClientWithoutClusterControllerInstances() throws Exception { - final List<HostName> clusterControllers = Arrays.asList(); - - try { - clientFactory.createClient(clusterControllers, "clusterName"); - fail(); - } catch (IllegalArgumentException e) { - // As expected. - } - } - - @Test - public void testCreateClientWithThreeClusterControllerInstances() throws Exception { - final List<HostName> clusterControllers = Arrays.asList(HOST_NAME_1, HOST_NAME_2, HOST_NAME_3); - - when(context.getSuboperationTimeoutInSeconds(anyFloat())).thenReturn(1.0f); - clientFactory.createClient(clusterControllers, "clusterName") - .setNodeState(context, 0, ClusterControllerNodeState.MAINTENANCE); - - verify(jaxRsClientFactory).createClient( - eq(ClusterControllerJaxRsApi.class), - argThat(is(anyOf( - equalTo(HOST_NAME_1), - equalTo(HOST_NAME_2), - equalTo(HOST_NAME_3)))), - eq(PORT), - eq(PATH), - eq("http")); - } - - private static ConfigId clusterControllerConfigId(final int index) { - return new ConfigId("admin/cluster-controllers/" + index); - } -} diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/HostResourceTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/HostResourceTest.java index 45ba862c8f1..a9b8127e7fe 100644 --- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/HostResourceTest.java +++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/HostResourceTest.java @@ -1,7 +1,6 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.orchestrator.resources; -import com.yahoo.jdisc.Timer; import com.yahoo.vespa.applicationmodel.ApplicationInstance; import com.yahoo.vespa.applicationmodel.ApplicationInstanceId; import com.yahoo.vespa.applicationmodel.ApplicationInstanceReference; @@ -32,6 +31,7 @@ import com.yahoo.vespa.orchestrator.status.ApplicationInstanceStatus; import com.yahoo.vespa.orchestrator.status.HostStatus; import com.yahoo.vespa.orchestrator.status.MutableStatusRegistry; import com.yahoo.vespa.orchestrator.status.StatusService; +import org.junit.Before; import org.junit.Test; import javax.ws.rs.BadRequestException; @@ -41,6 +41,7 @@ import javax.ws.rs.core.UriBuilder; import javax.ws.rs.core.UriInfo; import java.net.URI; import java.time.Clock; +import java.time.Instant; import java.util.Arrays; import java.util.Collections; import java.util.Optional; @@ -74,7 +75,7 @@ public class HostResourceTest { static { when(EVERY_HOST_IS_UP_HOST_STATUS_SERVICE.forApplicationInstance(eq(APPLICATION_INSTANCE_REFERENCE))) .thenReturn(EVERY_HOST_IS_UP_MUTABLE_HOST_STATUS_REGISTRY); - when(EVERY_HOST_IS_UP_HOST_STATUS_SERVICE.lockApplicationInstance_forCurrentThreadOnly(eq(APPLICATION_INSTANCE_REFERENCE))) + when(EVERY_HOST_IS_UP_HOST_STATUS_SERVICE.lockApplicationInstance_forCurrentThreadOnly(eq(APPLICATION_INSTANCE_REFERENCE), any())) .thenReturn(EVERY_HOST_IS_UP_MUTABLE_HOST_STATUS_REGISTRY); when(EVERY_HOST_IS_UP_MUTABLE_HOST_STATUS_REGISTRY.getHostStatus(any())) .thenReturn(HostStatus.NO_REMARKS); @@ -150,6 +151,11 @@ public class HostResourceTest { private final UriInfo uriInfo = mock(UriInfo.class); + @Before + public void setUp() { + when(clock.instant()).thenReturn(Instant.now()); + } + @Test public void returns_200_on_success() { HostResource hostResource = diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusServiceTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusServiceTest.java index 2e914718e20..44847666670 100644 --- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusServiceTest.java +++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusServiceTest.java @@ -17,6 +17,7 @@ import org.junit.After; import org.junit.Before; import org.junit.Test; +import java.time.Duration; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -81,7 +82,8 @@ public class ZookeeperStatusServiceTest { @Test public void setting_host_state_is_idempotent() { try (MutableStatusRegistry statusRegistry = zookeeperStatusService.lockApplicationInstance_forCurrentThreadOnly( - TestIds.APPLICATION_INSTANCE_REFERENCE)) { + TestIds.APPLICATION_INSTANCE_REFERENCE, + Duration.ofSeconds(10))) { //shuffling to catch "clean database" failures for all cases. for (HostStatus hostStatus: shuffledList(HostStatus.values())) { @@ -105,11 +107,12 @@ public class ZookeeperStatusServiceTest { final CompletableFuture<Void> lockedSuccessfullyFuture; try (MutableStatusRegistry statusRegistry = zookeeperStatusService.lockApplicationInstance_forCurrentThreadOnly( - TestIds.APPLICATION_INSTANCE_REFERENCE)) { + TestIds.APPLICATION_INSTANCE_REFERENCE, + Duration.ofSeconds(10))) { lockedSuccessfullyFuture = CompletableFuture.runAsync(() -> { try (MutableStatusRegistry statusRegistry2 = zookeeperStatusService2 - .lockApplicationInstance_forCurrentThreadOnly(TestIds.APPLICATION_INSTANCE_REFERENCE)) + .lockApplicationInstance_forCurrentThreadOnly(TestIds.APPLICATION_INSTANCE_REFERENCE, Duration.ofSeconds(10))) { } }); @@ -131,13 +134,13 @@ public class ZookeeperStatusServiceTest { ZookeeperStatusService zookeeperStatusService2 = new ZookeeperStatusService(curator); try (MutableStatusRegistry statusRegistry = zookeeperStatusService.lockApplicationInstance_forCurrentThreadOnly( - TestIds.APPLICATION_INSTANCE_REFERENCE)) { + TestIds.APPLICATION_INSTANCE_REFERENCE, Duration.ofSeconds(10))) { //must run in separate thread, since having 2 locks in the same thread fails CompletableFuture<Void> resultOfZkOperationAfterLockFailure = CompletableFuture.runAsync(() -> { try { zookeeperStatusService2.lockApplicationInstance_forCurrentThreadOnly( - TestIds.APPLICATION_INSTANCE_REFERENCE,1); + TestIds.APPLICATION_INSTANCE_REFERENCE, Duration.ofSeconds(1)); fail("Both zookeeper host status services locked simultaneously for the same application instance"); } catch (RuntimeException e) { } @@ -211,7 +214,7 @@ public class ZookeeperStatusServiceTest { // Suspend try (MutableStatusRegistry statusRegistry = zookeeperStatusService.lockApplicationInstance_forCurrentThreadOnly( - TestIds.APPLICATION_INSTANCE_REFERENCE)) { + TestIds.APPLICATION_INSTANCE_REFERENCE, Duration.ofSeconds(10))) { statusRegistry.setApplicationInstanceStatus(ApplicationInstanceStatus.ALLOWED_TO_BE_DOWN); } @@ -223,7 +226,7 @@ public class ZookeeperStatusServiceTest { // Resume try (MutableStatusRegistry statusRegistry = zookeeperStatusService.lockApplicationInstance_forCurrentThreadOnly( - TestIds.APPLICATION_INSTANCE_REFERENCE)) { + TestIds.APPLICATION_INSTANCE_REFERENCE, Duration.ofSeconds(10))) { statusRegistry.setApplicationInstanceStatus(ApplicationInstanceStatus.NO_REMARKS); } @@ -241,12 +244,12 @@ public class ZookeeperStatusServiceTest { assertThat(suspendedApps.size(), is(0)); try (MutableStatusRegistry statusRegistry = zookeeperStatusService.lockApplicationInstance_forCurrentThreadOnly( - TestIds.APPLICATION_INSTANCE_REFERENCE)) { + TestIds.APPLICATION_INSTANCE_REFERENCE, Duration.ofSeconds(10))) { statusRegistry.setApplicationInstanceStatus(ApplicationInstanceStatus.ALLOWED_TO_BE_DOWN); } try (MutableStatusRegistry statusRegistry = zookeeperStatusService.lockApplicationInstance_forCurrentThreadOnly( - TestIds.APPLICATION_INSTANCE_REFERENCE2)) { + TestIds.APPLICATION_INSTANCE_REFERENCE2, Duration.ofSeconds(10))) { statusRegistry.setApplicationInstanceStatus(ApplicationInstanceStatus.ALLOWED_TO_BE_DOWN); } diff --git a/searchcommon/src/vespa/searchcommon/common/schema.cpp b/searchcommon/src/vespa/searchcommon/common/schema.cpp index 5e9db03bf31..cef74409024 100644 --- a/searchcommon/src/vespa/searchcommon/common/schema.cpp +++ b/searchcommon/src/vespa/searchcommon/common/schema.cpp @@ -4,6 +4,7 @@ #include <fstream> #include <vespa/config/common/configparser.h> #include <vespa/vespalib/stllike/asciistream.h> +#include <vespa/vespalib/stllike/hashtable.hpp> #include <vespa/fastos/file.h> #include <vespa/log/log.h> @@ -421,6 +422,25 @@ Schema::getFieldSetId(vespalib::stringref name) const return getFieldId(name, _fieldSetIds); } +bool +Schema::isIndexField(vespalib::stringref name) const +{ + return _indexIds.find(name) != _indexIds.end(); +} + +bool +Schema::isSummaryField(vespalib::stringref name) const +{ + return _summaryIds.find(name) != _summaryIds.end(); +} + +bool +Schema::isAttributeField(vespalib::stringref name) const +{ + return _attributeIds.find(name) != _attributeIds.end(); +} + + void Schema::swap(Schema &rhs) { diff --git a/searchcommon/src/vespa/searchcommon/common/schema.h b/searchcommon/src/vespa/searchcommon/common/schema.h index d066ccb0a27..f8020d6ed9a 100644 --- a/searchcommon/src/vespa/searchcommon/common/schema.h +++ b/searchcommon/src/vespa/searchcommon/common/schema.h @@ -293,11 +293,7 @@ public: * @return true if field is an index field. * @param name the name of the field. **/ - bool - isIndexField(vespalib::stringref name) const - { - return _indexIds.find(name) != _indexIds.end(); - } + bool isIndexField(vespalib::stringref name) const; /** * Check if a field is a summary field @@ -305,22 +301,15 @@ public: * @return true if field is an summary field. * @param name the name of the field. **/ - bool - isSummaryField(vespalib::stringref name) const - { - return _summaryIds.find(name) != _summaryIds.end(); - } + bool isSummaryField(vespalib::stringref name) const; + /** * Check if a field is a attribute field * * @return true if field is an attribute field. * @param name the name of the field. **/ - bool - isAttributeField(vespalib::stringref name) const - { - return _attributeIds.find(name) != _attributeIds.end(); - } + bool isAttributeField(vespalib::stringref name) const; /** * Get information about a specific attribute field using the given fieldId. diff --git a/searchlib/src/vespa/searchlib/aggregation/group.cpp b/searchlib/src/vespa/searchlib/aggregation/group.cpp index 6eb131fe77d..17e56c9af21 100644 --- a/searchlib/src/vespa/searchlib/aggregation/group.cpp +++ b/searchlib/src/vespa/searchlib/aggregation/group.cpp @@ -34,8 +34,8 @@ struct SortByGroupRank { } }; -void reset(Group * & v) { v = NULL; } -void destruct(Group * v) { if (v) { delete v; } } +void reset(Group * & v) { v = nullptr; } +void destruct(Group * v) { delete v; } void destruct(Group::GroupList & l, size_t m) @@ -44,7 +44,7 @@ destruct(Group::GroupList & l, size_t m) destruct(l[i]); } delete [] l; - l = NULL; + l = nullptr; } } @@ -97,13 +97,13 @@ Group::groupNext(const GroupingLevel & level, const Doc & doc, HitRank rank) Group * Group::Value::groupSingle(const ResultNode & selectResult, HitRank rank, const GroupingLevel & level) { - if (_childInfo._childMap == NULL) { + if (_childInfo._childMap == nullptr) { assert(getChildrenSize() == 0); _childInfo._childMap = new GroupHash(1, GroupHasher(&_children), GroupEqual(&_children)); } GroupHash & childMap = *_childInfo._childMap; - Group * group(NULL); - GroupHash::iterator found = childMap.find<ResultNode, GroupResult, ResultHash, ResultEqual>(selectResult, GroupResult(&_children)); + Group * group(nullptr); + GroupHash::iterator found = childMap.find(selectResult); if (found == childMap.end()) { // group not present in child map if (level.allowMoreGroups(childMap.size())) { group = new Group(level.getGroupPrototype()); @@ -203,7 +203,7 @@ Group::Group() : Group::Group(const Group & rhs) = default; Group & Group::operator = (const Group & rhs) = default; -Group::~Group() { } +Group::~Group() = default; Group & Group::partialCopy(const Group & rhs) { @@ -230,7 +230,7 @@ void Group::Value::addExpressionResult(ExpressionNode::UP expressionNode) { uint32_t newSize = getAggrSize() + getExprSize() + 1; - ExpressionVector n = new ExpressionNode::CP[newSize]; + auto n = new ExpressionNode::CP[newSize]; for (uint32_t i(0); i < (newSize - 1); i++) { n[i] = std::move(_aggregationResults[i]); } @@ -246,7 +246,7 @@ Group::Value::addAggregationResult(ExpressionNode::UP aggr) { assert(getAggrSize() < 15); size_t newSize = getAggrSize() + 1 + getExprSize(); - ExpressionVector n = new ExpressionNode::CP[newSize]; + auto n = new ExpressionNode::CP[newSize]; for (size_t i(0), m(getAggrSize()); i < m; i++) { n[i] = std::move(_aggregationResults[i]); } @@ -292,10 +292,10 @@ Group::Value::addChild(Group * child) { const size_t sz(getChildrenSize()); assert(sz < 0xffffff); - if (_children == 0) { + if (_children == nullptr) { _children = new ChildP[4]; } else if ((sz >=4) && vespalib::Optimized::msbIdx(sz) == vespalib::Optimized::lsbIdx(sz)) { - GroupList n = new ChildP[sz*2]; + auto n = new ChildP[sz*2]; for (size_t i(0), m(getChildrenSize()); i < m; i++) { n[i] = _children[i]; } @@ -317,7 +317,7 @@ Group::Value::select(const vespalib::ObjectPredicate &predicate, vespalib::Objec void Group::Value::preAggregate() { - assert(_childInfo._childMap == NULL); + assert(_childInfo._childMap == nullptr); _childInfo._childMap = new GroupHash(getChildrenSize()*2, GroupHasher(&_children), GroupEqual(&_children)); GroupHash & childMap = *_childInfo._childMap; for (ChildP *it(_children), *mt(_children + getChildrenSize()); it != mt; ++it) { @@ -330,7 +330,7 @@ void Group::Value::postAggregate() { delete _childInfo._childMap; - _childInfo._childMap = NULL; + _childInfo._childMap = nullptr; for (ChildP *it(_children), *mt(_children + getChildrenSize()); it != mt; ++it) { (*it)->postAggregate(); } @@ -372,7 +372,7 @@ Group::Value::execute() { void Group::Value::mergeLevel(const Group & protoType, const Value & b) { for (ChildP *it(b._children), *mt(b._children + b.getChildrenSize()); it != mt; ++it) { - ChildP g(new Group(protoType)); + auto g(new Group(protoType)); g->partialCopy(**it); addChild(g); } @@ -382,7 +382,7 @@ void Group::Value::merge(const std::vector<GroupingLevel> &levels, uint32_t firstLevel, uint32_t currentLevel, const Value &b) { - GroupList z = new ChildP[getChildrenSize() + b.getChildrenSize()]; + auto z = new ChildP[getChildrenSize() + b.getChildrenSize()]; size_t kept(0); ChildP * px = _children; ChildP * ex = _children + getChildrenSize(); @@ -422,7 +422,7 @@ Group::Value::merge(const std::vector<GroupingLevel> &levels, void Group::Value::prune(const Value & b, uint32_t lastLevel, uint32_t currentLevel) { - GroupList keep = new ChildP[b.getChildrenSize()]; + auto keep = new ChildP[b.getChildrenSize()]; size_t kept(0); ChildP * px = _children; ChildP * ex = _children + getAllChildrenSize(); @@ -562,7 +562,7 @@ Group::Value::deserialize(Deserializer & is) { // results into a temporary buffer, and then reallocate the actual // vector when we know the total size. Then we copy the temp buffer and // deserialize the rest to the end of the vector. - ExpressionVector tmpAggregationResults = new ExpressionNode::CP[aggrSize]; + auto tmpAggregationResults = new ExpressionNode::CP[aggrSize]; setAggrSize(aggrSize); for(uint32_t i(0); i < aggrSize; i++) { is >> tmpAggregationResults[i]; @@ -589,7 +589,7 @@ Group::Value::deserialize(Deserializer & is) { _children = new ChildP[std::max(4ul, 2ul << vespalib::Optimized::msbIdx(count))]; setChildrenSize(count); for(uint32_t i(0); i < count; i++) { - ChildP group(new Group); + auto group(new Group); is >> *group; _children[i] = group; } @@ -633,24 +633,24 @@ Group::Value::visitMembers(vespalib::ObjectVisitor &visitor) const { Group::Value::Value() : _packedLength(0), _tag(-1), - _aggregationResults(NULL), - _children(NULL), + _aggregationResults(nullptr), + _children(nullptr), _childInfo(), _orderBy() { memset(_orderBy, 0, sizeof(_orderBy)); - _childInfo._childMap = NULL; + _childInfo._childMap = nullptr; } Group::Value::Value(const Value & rhs) : _packedLength(rhs._packedLength), _tag(rhs._tag), - _aggregationResults(NULL), - _children(NULL), + _aggregationResults(nullptr), + _children(nullptr), _childInfo(), _orderBy() { - _childInfo._childMap = NULL; + _childInfo._childMap = nullptr; memcpy(_orderBy, rhs._orderBy, sizeof(_orderBy)); uint32_t totalAggrSize = rhs.getAggrSize() + rhs.getExprSize(); if (totalAggrSize > 0) { @@ -665,17 +665,17 @@ Group::Value::Value(const Value & rhs) : _children = new ChildP[std::max(4ul, 2ul << vespalib::Optimized::msbIdx(rhs.getChildrenSize()))]; size_t i(0); for (const ChildP *it(rhs._children), *mt(rhs._children + rhs.getChildrenSize()); it != mt; ++it, i++) { - _children[i] = ChildP(new Group(**it)); + _children[i] = new Group(**it); } } } Group::Value::Value(Value && rhs) noexcept : - _packedLength(std::move(rhs._packedLength)), - _tag(std::move(rhs._tag)), - _aggregationResults(std::move(rhs._aggregationResults)), - _children(std::move(rhs._children)), - _childInfo(std::move(rhs._childInfo)), + _packedLength(rhs._packedLength), + _tag(rhs._tag), + _aggregationResults(rhs._aggregationResults), + _children(rhs._children), + _childInfo(rhs._childInfo), _orderBy() { memcpy(_orderBy, rhs._orderBy, sizeof(_orderBy)); @@ -688,11 +688,11 @@ Group::Value::Value(Value && rhs) noexcept : Group::Value & Group::Value::operator =(Value && rhs) noexcept { - _packedLength = std::move(rhs._packedLength); - _tag = std::move(rhs._tag); - _aggregationResults = std::move(rhs._aggregationResults); - _children = std::move(rhs._children); - _childInfo = std::move(rhs._childInfo); + _packedLength = rhs._packedLength; + _tag = rhs._tag; + _aggregationResults = rhs._aggregationResults; + _children = rhs._children; + _childInfo = rhs._childInfo; memcpy(_orderBy, rhs._orderBy, sizeof(_orderBy)); rhs.setChildrenSize(0); diff --git a/searchlib/src/vespa/searchlib/aggregation/group.h b/searchlib/src/vespa/searchlib/aggregation/group.h index c769b6c1d27..f302346f211 100644 --- a/searchlib/src/vespa/searchlib/aggregation/group.h +++ b/searchlib/src/vespa/searchlib/aggregation/group.h @@ -42,26 +42,18 @@ public: struct GroupEqual : public std::binary_function<ChildP, ChildP, bool> { GroupEqual(const GroupList * v) : _v(v) { } bool operator()(uint32_t a, uint32_t b) { return (*_v)[a]->getId().cmpFast((*_v)[b]->getId()) == 0; } + bool operator()(const Group & a, uint32_t b) { return a.getId().cmpFast((*_v)[b]->getId()) == 0; } + bool operator()(uint32_t a, const Group & b) { return (*_v)[a]->getId().cmpFast(b.getId()) == 0; } + bool operator()(const ResultNode & a, uint32_t b) { return a.cmpFast((*_v)[b]->getId()) == 0; } + bool operator()(uint32_t a, const ResultNode & b) { return (*_v)[a]->getId().cmpFast(b) == 0; } const GroupList *_v; }; struct GroupHasher { GroupHasher(const GroupList * v) : _v(v) { } size_t operator() (uint32_t arg) const { return (*_v)[arg]->getId().hash(); } - const GroupList *_v; - }; - struct GroupResult { - GroupResult(const GroupList * v) : _v(v) { } - const ResultNode & operator() (uint32_t arg) const { return (*_v)[arg]->getId(); } - const GroupList *_v; - }; - struct ResultLess : public std::binary_function<ResultNode::CP, ResultNode::CP, bool> { - bool operator()(const ResultNode::CP & a, const ResultNode::CP & b) { return a->cmpFast(*b) < 0; } - }; - struct ResultEqual : public std::binary_function<ResultNode, ResultNode, bool> { - bool operator()(const ResultNode & a, const ResultNode & b) { return a.cmpFast(b) == 0; } - }; - struct ResultHash { + size_t operator() (const Group & arg) const { return arg.getId().hash(); } size_t operator() (const ResultNode & arg) const { return arg.hash(); } + const GroupList *_v; }; using GroupingLevelList = std::vector<GroupingLevel>; @@ -192,12 +184,12 @@ public: return _aggr.groupSingle(result, rank, level); } - bool hasId() const { return (_id.get() != NULL); } + bool hasId() const { return static_cast<bool>(_id); } const ResultNode &getId() const { return *_id; } Group unchain() const { return *this; } - Group &setId(const ResultNode &id) { _id.reset(static_cast<ResultNode *>(id.clone())); return *this; } + Group &setId(const ResultNode &id) { _id.reset(id.clone()); return *this; } Group &addAggregationResult(ExpressionNode::UP result) { _aggr.addAggregationResult(std::move(result)); return *this; diff --git a/searchlib/src/vespa/searchlib/grouping/groupengine.cpp b/searchlib/src/vespa/searchlib/grouping/groupengine.cpp index aa289da068b..b652d5a840d 100644 --- a/searchlib/src/vespa/searchlib/grouping/groupengine.cpp +++ b/searchlib/src/vespa/searchlib/grouping/groupengine.cpp @@ -6,12 +6,10 @@ #include <vespa/vespalib/stllike/hash_set.hpp> #include <cassert> -namespace search { +using namespace search::expression; +using namespace search::aggregation; -using namespace expression; -using namespace aggregation; - -namespace grouping { +namespace search::grouping { GroupEngine::GroupEngine(const GroupingLevel * request, size_t level, GroupEngine * nextEngine, bool frozen) : Collect(request->getGroupPrototype()), @@ -52,7 +50,7 @@ GroupRef GroupEngine::group(Children & children, uint32_t docId, double rank) throw std::runtime_error("Does not know how to handle failed select statements"); } const ResultNode &selectResult = selector.getResult(); - Children::iterator found = children.find<ResultNode, GroupResult, Group::ResultHash, Group::ResultEqual>(selectResult, GroupResult(*this)); + Children::iterator found = children.find(selectResult); GroupRef gr; if (found == children.end()) { if (_request->allowMoreGroups(children.size())) { @@ -228,7 +226,6 @@ GroupEngine::preFillEngine(const Group & r, size_t depth) } } -} // this function was added by ../../forcelink.sh void forcelink_file_searchlib_grouping_groupengine() {} diff --git a/searchlib/src/vespa/searchlib/grouping/groupengine.h b/searchlib/src/vespa/searchlib/grouping/groupengine.h index a4f6d7a43f6..5167c37560a 100644 --- a/searchlib/src/vespa/searchlib/grouping/groupengine.h +++ b/searchlib/src/vespa/searchlib/grouping/groupengine.h @@ -5,8 +5,7 @@ #include <vespa/searchlib/grouping/collect.h> #include <vespa/vespalib/util/sort.h> -namespace search { -namespace grouping { +namespace search::grouping { class GroupEngine : protected Collect { @@ -15,6 +14,7 @@ public: public: GroupHash(const GroupEngine & engine) : _engine(engine) { } uint32_t operator () (GroupRef a) const { return _engine.hash(a); } + uint32_t operator () (const expression::ResultNode & a) const { return a.hash(); } private: const GroupEngine & _engine; }; @@ -22,6 +22,8 @@ public: public: GroupEqual(const GroupEngine & engine) : _engine(engine) { } bool operator () (GroupRef a, GroupRef b) const { return _engine.cmpId(a, b) == 0; } + bool operator () (const expression::ResultNode & a, GroupRef b) const { return a.cmpFast(_engine.getGroupId(b)) == 0; } + bool operator () (GroupRef a, const expression::ResultNode & b) const { return _engine.getGroupId(a).cmpFast(b) == 0; } private: const GroupEngine & _engine; }; @@ -137,4 +139,3 @@ private: }; } -} diff --git a/searchlib/src/vespa/searchlib/util/stringenum.cpp b/searchlib/src/vespa/searchlib/util/stringenum.cpp index 60238c32cc6..efcf33e73ab 100644 --- a/searchlib/src/vespa/searchlib/util/stringenum.cpp +++ b/searchlib/src/vespa/searchlib/util/stringenum.cpp @@ -2,13 +2,13 @@ #include "stringenum.h" #include <vespa/fastlib/io/bufferedfile.h> +#include <vespa/vespalib/stllike/hashtable.hpp> #include <cassert> #include <vespa/log/log.h> LOG_SETUP(".seachlib.util.stringenum"); -namespace search { -namespace util { +namespace search::util { static inline char * StripString(char *str) @@ -32,7 +32,14 @@ StripString(char *str) return first; } -StringEnum::~StringEnum() { } +StringEnum::StringEnum() + : _numEntries(0), + _mapping(), + _reverseMap() +{ +} + +StringEnum::~StringEnum() = default; void StringEnum::CreateReverseMapping() const @@ -123,5 +130,44 @@ StringEnum::Load(const char *filename) return true; } +void +StringEnum::Clear() +{ + _reverseMap.clear(); + _mapping.clear(); + _numEntries = 0; +} + +int +StringEnum::Add(const char *str) +{ + Map::const_iterator found(_mapping.find(str)); + if (found != _mapping.end()) { + return found->second; + } else { + int value = _numEntries++; + _mapping[str] = value; + return value; + } } + +int +StringEnum::Lookup(const char *str) const +{ + Map::const_iterator found(_mapping.find(str)); + return (found != _mapping.end()) ? found->second : -1; +} + +const char * +StringEnum::Lookup(uint32_t value) const +{ + if (value >= _numEntries) + return NULL; + + if (_numEntries > _reverseMap.size()) + CreateReverseMapping(); + + return _reverseMap[value]; +} + } diff --git a/searchlib/src/vespa/searchlib/util/stringenum.h b/searchlib/src/vespa/searchlib/util/stringenum.h index 44b3afca539..bd234ba2e36 100644 --- a/searchlib/src/vespa/searchlib/util/stringenum.h +++ b/searchlib/src/vespa/searchlib/util/stringenum.h @@ -5,8 +5,7 @@ #include <vector> #include <vespa/vespalib/stllike/hash_map.h> -namespace search { -namespace util { +namespace search::util { /** * An object of this class represents an enumeration of a set of @@ -35,29 +34,17 @@ public: /** * Create an empty string enumeration. **/ - StringEnum() - : _numEntries(0), - _mapping(), - _reverseMap() - { - } + StringEnum(); /** * Destructor. **/ ~StringEnum(); - /** * Discard all entries held by this object. **/ - void Clear() - { - _reverseMap.clear(); - _mapping.clear(); - _numEntries = 0; - } - + void Clear(); /** * Add a string to this enumeration. Equal strings will get the same @@ -68,18 +55,7 @@ public: * @return the enumerated value for the given string. * @param str string you want to add. **/ - int Add(const char *str) - { - Map::const_iterator found(_mapping.find(str)); - if (found != _mapping.end()) { - return found->second; - } else { - int value = _numEntries++; - _mapping[str] = value; - return value; - } - } - + int Add(const char *str); /** * Obtain the enumerated value for the given string. @@ -87,12 +63,7 @@ public: * @return enumerated value or -1 if not present. * @param str the string to look up. **/ - int Lookup(const char *str) const - { - Map::const_iterator found(_mapping.find(str)); - return (found != _mapping.end()) ? found->second : -1; - } - + int Lookup(const char *str) const; /** * Obtain the string for the given enumerated value. @@ -100,17 +71,7 @@ public: * @return string or NULL if out of range. * @param value the enumerated value to look up. **/ - const char *Lookup(uint32_t value) const - { - if (value >= _numEntries) - return NULL; - - if (_numEntries > _reverseMap.size()) - CreateReverseMapping(); - - return _reverseMap[value]; - } - + const char *Lookup(uint32_t value) const; /** * Obtain the number of entries currently present in this @@ -141,5 +102,3 @@ public: }; } -} - diff --git a/searchsummary/src/vespa/searchsummary/docsummary/keywordextractor.cpp b/searchsummary/src/vespa/searchsummary/docsummary/keywordextractor.cpp index d75ca47dd33..79266d34585 100644 --- a/searchsummary/src/vespa/searchsummary/docsummary/keywordextractor.cpp +++ b/searchsummary/src/vespa/searchsummary/docsummary/keywordextractor.cpp @@ -4,6 +4,7 @@ #include "keywordextractor.h" #include "idocsumenvironment.h" #include <vespa/searchlib/parsequery/stackdumpiterator.h> +#include <vespa/vespalib/stllike/hashtable.hpp> /** Tell us what parts of the query we are interested in */ @@ -12,12 +13,7 @@ namespace search::docsummary { bool useful(search::ParseItem::ItemCreator creator) { - switch (creator) { - case search::ParseItem::CREA_ORIG: - return true; - default: - return false; - } + return creator == search::ParseItem::CREA_ORIG; } @@ -38,6 +34,12 @@ KeywordExtractor::~KeywordExtractor() } } +bool +KeywordExtractor::IsLegalIndexName(const char *idxName) const +{ + return _legalIndexes.find(idxName) != _legalIndexes.end(); +} + KeywordExtractor::IndexPrefix::IndexPrefix(const char *prefix, IndexPrefix **list) : _prefix(NULL), _prefixLen(0), diff --git a/searchsummary/src/vespa/searchsummary/docsummary/keywordextractor.h b/searchsummary/src/vespa/searchsummary/docsummary/keywordextractor.h index 6c9ed675225..cfc73d606a0 100644 --- a/searchsummary/src/vespa/searchsummary/docsummary/keywordextractor.h +++ b/searchsummary/src/vespa/searchsummary/docsummary/keywordextractor.h @@ -51,11 +51,7 @@ private: return false; } - bool IsLegalIndexName(const char *idxName) const - { - return _legalIndexes.find(idxName) != _legalIndexes.end(); - } - + bool IsLegalIndexName(const char *idxName) const; public: explicit KeywordExtractor(IDocsumEnvironment * env); ~KeywordExtractor(); diff --git a/searchsummary/src/vespa/searchsummary/docsummary/resultclass.cpp b/searchsummary/src/vespa/searchsummary/docsummary/resultclass.cpp index 507ec7b3866..8066a5e65db 100644 --- a/searchsummary/src/vespa/searchsummary/docsummary/resultclass.cpp +++ b/searchsummary/src/vespa/searchsummary/docsummary/resultclass.cpp @@ -2,6 +2,7 @@ #include "resultclass.h" #include "resultconfig.h" +#include <vespa/vespalib/stllike/hashtable.hpp> #include <cassert> #include <zlib.h> @@ -18,8 +19,14 @@ ResultClass::ResultClass(const char *name, uint32_t id, util::StringEnum & field { } -ResultClass::~ResultClass() { } +ResultClass::~ResultClass() = default; +int +ResultClass::GetIndexFromName(const char* name) const +{ + NameIdMap::const_iterator found(_nameMap.find(name)); + return (found != _nameMap.end()) ? found->second : -1; +} bool ResultClass::AddConfigEntry(const char *name, ResType type) diff --git a/searchsummary/src/vespa/searchsummary/docsummary/resultclass.h b/searchsummary/src/vespa/searchsummary/docsummary/resultclass.h index d1504ed5bdd..e7c7c799b5f 100644 --- a/searchsummary/src/vespa/searchsummary/docsummary/resultclass.h +++ b/searchsummary/src/vespa/searchsummary/docsummary/resultclass.h @@ -245,11 +245,7 @@ public: * * @return field index or -1 if not found. **/ - int GetIndexFromName(const char* name) const - { - NameIdMap::const_iterator found(_nameMap.find(name)); - return (found != _nameMap.end()) ? found->second : -1; - } + int GetIndexFromName(const char* name) const; /** diff --git a/staging_vespalib/src/vespa/vespalib/objects/identifiable.cpp b/staging_vespalib/src/vespa/vespalib/objects/identifiable.cpp index 7e14fbc0014..84bdc0a45d0 100644 --- a/staging_vespalib/src/vespa/vespalib/objects/identifiable.cpp +++ b/staging_vespalib/src/vespa/vespalib/objects/identifiable.cpp @@ -27,18 +27,28 @@ public: bool erase(RuntimeClass * c); const RuntimeClass * classFromId(unsigned id) const; const RuntimeClass * classFromName(const char * name) const; - const char * id2Name(unsigned id) const; - unsigned name2Id(const char * name) const; bool empty() const { return _listById.empty(); } private: - struct GetId { uint32_t operator() (const RuntimeClass * f) const { return f->id(); } }; - struct HashId { size_t operator() (const RuntimeClass * f) const { return f->id(); } }; - struct EqualId { bool operator() (const RuntimeClass * a, const RuntimeClass * b) const { return a->id() == b->id(); } }; - struct GetName { const char * operator() (const RuntimeClass * f) const { return f->name(); } }; - struct HashName { size_t operator() (const RuntimeClass * f) const { return hashValue(f->name()); } }; - struct EqualName { bool operator() (const RuntimeClass * a, const RuntimeClass * b) const { return strcmp(a->name(), b->name()) == 0; } }; - typedef hash_set<RuntimeClass *, HashId, EqualId> IdList; - typedef hash_set<RuntimeClass *, HashName, EqualName> NameList; + struct HashId { + uint32_t operator() (const RuntimeClass * f) const { return f->id(); } + uint32_t operator() (uint32_t id) const { return id; } + }; + struct EqualId { + bool operator() (const RuntimeClass * a, const RuntimeClass * b) const { return a->id() == b->id(); } + bool operator() (const RuntimeClass * a, uint32_t b) const { return a->id() == b; } + bool operator() (uint32_t a, const RuntimeClass * b) const { return a == b->id(); } + }; + struct HashName { + uint32_t operator() (const RuntimeClass * f) const { return hashValue(f->name()); } + uint32_t operator() (const char * name) const { return hashValue(name); } + }; + struct EqualName { + bool operator() (const RuntimeClass * a, const RuntimeClass * b) const { return strcmp(a->name(), b->name()) == 0; } + bool operator() (const RuntimeClass * a, const char * b) const { return strcmp(a->name(), b) == 0; } + bool operator() (const char * a, const RuntimeClass * b) const { return strcmp(a, b->name()) == 0; } + }; + using IdList = hash_set<RuntimeClass *, HashId, EqualId>; + using NameList = hash_set<RuntimeClass *, HashName, EqualName>; IdList _listById; NameList _listByName; }; @@ -69,14 +79,14 @@ bool Register::append(Identifiable::RuntimeClass * c) const Identifiable::RuntimeClass * Register::classFromId(unsigned id) const { - IdList::const_iterator it(_listById.find<uint32_t, GetId, hash<uint32_t>, std::equal_to<uint32_t> >(id)); - return (it != _listById.end()) ? *it : NULL; + IdList::const_iterator it(_listById.find<uint32_t>(id)); + return (it != _listById.end()) ? *it : nullptr; } const Identifiable::RuntimeClass * Register::classFromName(const char *name) const { - NameList::const_iterator it(_listByName.find<const char *, GetName, hash<const char *>, std::equal_to<const char *> >(name)); - return (it != _listByName.end()) ? *it : NULL; + NameList::const_iterator it(_listByName.find<const char *>(name)); + return (it != _listByName.end()) ? *it : nullptr; } Register * _register = nullptr; @@ -115,7 +125,7 @@ Identifiable::RuntimeClass::RuntimeClass(RuntimeInfo * info_) : } } } - if (_register == NULL) { + if (_register == nullptr) { _register = new Register(); } if (! _register->append(this)) { @@ -131,7 +141,7 @@ Identifiable::RuntimeClass::~RuntimeClass() } if (_register->empty()) { delete _register; - _register = NULL; + _register = nullptr; } } @@ -142,20 +152,6 @@ bool Identifiable::RuntimeClass::inherits(unsigned cid) const return (cid == cur->_id); } -class SortById : public std::binary_function<const Identifiable::RuntimeClass *, const Identifiable::RuntimeClass *, bool> { -public: - bool operator() (const Identifiable::RuntimeClass * x, const Identifiable::RuntimeClass * y) const { - return x->id() < y->id(); - } -}; - -class SortByName : public std::binary_function<const Identifiable::RuntimeClass *, const Identifiable::RuntimeClass *, bool> { -public: - bool operator() (const Identifiable::RuntimeClass * x, const Identifiable::RuntimeClass * y) const { - return strcmp(x->name(), y->name()) < 0; - } -}; - Serializer & operator << (Serializer & os, const Identifiable & obj) { os.put(Identifiable::classIdField, obj.getClass().id()); @@ -199,16 +195,16 @@ Identifiable::UP Identifiable::create(Deserializer & is) is.get(classIdField, cid); UP obj; const Identifiable::RuntimeClass *rtc = Identifiable::classFromId(cid); - if (rtc == NULL) { - if ((_classLoader != NULL) && _classLoader->hasClass(cid)) { + if (rtc == nullptr) { + if ((_classLoader != nullptr) && _classLoader->hasClass(cid)) { _classLoader->loadClass(cid); rtc = Identifiable::classFromId(cid); - if (rtc == NULL) { + if (rtc == nullptr) { throw std::runtime_error(make_string("Failed loading class for Identifiable with classId %d(%0x)", cid, cid)); } } } - if (rtc != NULL) { + if (rtc != nullptr) { obj.reset(rtc->create()); if (obj.get()) { obj->deserialize(is); diff --git a/vespajlib/src/main/java/com/yahoo/time/TimeBudget.java b/vespajlib/src/main/java/com/yahoo/time/TimeBudget.java index a7963df0208..b3750440493 100644 --- a/vespajlib/src/main/java/com/yahoo/time/TimeBudget.java +++ b/vespajlib/src/main/java/com/yahoo/time/TimeBudget.java @@ -66,6 +66,23 @@ public class TimeBudget { }); } + /** Returns the time left, possibly negative if the deadline has passed. */ + public Optional<Duration> timeLeft() { + return timeout.map(timeout -> timeout.minus(timePassed())); + } + + /** Returns the time left as a new TimeBudget. */ + public TimeBudget timeLeftAsTimeBudget() { + Instant now = clock.instant(); + Optional<Instant> deadline = deadline(); + return new TimeBudget(clock, now, deadline.map(d -> Duration.between(now, d))); + } + + /** Returns a new TimeBudget with the same clock and start, but with this deadline. */ + public TimeBudget withDeadline(Instant deadline) { + return new TimeBudget(clock, start, Optional.of(Duration.between(start, deadline))); + } + private static Duration nonNegativeBetween(Instant start, Instant end) { return makeNonNegative(Duration.between(start, end)); } diff --git a/vespalib/src/tests/stllike/hash_test.cpp b/vespalib/src/tests/stllike/hash_test.cpp index e7fe729c1ba..017a16ee7b6 100644 --- a/vespalib/src/tests/stllike/hash_test.cpp +++ b/vespalib/src/tests/stllike/hash_test.cpp @@ -154,14 +154,14 @@ TEST("test hash set with simple type") TEST("test hash map iterator stability") { - hash_map<int, int> h; + hash_map<uint32_t, uint32_t> h; EXPECT_EQUAL(1ul, h.capacity()); for (size_t i(0); i < 100; i++) { EXPECT_TRUE(h.find(i) == h.end()); h[i] = i; EXPECT_TRUE(h.find(i) != h.end()); - int * p1 = & h.find(i)->second; - int * p2 = & h[i]; + uint32_t * p1 = & h.find(i)->second; + uint32_t * p2 = & h[i]; EXPECT_EQUAL(p1, p2); } EXPECT_EQUAL(128ul, h.capacity()); @@ -341,11 +341,11 @@ private: struct myhash { size_t operator() (const S & arg) const { return arg.hash(); } + size_t operator() (uint32_t arg) const { return arg; } }; -struct myextract { - uint32_t operator() (const S & arg) const { return arg.a(); } -}; +bool operator == (uint32_t a, const S & b) { return a == b.a(); } +bool operator == (const S & a, uint32_t b) { return a.a() == b; } TEST("test hash set find") { @@ -354,7 +354,7 @@ TEST("test hash set find") set.insert(S(i)); } EXPECT_TRUE(*set.find(S(1)) == S(1)); - hash_set<S, myhash>::iterator cit = set.find<uint32_t, myextract, vespalib::hash<uint32_t>, std::equal_to<uint32_t> >(7); + auto cit = set.find<uint32_t>(7); EXPECT_TRUE(*cit == S(7)); } diff --git a/vespalib/src/vespa/vespalib/stllike/hash_fun.h b/vespalib/src/vespa/vespalib/stllike/hash_fun.h index f708f49081e..a3401083f95 100644 --- a/vespalib/src/vespa/vespalib/stllike/hash_fun.h +++ b/vespalib/src/vespa/vespalib/stllike/hash_fun.h @@ -62,22 +62,18 @@ template<typename T> struct hash<const T *> { size_t hashValue(const char *str); size_t hashValue(const void *str, size_t sz); -template<> struct hash<const char *> { - size_t operator() (const char * arg) const { return hashValue(arg); } -}; - -template<> struct hash<vespalib::stringref> { - size_t operator() (vespalib::stringref arg) const { return hashValue(arg.data(), arg.size()); } -}; - -template<> struct hash<vespalib::string> { +struct hash_strings { size_t operator() (const vespalib::string & arg) const { return hashValue(arg.c_str()); } -}; - -template<> struct hash<std::string> { + size_t operator() (vespalib::stringref arg) const { return hashValue(arg.data(), arg.size()); } + size_t operator() (const char * arg) const { return hashValue(arg); } size_t operator() (const std::string& arg) const { return hashValue(arg.c_str()); } }; +template<> struct hash<const char *> : hash_strings { }; +template<> struct hash<vespalib::stringref> : public hash_strings { }; +template<> struct hash<vespalib::string> : hash_strings {}; +template<> struct hash<std::string> : hash_strings {}; + template<typename V> struct size { size_t operator() (const V & arg) const { return arg.size(); } }; @@ -86,6 +82,4 @@ template<typename V> struct zero { size_t operator() (const V & ) const { return 0; } }; - } // namespace vespalib - diff --git a/vespalib/src/vespa/vespalib/stllike/hash_map.h b/vespalib/src/vespa/vespalib/stllike/hash_map.h index 6d6498f8e78..34b22ba7ca3 100644 --- a/vespalib/src/vespa/vespalib/stllike/hash_map.h +++ b/vespalib/src/vespa/vespalib/stllike/hash_map.h @@ -6,7 +6,7 @@ namespace vespalib { -template< typename K, typename V, typename H = vespalib::hash<K>, typename EQ = std::equal_to<K>, typename M=hashtable_base::prime_modulator > +template< typename K, typename V, typename H = vespalib::hash<K>, typename EQ = std::equal_to<>, typename M=hashtable_base::prime_modulator > class hash_map { public: @@ -50,6 +50,16 @@ public: void erase(const_iterator it) { return erase(it->first); } iterator find(const K & key) { return _ht.find(key); } const_iterator find(const K & key) const { return _ht.find(key); } + + template< typename AltKey > + const_iterator find(const AltKey & key) const { + return _ht.template find<AltKey>(key); + } + template< typename AltKey> + iterator find(const AltKey & key) { + return _ht.template find<AltKey>(key); + } + void clear(); void resize(size_t newSize); void swap(hash_map & rhs); diff --git a/vespalib/src/vespa/vespalib/stllike/hash_map.hpp b/vespalib/src/vespa/vespalib/stllike/hash_map.hpp index b526188b8b2..74f1594965a 100644 --- a/vespalib/src/vespa/vespalib/stllike/hash_map.hpp +++ b/vespalib/src/vespa/vespalib/stllike/hash_map.hpp @@ -71,7 +71,7 @@ hash_map<K, V, H, EQ, M>::getMemoryUsed() const #define VESPALIB_HASH_MAP_INSTANTIATE_H_E(K, V, H, E) \ VESPALIB_HASH_MAP_INSTANTIATE_H_E_M(K, V, H, E, vespalib::hashtable_base::prime_modulator) -#define VESPALIB_HASH_MAP_INSTANTIATE_H(K, V, H) VESPALIB_HASH_MAP_INSTANTIATE_H_E(K, V, H, std::equal_to<K>) +#define VESPALIB_HASH_MAP_INSTANTIATE_H(K, V, H) VESPALIB_HASH_MAP_INSTANTIATE_H_E(K, V, H, std::equal_to<>) #define VESPALIB_HASH_MAP_INSTANTIATE(K, V) VESPALIB_HASH_MAP_INSTANTIATE_H(K, V, vespalib::hash<K>) diff --git a/vespalib/src/vespa/vespalib/stllike/hash_set.h b/vespalib/src/vespa/vespalib/stllike/hash_set.h index c4ccc662787..7a2db4735aa 100644 --- a/vespalib/src/vespa/vespalib/stllike/hash_set.h +++ b/vespalib/src/vespa/vespalib/stllike/hash_set.h @@ -7,7 +7,7 @@ namespace vespalib { -template< typename K, typename H = vespalib::hash<K>, typename EQ = std::equal_to<K>, typename M=hashtable_base::prime_modulator> +template< typename K, typename H = vespalib::hash<K>, typename EQ = std::equal_to<>, typename M=hashtable_base::prime_modulator> class hash_set { private: @@ -48,21 +48,11 @@ public: template <typename Func> void for_each(Func func) const { _ht.for_each(func); } - template< typename AltKey, typename AltExtract, typename AltHash, typename AltEqual > - const_iterator find(const AltKey & key) const { return _ht.template find<AltKey, AltExtract, AltHash, AltEqual>(key); } + template< typename AltKey > + const_iterator find(const AltKey & key) const { return _ht.template find<AltKey>(key); } - template< typename AltKey, typename AltExtract, typename AltHash, typename AltEqual > - iterator find(const AltKey & key) { return _ht.template find<AltKey, AltExtract, AltHash, AltEqual>(key); } - - template< typename AltKey, typename AltExtract, typename AltHash, typename AltEqual > - const_iterator find(const AltKey & key, const AltExtract & altExtract) const { - return _ht.template find<AltKey, AltExtract, AltHash, AltEqual>(key, altExtract); - } - - template< typename AltKey, typename AltExtract, typename AltHash, typename AltEqual > - iterator find(const AltKey & key, const AltExtract & altExtract) { - return _ht.template find<AltKey, AltExtract, AltHash, AltEqual>(key, altExtract); - } + template< typename AltKey> + iterator find(const AltKey & key) { return _ht.template find<AltKey>(key); } void clear(); void resize(size_t newSize); diff --git a/vespalib/src/vespa/vespalib/stllike/hash_set.hpp b/vespalib/src/vespa/vespalib/stllike/hash_set.hpp index cf6341218f1..f0427595382 100644 --- a/vespalib/src/vespa/vespalib/stllike/hash_set.hpp +++ b/vespalib/src/vespa/vespalib/stllike/hash_set.hpp @@ -84,11 +84,11 @@ hash_set<K, H, EQ, M>::insert(K &&value) { #define VESPALIB_HASH_SET_INSTANTIATE(K) \ template class vespalib::hash_set<K>; \ - template class vespalib::hashtable<K, K, vespalib::hash<K>, std::equal_to<K>, std::_Identity<K>>; \ + template class vespalib::hashtable<K, K, vespalib::hash<K>, std::equal_to<>, std::_Identity<K>>; \ template class vespalib::Array<vespalib::hash_node<K>>; #define VESPALIB_HASH_SET_INSTANTIATE_H(K, H) \ template class vespalib::hash_set<K, H>; \ - template class vespalib::hashtable<K, K, H, std::equal_to<K>, std::_Identity<K>>; \ + template class vespalib::hashtable<K, K, H, std::equal_to<>, std::_Identity<K>>; \ template class vespalib::Array<vespalib::hash_node<K>>; diff --git a/vespalib/src/vespa/vespalib/stllike/hashtable.h b/vespalib/src/vespa/vespalib/stllike/hashtable.h index 612c50ffb61..fa418cbad02 100644 --- a/vespalib/src/vespa/vespalib/stllike/hashtable.h +++ b/vespalib/src/vespa/vespalib/stllike/hashtable.h @@ -235,15 +235,12 @@ public: size_t capacity() const { return _nodes.capacity(); } size_t size() const { return _count; } bool empty() const { return _count == 0; } - template< typename AltKey, typename AltExtract, typename AltHash, typename AltEqual > - iterator find(const AltKey & key, const AltExtract & altExtract); - template< typename AltKey, typename AltExtract, typename AltHash, typename AltEqual > - iterator find(const AltKey & key) { return find<AltKey, AltExtract, AltHash, AltEqual>(key, AltExtract()); } + template< typename AltKey> + iterator find(const AltKey & key); iterator find(const Key & key); - template< typename AltKey, typename AltExtract, typename AltHash, typename AltEqual > - const_iterator find(const AltKey & key, const AltExtract & altExtract) const; - template< typename AltKey, typename AltExtract, typename AltHash, typename AltEqual > - const_iterator find(const AltKey & key) const { return find<AltKey, AltExtract, AltHash, AltEqual>(key, AltExtract()); } + + template< typename AltKey> + const_iterator find(const AltKey & key) const; const_iterator find(const Key & key) const; template <typename V> insert_result insert(V && node) { @@ -285,7 +282,8 @@ protected: const Value & getByInternalIndex(size_t index) const { return _nodes[index].getValue(); } template <typename MoveHandler> void erase(MoveHandler & moveHandler, next_t h, const const_iterator & key); - next_t hash(const Key & key) const { return modulator(_hasher(key)); } + template<typename K> + next_t hash(const K & key) const { return modulator(_hasher(key)); } private: Modulator _modulator; size_t _count; diff --git a/vespalib/src/vespa/vespalib/stllike/hashtable.hpp b/vespalib/src/vespa/vespalib/stllike/hashtable.hpp index 2fbe83eb226..60c391a0b7e 100644 --- a/vespalib/src/vespa/vespalib/stllike/hashtable.hpp +++ b/vespalib/src/vespa/vespalib/stllike/hashtable.hpp @@ -95,17 +95,15 @@ hashtable<Key, Value, Hash, Equal, KeyExtract, Modulator>::find(const Key & key) } template< typename Key, typename Value, typename Hash, typename Equal, typename KeyExtract, typename Modulator > -template< typename AltKey, typename AltExtract, typename AltHash, typename AltEqual> -typename hashtable<Key, Value, Hash, Equal, KeyExtract, Modulator>::const_iterator -hashtable<Key, Value, Hash, Equal, KeyExtract, Modulator>::find(const AltKey & key, const AltExtract & altExtract) const +template< typename AltKey> +typename hashtable<Key, Value, Hash, Equal, KeyExtract, Modulator>::iterator +hashtable<Key, Value, Hash, Equal, KeyExtract, Modulator>::find(const AltKey & key) { - AltHash altHasher; - next_t h = modulator(altHasher(key)); - if (_nodes[h].valid()) { - AltEqual altEqual; + next_t h = hash(key); + if (__builtin_expect(_nodes[h].valid(), true)) { do { - if (altEqual(altExtract(_keyExtractor(_nodes[h].getValue())), key)) { - return const_iterator(this, h); + if (__builtin_expect(_equal(_keyExtractor(_nodes[h].getValue()), key), true)) { + return iterator(this, h); } h = _nodes[h].getNext(); } while (h != Node::npos); @@ -114,17 +112,15 @@ hashtable<Key, Value, Hash, Equal, KeyExtract, Modulator>::find(const AltKey & k } template< typename Key, typename Value, typename Hash, typename Equal, typename KeyExtract, typename Modulator > -template< typename AltKey, typename AltExtract, typename AltHash, typename AltEqual> -typename hashtable<Key, Value, Hash, Equal, KeyExtract, Modulator>::iterator -hashtable<Key, Value, Hash, Equal, KeyExtract, Modulator>::find(const AltKey & key, const AltExtract & altExtract) +template< typename AltKey> +typename hashtable<Key, Value, Hash, Equal, KeyExtract, Modulator>::const_iterator +hashtable<Key, Value, Hash, Equal, KeyExtract, Modulator>::find(const AltKey & key) const { - AltHash altHasher; - next_t h = modulator(altHasher(key)); - if (_nodes[h].valid()) { - AltEqual altEqual; + next_t h = hash(key); + if (__builtin_expect(_nodes[h].valid(), true)) { do { - if (altEqual(altExtract(_keyExtractor(_nodes[h].getValue())), key)) { - return iterator(this, h); + if (__builtin_expect(_equal(_keyExtractor(_nodes[h].getValue()), key), true)) { + return const_iterator(this, h); } h = _nodes[h].getNext(); } while (h != Node::npos); |