diff options
105 files changed, 1814 insertions, 2460 deletions
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 b7482a3646d..91d5b7fe267 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 @@ -23,7 +23,6 @@ import com.yahoo.container.QrSearchersConfig; import com.yahoo.container.bundle.BundleInstantiationSpecification; import com.yahoo.container.core.ApplicationMetadataConfig; import com.yahoo.container.core.document.ContainerDocumentConfig; -import com.yahoo.container.core.identity.IdentityConfig; import com.yahoo.container.handler.ThreadPoolProvider; import com.yahoo.container.handler.ThreadpoolConfig; import com.yahoo.container.jdisc.ContainerMbusConfig; @@ -135,8 +134,7 @@ public final class ContainerCluster ServletPathsConfig.Producer, RoutingProviderConfig.Producer, ConfigserverConfig.Producer, - ThreadpoolConfig.Producer, - IdentityConfig.Producer + ThreadpoolConfig.Producer { /** @@ -188,8 +186,6 @@ public final class ContainerCluster private Optional<String> hostClusterId = Optional.empty(); private Optional<Integer> memoryPercentage = Optional.empty(); - private Identity identity; - private static class AcceptAllVerifier implements ContainerClusterVerifier { @Override public boolean acceptComponent(Component component) { return true; } @@ -863,16 +859,4 @@ public final class ContainerCluster this.containerCoreMemory = containerCoreMemory; } } - - public void setIdentity(Identity identity) { - this.identity = identity; - addComponent(identity); - } - - @Override - public void getConfig(IdentityConfig.Builder builder) { - if (identity != null) { - identity.getConfig(builder); - } - } } diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/Identity.java b/config-model/src/main/java/com/yahoo/vespa/model/container/Identity.java index ffa2bbb9c7b..81762f95a90 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/Identity.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/Identity.java @@ -2,18 +2,21 @@ package com.yahoo.vespa.model.container; import com.yahoo.container.core.identity.IdentityConfig; +import com.yahoo.container.jdisc.athenz.impl.AthenzIdentityProviderImpl; import com.yahoo.vespa.model.container.component.SimpleComponent; /** * @author mortent */ public class Identity extends SimpleComponent implements IdentityConfig.Producer { + public static final String CLASS = AthenzIdentityProviderImpl.class.getName(); + private final String domain; private final String service; private final String loadBalancerAddress; public Identity(String domain, String service, String loadBalancerAddress) { - super("com.yahoo.container.jdisc.athenz.impl.AthenzIdentityProviderImpl"); + super(CLASS); this.domain = domain; this.service = service; this.loadBalancerAddress = loadBalancerAddress; 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 f00e6e5e271..fb7ad137c22 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 @@ -701,7 +701,9 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> { .orElse(""); // How to test this? Identity identity = new Identity(domain.trim(), service.trim(), cfgHostName); - cluster.setIdentity(identity); + cluster.addComponent(identity); + + } } diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/IdentityBuilderTest.java b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/IdentityBuilderTest.java index 2f7b7f35b91..df118b0e349 100644 --- a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/IdentityBuilderTest.java +++ b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/IdentityBuilderTest.java @@ -3,6 +3,7 @@ package com.yahoo.vespa.model.container.xml; import com.yahoo.config.model.builder.xml.test.DomBuilderTest; import com.yahoo.container.core.identity.IdentityConfig; +import com.yahoo.vespa.model.container.Identity; import org.junit.Test; import org.w3c.dom.Element; import org.xml.sax.SAXException; @@ -26,7 +27,7 @@ public class IdentityBuilderTest extends ContainerModelBuilderTestBase { "</jdisc>"); createModel(root, clusterElem); - IdentityConfig identityConfig = root.getConfig(IdentityConfig.class, "default"); + IdentityConfig identityConfig = root.getConfig(IdentityConfig.class, "default/component/" + Identity.CLASS); assertEquals("domain", identityConfig.domain()); assertEquals("service", identityConfig.service()); } diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/Contacts.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/Contacts.java deleted file mode 100644 index 329483a85c5..00000000000 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/Contacts.java +++ /dev/null @@ -1,136 +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.hosted.controller.api.integration; - -import org.jetbrains.annotations.NotNull; - -import java.net.URI; -import java.util.Collection; -import java.util.Objects; - -import static com.yahoo.vespa.hosted.controller.api.integration.Contacts.Category.unknown; -import static java.util.Comparator.reverseOrder; - -/** - * @author jvenstad - */ -public interface Contacts { - - /** - * Returns the most relevant user of lowest non-empty level above that of @assignee, or, if no such user exists, - * the @assignee with @Category information. - */ - static UserContact escalationTargetFrom(Collection<UserContact> userContacts, String assignee) { - return userContacts.stream() - .filter(contact -> ! contact.username().isEmpty()) // don't assign to empty names - .sorted(reverseOrder()).distinct() // Pick out the highest category per user. - // Keep the assignee, or the last user on the first non-empty level above her. - .sorted().reduce(new UserContact(assignee, assignee, unknown), (current, next) -> - next.is(assignee) || (current.is(assignee) ^ current.level() == next.level()) ? next : current); - } - - /** - * Return a list of all contact entries for property with id @propertyId, where username is set. - */ - Collection<UserContact> userContactsFor(long propertyId); - - /** Returns the URL listing contacts for the given property */ - URI contactsUri(long propertyId); - - /** - * Return a target of escalation above @assignee, from the set of @UserContact entries found for @propertyId. - */ - default UserContact escalationTargetFor(long propertyId, String assignee) { - return escalationTargetFrom(userContactsFor(propertyId), assignee); - } - - /** - * A list of contact roles, in the order in which we look for escalation targets. - * Categories must be listed in increasing order of relevancy per level, and by increasing level. - */ - enum Category { - - unknown(-1, Level.none, "Unknown"), - admin(54, Level.grunt, "Administrator"), // TODO: Find more grunts? - businessOwner(567, Level.owner, "Business Owner"), - serviceOwner(646, Level.owner, "Service Engineering Owner"), - engineeringOwner(566, Level.owner, "Engineering Owner"), - vpBusiness(11, Level.VP, "VP Business"), - vpService(647, Level.VP, "VP Service Engineering"), - vpEngineering(9, Level.VP, "VP Engineering"); - - public final long id; - public final Level level; - public final String name; - - Category(long id, Level level, String name) { - this.id = id; - this.level = level; - this.name = name; - } - - /** Find the category for the given id, or unknown if the id is unknown. */ - public static Category of(Long id) { - for (Category category : values()) - if (category.id == id) - return category; - return unknown; - } - - public enum Level { - none, - grunt, - owner, - VP; - } - - } - - /** Container class for user contact information; sorts by category and identifies by username. Immutable. */ - class UserContact implements Comparable<UserContact> { - - private final String username; - private final String name; - private final Category category; - - public UserContact(String username, String name, Category category) { - Objects.requireNonNull(username, "username cannot be null"); - Objects.requireNonNull(name, "name cannot be null"); - Objects.requireNonNull(category, "category cannot be null"); - this.username = username; - this.name = name; - this.category = category; - } - - public String username() { return username; } - public String name() { return name; } - public Category category() { return category; } - public Category.Level level() { return category.level; } - - public boolean is(String username) { return this.username.equals(username); } - - @Override - public boolean equals(Object o) { - if (this == o) return true; - if (o == null || getClass() != o.getClass()) return false; - UserContact that = (UserContact) o; - return Objects.equals(username, that.username); - } - - @Override - public int hashCode() { - return Objects.hash(username); - } - - @Override - public int compareTo(@NotNull UserContact other) { - return category().compareTo(other.category()); - } - - @Override - public String toString() { - return String.format("%s, %s, %s", username, name, category.name); - } - - } - -} diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/Issues.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/Issues.java deleted file mode 100644 index 8f24b4e3ede..00000000000 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/Issues.java +++ /dev/null @@ -1,186 +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.hosted.controller.api.integration; - -import java.time.Instant; -import java.util.List; -import java.util.Optional; - -/** - * @author jvenstad - */ -public interface Issues { - - /** - * Returns information about an issue. - * If this issue does not exist this returns an issue id containing the id and default values. - */ - IssueInfo fetch(String issueId); - - /** - * Returns the @Meta of all unresolved issues which have the same summary (and queue, if present) as @issue. - */ - List<IssueInfo> fetchSimilarTo(Issue issue); - - /** - * Files the given issue - * - * @return the id of the created issue - */ - String file(Issue issue); - - /** - * Update the description fields of the issue stored with id @issueId to be @description. - */ - void update(String issueId, String description); - - /** - * Set the assignee of the issue with id @issueId to the user with usename @assignee. - */ - void reassign(String issueId, String assignee); - - /** - * Add the user with username @watcher to the watcher list of the issue with id @issueId. - */ - void addWatcher(String issueId, String watcher); - - /** - * Post @comment as a comment to the issue with id @issueId. - */ - void comment(String issueId, String comment); - - - /** Contains information used to file an issue with the responsible party; only @queue is mandatory. */ - class Classification { - - private final String queue; - private final String component; - private final String label; - private final String assignee; - - public Classification(String queue, String component, String label, String assignee) { - if (queue.isEmpty()) throw new IllegalArgumentException("Queue can not be empty!"); - - this.queue = queue; - this.component = component; - this.label = label; - this.assignee = assignee; - } - - public Classification(String queue) { - this(queue, null, null, null); - } - - public Classification withComponent(String component) { return new Classification(queue, component, label, assignee); } - public Classification withLabel(String label) { return new Classification(queue, component, label, assignee); } - public Classification withAssignee(String assignee) { return new Classification(queue, component, label, assignee); } - - public String queue() { return queue; } - public Optional<String> component() { return Optional.ofNullable(component); } - public Optional<String> label() { return Optional.ofNullable(label); } - public Optional<String> assignee() { return Optional.ofNullable(assignee); } - - @Override - public String toString() { - return "Queue : " + queue() + "\n" + - "Component : " + component() + "\n" + - "Label : " + label() + "\n" + - "Assignee : " + assignee() + "\n"; - } - - } - - - /** Information about a stored issue */ - class IssueInfo { - - private final String id; - private final String key; - private final Instant updated; - private final Optional<String> assignee; - private final Status status; - - public IssueInfo(String id, String key, Instant updated, Optional<String> assignee, Status status) { - if (assignee == null || assignee.isPresent() && assignee.get().isEmpty()) // TODO: Throw on these things - assignee = Optional.empty(); - this.id = id; - this.key = key; - this.updated = updated; - this.assignee = assignee; - this.status = status; - } - - public IssueInfo withAssignee(Optional<String> assignee) { - return new IssueInfo(id, key, updated, assignee, status); - } - - public String id() { return id; } - public String key() { return key; } - public Instant updated() { return updated; } - public Optional<String> assignee() { return assignee; } - public Status status() { return status; } - - public enum Status { - - toDo("To Do"), - inProgress("In Progress"), - done("Done"), - noCategory("No Category"); - - private final String value; - - Status(String value) { this.value = value; } - - public static Status fromValue(String value) { - for (Status status : Status.values()) - if (status.value.equals(value)) - return status; - throw new IllegalArgumentException(value + " is not a valid status."); - } - - } - - } - - - /** - * A representation of an issue with a Vespa application which can be reported and escalated through an external issue service. - * This class is immutable. - * - * @author jvenstad - */ - class Issue { - - private final String summary; - private final String description; - private final Classification classification; - - public Issue(String summary, String description, Classification classification) { - if (summary.isEmpty()) throw new IllegalArgumentException("Summary can not be empty."); - if (description.isEmpty()) throw new IllegalArgumentException("Description can not be empty."); - - this.summary = summary; - this.description = description; - this.classification = classification; - } - - public Issue(String summary, String description) { - this(summary, description, null); - } - - public Issue with(Classification classification) { - return new Issue(summary, description, classification); - } - public Issue withDescription(String description) { return new Issue(summary, description, classification); } - - /** Return a new @Issue with the description of @this, but with @appendage appended. */ - public Issue append(String appendage) { - return new Issue(summary, description + "\n\n" + appendage, classification); - } - - public String summary() { return summary; } - public String description() { return description; } - public Optional<Classification> classification() { return Optional.ofNullable(classification); } - - } - -} diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/Properties.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/Properties.java deleted file mode 100644 index 652b5495bc5..00000000000 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/Properties.java +++ /dev/null @@ -1,16 +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.hosted.controller.api.integration; - -import java.util.Optional; - -/** - * @author jvenstad - */ -public interface Properties { - - /** - * Return the @Issues.Classification listed for the property with id @propertyId. - */ - Optional<Issues.Classification> classificationFor(long propertyId); - -} diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/jira/JiraMock.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/jira/JiraMock.java deleted file mode 100644 index da653ddd8a8..00000000000 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/jira/JiraMock.java +++ /dev/null @@ -1,50 +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.hosted.controller.api.integration.jira; - -import java.time.Instant; -import java.util.HashMap; -import java.util.List; -import java.util.Map; -import java.util.stream.Collectors; - -/** - * @author jvenstad - */ -// TODO: Make mock. -public class JiraMock implements Jira { - - public final Map<String, JiraCreateIssue.JiraFields> issues = new HashMap<>(); - - private Long counter = 0L; - - @Override - public List<JiraIssue> searchByProjectAndSummary(String project, String summary) { - return issues.entrySet().stream() - .filter(entry -> entry.getValue().project.key.equals(project)) - .filter(entry -> entry.getValue().summary.contains(summary)) - .map(entry -> new JiraIssue(entry.getKey(), new JiraIssue.Fields(Instant.now()))) - .collect(Collectors.toList()); - } - - @Override - public JiraIssue createIssue(JiraCreateIssue issueData) { - JiraIssue issue = uniqueKey(); - issues.put(issue.key, issueData.fields); - return issue; - } - - @Override - public void commentIssue(JiraIssue issue, JiraComment comment) { - // Add mock when relevant. - } - - @Override - public void addAttachment(JiraIssue issue, String filename, String fileContent) { - // Add mock when relevant. - } - - private JiraIssue uniqueKey() { - return new JiraIssue((++counter).toString(), new JiraIssue.Fields(Instant.now())); - } - -} diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/organization/DeploymentIssues.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/organization/DeploymentIssues.java new file mode 100644 index 00000000000..7874fcd8c45 --- /dev/null +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/organization/DeploymentIssues.java @@ -0,0 +1,26 @@ +// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.vespa.hosted.controller.api.integration.organization; + +import com.yahoo.config.provision.ApplicationId; +import com.yahoo.vespa.hosted.controller.api.identifiers.PropertyId; + +import java.time.Duration; +import java.util.Collection; +import java.util.Optional; + +/** + * Represents the people responsible for keeping Vespa up and running in a given organization, etc.. + * + * @author jvenstad + */ +public interface DeploymentIssues { + + IssueId fileUnlessOpen(Optional<IssueId> issueId, ApplicationId applicationId, PropertyId propertyId); + + IssueId fileUnlessOpen(Optional<IssueId> issueId, ApplicationId applicationId, User assignee); + + IssueId fileUnlessOpen(Collection<ApplicationId> applicationIds); + + void escalateIfInactive(IssueId issueId, Optional<PropertyId> propertyId, Duration maxInactivity); + +} diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/organization/Issue.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/organization/Issue.java new file mode 100644 index 00000000000..df182b56fd8 --- /dev/null +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/organization/Issue.java @@ -0,0 +1,75 @@ +// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.vespa.hosted.controller.api.integration.organization; + +import com.yahoo.vespa.hosted.controller.api.identifiers.PropertyId; + +import java.util.Objects; +import java.util.Optional; + +/** + * Represents an issue which needs to reported, typically from the controller, to a responsible party, + * the identity of which is determined by the propertyId and, possibly, assignee fields. + * + * @author jvenstad + */ +public class Issue { + + private final String summary; + private final String description; + private final String label; + private final User assignee; + private final PropertyId propertyId; + + private Issue(String summary, String description, String label, User assignee, PropertyId propertyId) { + if (summary.isEmpty()) throw new IllegalArgumentException("Issue summary can not be empty!"); + if (description.isEmpty()) throw new IllegalArgumentException("Issue description can not be empty!"); + Objects.requireNonNull(propertyId, "An issue must belong to a property!"); + + this.summary = summary; + this.description = description; + this.label = label; + this.assignee = assignee; + this.propertyId = propertyId; + } + + public Issue(String summary, String description, PropertyId propertyId) { + this(summary, description, null, null, propertyId); + } + + public Issue append(String appendage) { + return new Issue(summary, description + appendage, label, assignee, propertyId); + } + + public Issue withLabel(String label) { + return new Issue(summary, description, label, assignee, propertyId); + } + + public Issue withAssignee(User assignee) { + return new Issue(summary, description, label, assignee, propertyId); + } + + public Issue withPropertyId(PropertyId propertyId) { + return new Issue(summary, description, label, assignee, propertyId); + } + + public String summary() { + return summary; + } + + public String description() { + return description; + } + + public Optional<String> label() { + return Optional.ofNullable(label); + } + + public Optional<User> assignee() { + return Optional.ofNullable(assignee); + } + + public PropertyId propertyId() { + return propertyId; + } + +} diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/organization/IssueId.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/organization/IssueId.java new file mode 100644 index 00000000000..84b441ff4a8 --- /dev/null +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/organization/IssueId.java @@ -0,0 +1,49 @@ +// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.vespa.hosted.controller.api.integration.organization; + +import java.util.Objects; + +/** + * Used to identify issues stored in some issue tracking system. + * The {@code value()} and {@code from()} methods should be inverses. + * + * @author jvenstad + */ +public class IssueId { + + protected final String id; + + protected IssueId(String id) { + this.id = id; + } + + public static IssueId from(String value) { + if (value.isEmpty()) + throw new IllegalArgumentException("Can not make an IssueId from an empty value."); + + return new IssueId(value); + } + + public String value() { + return id; + } + + @Override + public String toString() { + return value(); + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + IssueId issueId = (IssueId) o; + return Objects.equals(id, issueId.id); + } + + @Override + public int hashCode() { + return Objects.hash(id); + } + +} diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/organization/MockOrganization.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/organization/MockOrganization.java new file mode 100644 index 00000000000..4eaca8fb642 --- /dev/null +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/organization/MockOrganization.java @@ -0,0 +1,154 @@ +// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.vespa.hosted.controller.api.integration.organization; + +import com.google.inject.Inject; +import com.yahoo.vespa.hosted.controller.api.identifiers.PropertyId; + +import java.net.URI; +import java.time.Clock; +import java.time.Duration; +import java.time.Instant; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.concurrent.atomic.AtomicLong; + +public class MockOrganization implements Organization { + + private final Clock clock; + private final AtomicLong counter; + private final HashMap<IssueId, WrappedIssue> issues; + private final HashMap<PropertyId, PropertyInfo> properties; + + @Inject + @SuppressWarnings("unused") + public MockOrganization() { + this(Clock.systemUTC()); + } + + public MockOrganization(Clock clock) { + this.clock = clock; + + counter = new AtomicLong(); + issues = new HashMap<>(); + properties = new HashMap<>(); + } + + @Override + public IssueId file(Issue issue) { + IssueId issueId = IssueId.from("" + counter.incrementAndGet()); + issues.put(issueId, new WrappedIssue(issue)); + return issueId; + } + + @Override + public Optional<IssueId> findBySimilarity(Issue issue) { + return issues.entrySet().stream() + .filter(entry -> entry.getValue().issue.summary().equals(issue.summary())) + .findFirst() + .map(Map.Entry::getKey); + } + + @Override + public void update(IssueId issueId, String description) { + touch(issueId); + } + + @Override + public void commentOn(IssueId issueId, String comment) { + touch(issueId); + } + + @Override + public boolean isOpen(IssueId issueId) { + return issues.get(issueId).open; + } + + @Override + public boolean isActive(IssueId issueId, Duration maxInactivity) { + return issues.get(issueId).updated.isAfter(clock.instant().minus(maxInactivity)); + } + + @Override + public Optional<User> assigneeOf(IssueId issueId) { + return Optional.ofNullable(issues.get(issueId).assignee); + } + + @Override + public boolean reassign(IssueId issueId, User assignee) { + issues.get(issueId).assignee = assignee; + touch(issueId); + return true; + } + + @Override + public List<? extends List<? extends User>> contactsFor(PropertyId propertyId) { + return properties.get(propertyId).contacts; + } + + @Override + public URI issueCreationUri(PropertyId propertyId) { + return URI.create("www.issues.tld/" + propertyId.id()); + } + + @Override + public URI contactsUri(PropertyId propertyId) { + return URI.create("www.contacts.tld/" + propertyId.id()); + } + + @Override + public URI propertyUri(PropertyId propertyId) { + return URI.create("www.properties.tld/" + propertyId.id()); + } + + public void close(IssueId issueId) { + issues.get(issueId).open = false; + touch(issueId); + } + + public void setDefaultAssigneeFor(PropertyId propertyId, User defaultAssignee) { + properties.get(propertyId).defaultAssignee = defaultAssignee; + } + + public void setContactsFor(PropertyId propertyId, List<List<User>> contacts) { + properties.get(propertyId).contacts = contacts; + } + + public void addProperty(PropertyId propertyId) { + properties.put(propertyId, new PropertyInfo()); + } + + private void touch(IssueId issueId) { + issues.get(issueId).updated = clock.instant(); + } + + + private class WrappedIssue { + + private Issue issue; + private Instant updated; + private boolean open; + private User assignee; + + private WrappedIssue(Issue issue) { + this.issue = issue; + + updated = clock.instant(); + open = true; + assignee = issue.assignee().orElse(properties.get(issue.propertyId()).defaultAssignee); + } + + } + + + private class PropertyInfo { + + private User defaultAssignee; + private List<List<User>> contacts = Collections.emptyList(); + + } + +} + diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/organization/Organization.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/organization/Organization.java new file mode 100644 index 00000000000..00c0d87554a --- /dev/null +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/organization/Organization.java @@ -0,0 +1,124 @@ +// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.vespa.hosted.controller.api.integration.organization; + +import com.yahoo.vespa.hosted.controller.api.identifiers.PropertyId; + +import java.io.UncheckedIOException; +import java.net.URI; +import java.time.Duration; +import java.util.List; +import java.util.Optional; + +/** + * Represents the humans who use this software, and their organization. + * Lets the software report issues to its caretakers, and provides other useful human resource lookups. + * + * @author jvenstad + */ +public interface Organization { + + /** + * File an issue with its given property or the default, and with the specific assignee, if present. + * + * @param issue The issue to file. + * @return ID of the created issue. + */ + IssueId file(Issue issue); + + /** + * Returns the ID of this issue, if it exists and is open, based on a similarity search. + * + * @param issue The issue to search for; relevant fields are the summary and the owner (propertyId). + * @return ID of the issue, if it is found. + */ + Optional<IssueId> findBySimilarity(Issue issue); + + /** + * Update the description of the issue with the given ID. + * + * @param issueId ID of the issue to comment on. + * @param description The updated description. + */ + void update(IssueId issueId, String description); + + /** + * Add a comment to the issue with the given ID. + * + * @param issueId ID of the issue to comment on. + * @param comment The comment to add. + */ + void commentOn(IssueId issueId, String comment); + + /** + * Returns whether the issue is still under investigation. + * + * @param issueId ID of the issue to examine. + * @return Whether the given issue is under investigation. + */ + boolean isOpen(IssueId issueId); + + /** + * Returns whether there has been significant activity on the issue within the given duration. + * + * @param issueId ID of the issue to examine. + * @return Whether the given issue is actively worked on. + */ + boolean isActive(IssueId issueId, Duration maxInactivity); + + /** + * Returns the user assigned to the given issue, if any. + * + * @param issueId ID of the issue for which to find the assignee. + * @return The user responsible for fixing the given issue, if found. + */ + Optional<User> assigneeOf(IssueId issueId); + + /** + * Reassign the issue with the given ID to the given user, and returns the outcome of this. + * + * @param issueId ID of the issue to be reassigned. + * @param assignee User to which the issue shall be assigned. + * @return Whether the reassignment was successful. + */ + boolean reassign(IssueId issueId, User assignee); + + /** + * Escalate an issue filed with the given property. + * + * @param issueId ID of the issue to escalate. + * @param propertyId PropertyId of the tenant owning the application for which the issue was filed. + */ + default boolean escalate(IssueId issueId, PropertyId propertyId) { + List<? extends List<? extends User>> contacts = contactsFor(propertyId); + + Optional<User> assignee = assigneeOf(issueId); + int assigneeLevel = -1; + if (assignee.isPresent()) + for (int level = contacts.size(); --level > assigneeLevel; ) + if (contacts.get(level).contains(assignee.get())) + assigneeLevel = level; + + for (int level = assigneeLevel + 1; level < contacts.size(); level++) + for (User target : contacts.get(level)) + if (reassign(issueId, target)) + return true; + + return false; + } + + /** + * Returns a nested list where the entries have increasing rank, and where each entry is + * a list of the users of that rank, by decreasing relevance. + * + * @param propertyId ID of the property for which to list contacts. + * @return A sorted, nested, reverse sorted list of contacts. + */ + List<? extends List<? extends User>> contactsFor(PropertyId propertyId); + + URI issueCreationUri(PropertyId propertyId); + + URI contactsUri(PropertyId propertyId); + + URI propertyUri(PropertyId propertyId); + +} diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/organization/User.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/organization/User.java new file mode 100644 index 00000000000..82a86de3824 --- /dev/null +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/organization/User.java @@ -0,0 +1,52 @@ +// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.vespa.hosted.controller.api.integration.organization; + +import java.util.Objects; + +/** + * Represents a human computer user, typically by UNIX account name. + * + * @author jvenstad + */ +public class User { + + private final String username; + + protected User(String username) { + this.username = username; + } + + public String username() { + return username; + } + + public String displayName() { + return username; + } + + public static User from(String username) { + if (username.isEmpty()) + throw new IllegalArgumentException("Username may not be empty!"); + + return new User(username); + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if ( ! (o instanceof User)) return false; + User that = (User) o; + return Objects.equals(username, that.username); + } + + @Override + public int hashCode() { + return Objects.hash(username); + } + + @Override + public String toString() { + return username(); + } + +} diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/stubs/ContactsMock.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/stubs/ContactsMock.java deleted file mode 100644 index 9114cf20ccc..00000000000 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/stubs/ContactsMock.java +++ /dev/null @@ -1,31 +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.hosted.controller.api.integration.stubs; - -import com.yahoo.vespa.hosted.controller.api.integration.Contacts; - -import java.net.URI; -import java.util.HashMap; -import java.util.List; -import java.util.Map; - -/** - * @author mpolden - */ -public class ContactsMock implements Contacts { - - private final Map<Long, List<UserContact>> userContacts = new HashMap<>(); - - public void addContact(long propertyId, List<UserContact> contacts) { - userContacts.put(propertyId, contacts); - } - - public List<UserContact> userContactsFor(long propertyId) { - return userContacts.get(propertyId); - } - - @Override - public URI contactsUri(long propertyId) { - return URI.create("http://contacts.test?propertyId=" + propertyId); - } - -} diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/stubs/LoggingDeploymentIssues.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/stubs/LoggingDeploymentIssues.java new file mode 100644 index 00000000000..62dde3efe55 --- /dev/null +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/stubs/LoggingDeploymentIssues.java @@ -0,0 +1,96 @@ +// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. + +package com.yahoo.vespa.hosted.controller.api.integration.stubs; + +import com.google.inject.Inject; +import com.yahoo.config.provision.ApplicationId; +import com.yahoo.vespa.hosted.controller.api.identifiers.PropertyId; +import com.yahoo.vespa.hosted.controller.api.integration.organization.DeploymentIssues; +import com.yahoo.vespa.hosted.controller.api.integration.organization.IssueId; +import com.yahoo.vespa.hosted.controller.api.integration.organization.User; +import org.jetbrains.annotations.TestOnly; + +import java.time.Clock; +import java.time.Duration; +import java.time.Instant; +import java.util.Collection; +import java.util.HashMap; +import java.util.Map; +import java.util.Optional; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicLong; +import java.util.logging.Logger; + +/** + * A memory backed implementation of the Issues API which logs changes and does nothing else. + * + * @author bratseth, jvenstad + */ +public class LoggingDeploymentIssues implements DeploymentIssues { + + private static final Logger log = Logger.getLogger(LoggingDeploymentIssues.class.getName()); + + /** Whether the platform is currently broken. */ + protected final AtomicBoolean platformIssue = new AtomicBoolean(false); + /** Last updates for each issue -- used to determine if issues are already logged and when to escalate. */ + protected final Map<IssueId, Instant> issueUpdates = new HashMap<>(); + + /** Used to fabricate unique issue ids. */ + private final AtomicLong issueIdSequence = new AtomicLong(0); + + private final Clock clock; + + @SuppressWarnings("unused") // Created by dependency injection. + @Inject + public LoggingDeploymentIssues() { + this(Clock.systemUTC()); + } + + @TestOnly + protected LoggingDeploymentIssues(Clock clock) { + this.clock = clock; + } + + @Override + public IssueId fileUnlessOpen(Optional<IssueId> issueId, ApplicationId applicationId, PropertyId propertyId) { + return fileUnlessPresent(issueId, applicationId); + } + + @Override + public IssueId fileUnlessOpen(Optional<IssueId> issueId, ApplicationId applicationId, User assignee) { + return fileUnlessPresent(issueId, applicationId); + } + + @Override + public IssueId fileUnlessOpen(Collection<ApplicationId> applicationIds) { + if ( ! platformIssue.get()) + log.info("These applications are all failing deployment:\n" + applicationIds); + + platformIssue.set(true); + return null; + } + + @Override + public void escalateIfInactive(IssueId issueId, Optional<PropertyId> propertyId, Duration maxInactivity) { + if (issueUpdates.containsKey(issueId) && issueUpdates.get(issueId).isBefore(clock.instant().minus(maxInactivity))) + escalateIssue(issueId); + } + + protected void escalateIssue(IssueId issueId) { + issueUpdates.put(issueId, clock.instant()); + log.info("Deployment issue " + issueId + " should be escalated."); + } + + protected IssueId fileIssue(ApplicationId applicationId) { + IssueId issueId = IssueId.from("" + issueIdSequence.incrementAndGet()); + issueUpdates.put(issueId, clock.instant()); + log.info("Deployment issue " + issueId +": " + applicationId + " has failing deployments."); + return issueId; + } + + private IssueId fileUnlessPresent(Optional<IssueId> issueId, ApplicationId applicationId) { + platformIssue.set(false); + return issueId.filter(issueUpdates::containsKey).orElseGet(() -> fileIssue(applicationId)); + } + +} diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/stubs/LoggingIssues.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/stubs/LoggingIssues.java deleted file mode 100644 index 4801f551307..00000000000 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/stubs/LoggingIssues.java +++ /dev/null @@ -1,95 +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.hosted.controller.api.integration.stubs; - -import com.yahoo.vespa.hosted.controller.api.integration.Issues; - -import java.time.Instant; -import java.util.Collections; -import java.util.HashMap; -import java.util.List; -import java.util.Map; -import java.util.Optional; -import java.util.concurrent.atomic.AtomicLong; -import java.util.logging.Logger; - -/** - * An memory backed implementation of the Issues API which logs changes and does nothing else. - * - * @author bratseth - */ -@SuppressWarnings("unused") // created by dependency injection -public class LoggingIssues implements Issues { - - private static final Logger log = Logger.getLogger(LoggingIssues.class.getName()); - - /** Used to fabricate unique issue ids */ - private AtomicLong issueIdSequence = new AtomicLong(0); - - // These two maps should have precisely the same keys - private final Map<String, Issue> issues = new HashMap<>(); - private final Map<String, IssueInfo> issueInfos = new HashMap<>(); - - @Override - public IssueInfo fetch(String issueId) { - return issueInfos.getOrDefault(issueId, - new IssueInfo(issueId, null, Instant.ofEpochMilli(0), null, IssueInfo.Status.noCategory)); - } - - @Override - public List<IssueInfo> fetchSimilarTo(Issue issue) { - return Collections.emptyList(); - } - - @Override - public String file(Issue issue) { - log.info("Want to file " + issue); - String issueId = "issue-" + issueIdSequence.getAndIncrement(); - file(issueId, issue); - return issueId; - } - - private IssueInfo file(String issueId, Issue issue) { - IssueInfo issueInfo = new IssueInfo(issueId, null, Instant.now(), null, IssueInfo.Status.noCategory); - issues.put(issueId, issue); - issueInfos.put(issueId, issueInfo); - return issueInfo; - } - - @Override - public void update(String issueId, String description) { - log.info("Want to update " + issueId); - issues.put(issueId, requireIssue(issueId).withDescription(description)); - } - - @Override - public void reassign(String issueId, String assignee) { - log.info("Want to reassign issue " + issueId + " to " + assignee); - issueInfos.put(issueId, requireInfo(issueId).withAssignee(Optional.of(assignee))); - } - - @Override - public void addWatcher(String issueId, String watcher) { - log.info("Want to add watcher " + watcher + " to issue " + issueId); - } - - @Override - public void comment(String issueId, String comment) { - log.info("Want to comment on issue " + issueId); - } - - private Issue requireIssue(String issueId) { - Issue issue = issues.get(issueId); - if (issue == null) - throw new IllegalArgumentException("No issue with id '" + issueId + "'"); - return issue; - } - - private IssueInfo requireInfo(String issueId) { - IssueInfo info = issueInfos.get(issueId); - if (info != null) // we still remember this issue - return info; - else // we forgot this issue (due to restart) - recreate it here to avoid log noise - return file(issueId, new Issue("(Forgotten)", "(Forgotten)")); - } - -} diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/stubs/PropertiesMock.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/stubs/PropertiesMock.java deleted file mode 100644 index 53a31933e03..00000000000 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/stubs/PropertiesMock.java +++ /dev/null @@ -1,26 +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.hosted.controller.api.integration.stubs; - -import com.yahoo.vespa.hosted.controller.api.integration.Issues; -import com.yahoo.vespa.hosted.controller.api.integration.Properties; - -import java.util.HashMap; -import java.util.Map; -import java.util.Optional; - -/** - * @author mpolden - */ -public class PropertiesMock implements Properties { - - private final Map<Long, Issues.Classification> projects = new HashMap<>(); - - public void addClassification(long propertyId, String classification) { - projects.put(propertyId, new Issues.Classification(classification)); - } - - public Optional<Issues.Classification> classificationFor(long propertyId) { - return Optional.ofNullable(projects.get(propertyId)); - } - -} diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Application.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Application.java index 2066b98aeb9..6f5116a1825 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Application.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Application.java @@ -8,6 +8,7 @@ import com.yahoo.config.application.api.ValidationOverrides; import com.yahoo.config.provision.ApplicationId; import com.yahoo.config.provision.Environment; import com.yahoo.config.provision.Zone; +import com.yahoo.vespa.hosted.controller.api.integration.organization.IssueId; import com.yahoo.vespa.hosted.controller.application.ApplicationRevision; import com.yahoo.vespa.hosted.controller.application.Change; import com.yahoo.vespa.hosted.controller.application.Deployment; @@ -140,8 +141,8 @@ public class Application { return new Application(id, deploymentSpec, validationOverrides, deployments, deploymentJobs.withProjectId(projectId), deploying, outstandingChange); } - public Application withJiraIssueId(Optional<String> jiraIssueId) { - return new Application(id, deploymentSpec, validationOverrides, deployments, deploymentJobs.withJiraIssueId(jiraIssueId), deploying, outstandingChange); + public Application with(IssueId issueId) { + return new Application(id, deploymentSpec, validationOverrides, deployments, deploymentJobs.with(issueId), deploying, outstandingChange); } public Application withJobCompletion(JobReport report, Instant notificationTime, Controller controller) { diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java index 1bb78078cd5..1ff6802f0ab 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java @@ -22,6 +22,7 @@ import com.yahoo.vespa.hosted.controller.api.identifiers.DeploymentId; import com.yahoo.vespa.hosted.controller.api.identifiers.Hostname; import com.yahoo.vespa.hosted.controller.api.identifiers.RevisionId; import com.yahoo.vespa.hosted.controller.api.identifiers.TenantId; +import com.yahoo.vespa.hosted.controller.api.integration.organization.IssueId; import com.yahoo.vespa.hosted.controller.api.integration.configserver.ConfigServerClient; import com.yahoo.vespa.hosted.controller.api.integration.configserver.Log; import com.yahoo.vespa.hosted.controller.api.integration.configserver.NoInstanceException; @@ -499,9 +500,9 @@ public class ApplicationController { } } - public void setJiraIssueId(ApplicationId id, Optional<String> jiraIssueId) { + public void setIssueId(ApplicationId id, IssueId issueId) { try (Lock lock = lock(id)) { - get(id).ifPresent(application -> store(application.withJiraIssueId(jiraIssueId), lock)); + get(id).ifPresent(application -> store(application.with(issueId), lock)); } } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Controller.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Controller.java index 3b87af5c95a..660013daa62 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Controller.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Controller.java @@ -20,7 +20,7 @@ import com.yahoo.vespa.hosted.controller.api.integration.configserver.ConfigServ import com.yahoo.vespa.hosted.controller.api.integration.dns.NameService; import com.yahoo.vespa.hosted.controller.api.integration.entity.EntityService; import com.yahoo.vespa.hosted.controller.api.integration.github.GitHub; -import com.yahoo.vespa.hosted.controller.api.integration.jira.Jira; +import com.yahoo.vespa.hosted.controller.api.integration.organization.Organization; import com.yahoo.vespa.hosted.controller.api.integration.routing.GlobalRoutingService; import com.yahoo.vespa.hosted.controller.api.integration.routing.RotationStatus; import com.yahoo.vespa.hosted.controller.api.integration.routing.RoutingGenerator; @@ -72,6 +72,7 @@ public class Controller extends AbstractComponent { private final ConfigServerClient configServerClient; private final MetricsService metricsService; private final Chef chefClient; + private final Organization organization; private final AthenzClientFactory athenzClientFactory; /** @@ -82,19 +83,19 @@ public class Controller extends AbstractComponent { */ @Inject public Controller(ControllerDb db, CuratorDb curator, RotationRepository rotationRepository, - GitHub gitHub, Jira jiraClient, EntityService entityService, + GitHub gitHub, EntityService entityService, Organization organization, GlobalRoutingService globalRoutingService, ZoneRegistry zoneRegistry, ConfigServerClient configServerClient, MetricsService metricsService, NameService nameService, RoutingGenerator routingGenerator, Chef chefClient, AthenzClientFactory athenzClientFactory) { this(db, curator, rotationRepository, - gitHub, jiraClient, entityService, globalRoutingService, zoneRegistry, + gitHub, entityService, organization, globalRoutingService, zoneRegistry, configServerClient, metricsService, nameService, routingGenerator, chefClient, Clock.systemUTC(), athenzClientFactory); } public Controller(ControllerDb db, CuratorDb curator, RotationRepository rotationRepository, - GitHub gitHub, Jira jiraClient, EntityService entityService, + GitHub gitHub, EntityService entityService, Organization organization, GlobalRoutingService globalRoutingService, ZoneRegistry zoneRegistry, ConfigServerClient configServerClient, MetricsService metricsService, NameService nameService, @@ -104,8 +105,8 @@ public class Controller extends AbstractComponent { Objects.requireNonNull(curator, "Curator cannot be null"); Objects.requireNonNull(rotationRepository, "Rotation repository cannot be null"); Objects.requireNonNull(gitHub, "GitHubClient cannot be null"); - Objects.requireNonNull(jiraClient, "JiraClient cannot be null"); Objects.requireNonNull(entityService, "EntityService cannot be null"); + Objects.requireNonNull(organization, "Organization cannot be null"); Objects.requireNonNull(globalRoutingService, "GlobalRoutingService cannot be null"); Objects.requireNonNull(zoneRegistry, "ZoneRegistry cannot be null"); Objects.requireNonNull(configServerClient, "ConfigServerClient cannot be null"); @@ -120,6 +121,7 @@ public class Controller extends AbstractComponent { this.curator = curator; this.gitHub = gitHub; this.entityService = entityService; + this.organization = organization; this.globalRoutingService = globalRoutingService; this.zoneRegistry = zoneRegistry; this.configServerClient = configServerClient; @@ -240,7 +242,13 @@ public class Controller extends AbstractComponent { return chefClient; } - public CuratorDb curator() { return curator; } + public Organization organization() { + return organization; + } + + public CuratorDb curator() { + return curator; + } private String printableVersion(Optional<VespaVersion> vespaVersion) { return vespaVersion.map(v -> v.versionNumber().toFullString()).orElse("Unknown"); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/api/Tenant.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/api/Tenant.java index 4889f789819..9b8643c7167 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/api/Tenant.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/api/Tenant.java @@ -29,24 +29,25 @@ public class Tenant { public Tenant(TenantId id, Optional<UserGroup> userGroup, Optional<Property> property, Optional<AthenzDomain> athenzDomain, Optional<PropertyId> propertyId) { if (id.isUser()) { - require(!userGroup.isPresent(), "User tenant '%s' cannot have a user group.", id); - require(!property.isPresent(), "User tenant '%s' cannot have a property.", id); - require(!propertyId.isPresent(), "User tenant '%s' cannot have a property ID.", id); - require(!athenzDomain.isPresent(), "User tenant '%s' cannot have an athens domain.", id); + require( ! userGroup.isPresent(), "User tenant '%s' cannot have a user group.", id); + require( ! property.isPresent(), "User tenant '%s' cannot have a property.", id); + require( ! propertyId.isPresent(), "User tenant '%s' cannot have a property ID.", id); + require( ! athenzDomain.isPresent(), "User tenant '%s' cannot have an athens domain.", id); } else if (athenzDomain.isPresent()) { - require(property.isPresent(), "Athens tenant '%s' must have a property.", id); - require(!userGroup.isPresent(), "Athens tenant '%s' cannot have a user group.", id); - require(athenzDomain.isPresent(), "Athens tenant '%s' must have an athens domain.", id); + require( property.isPresent(), "Athens tenant '%s' must have a property.", id); + require( ! userGroup.isPresent(), "Athens tenant '%s' cannot have a user group.", id); + require( athenzDomain.isPresent(), "Athens tenant '%s' must have an athens domain.", id); } else { - require(property.isPresent(), "OpsDB tenant '%s' must have a property.", id); - require(userGroup.isPresent(), "OpsDb tenant '%s' must have a user group.", id); - require(!athenzDomain.isPresent(), "OpsDb tenant '%s' cannot have an athens domain.", id); + require( property.isPresent(), "OpsDB tenant '%s' must have a property.", id); + require( userGroup.isPresent(), "OpsDb tenant '%s' must have a user group.", id); + require( ! athenzDomain.isPresent(), "OpsDb tenant '%s' cannot have an athens domain.", id); } this.id = id; this.userGroup = userGroup; this.property = property; this.athenzDomain = athenzDomain; this.propertyId = propertyId; // TODO: Check validity after TODO@14. OpsDb tenants have this set in Sherpa, while athens tenants do not. + // TODO: Require PropertyId for non-users, and fetch Property from EntityService (which will be moved to Organization) in the controller. } public boolean isAthensTenant() { return athenzDomain.isPresent(); } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/DeploymentJobs.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/DeploymentJobs.java index 1ffa06bb624..68e0ce39c5c 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/DeploymentJobs.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/DeploymentJobs.java @@ -9,6 +9,7 @@ import com.yahoo.config.provision.RegionName; import com.yahoo.config.provision.SystemName; import com.yahoo.config.provision.Zone; import com.yahoo.vespa.hosted.controller.Controller; +import com.yahoo.vespa.hosted.controller.api.integration.organization.IssueId; import java.time.Instant; import java.util.Collection; @@ -30,20 +31,20 @@ public class DeploymentJobs { private final Optional<Long> projectId; private final ImmutableMap<JobType, JobStatus> status; - private final Optional<String> jiraIssueId; + private final Optional<IssueId> issueId; public DeploymentJobs(Optional<Long> projectId, Collection<JobStatus> jobStatusEntries, - Optional<String> jiraIssueId) { - this(projectId, asMap(jobStatusEntries), jiraIssueId); + Optional<IssueId> issueId) { + this(projectId, asMap(jobStatusEntries), issueId); } - private DeploymentJobs(Optional<Long> projectId, Map<JobType, JobStatus> status, Optional<String> jiraIssueId) { - requireId(projectId, "projectId cannot be null or <= 0"); + private DeploymentJobs(Optional<Long> projectId, Map<JobType, JobStatus> status, Optional<IssueId> issueId) { + requireId(projectId, "projectId must be a positive integer"); Objects.requireNonNull(status, "status cannot be null"); - Objects.requireNonNull(jiraIssueId, "jiraIssueId cannot be null"); + Objects.requireNonNull(issueId, "issueId cannot be null"); this.projectId = projectId; this.status = ImmutableMap.copyOf(status); - this.jiraIssueId = jiraIssueId; + this.issueId = issueId; } private static Map<JobType, JobStatus> asMap(Collection<JobStatus> jobStatusEntries) { @@ -60,7 +61,7 @@ public class DeploymentJobs { if (job == null) job = JobStatus.initial(report.jobType()); return job.withCompletion(report.jobError(), notificationTime, controller); }); - return new DeploymentJobs(Optional.of(report.projectId()), status, jiraIssueId); + return new DeploymentJobs(Optional.of(report.projectId()), status, issueId); } public DeploymentJobs withTriggering(JobType jobType, @@ -75,21 +76,21 @@ public class DeploymentJobs { change.isPresent() && change.get() instanceof Change.VersionChange, triggerTime); }); - return new DeploymentJobs(projectId, status, jiraIssueId); + return new DeploymentJobs(projectId, status, issueId); } public DeploymentJobs withProjectId(long projectId) { - return new DeploymentJobs(Optional.of(projectId), status, jiraIssueId); + return new DeploymentJobs(Optional.of(projectId), status, issueId); } - public DeploymentJobs withJiraIssueId(Optional<String> jiraIssueId) { - return new DeploymentJobs(projectId, status, jiraIssueId); + public DeploymentJobs with(IssueId issueId) { + return new DeploymentJobs(projectId, status, Optional.ofNullable(issueId)); } public DeploymentJobs without(JobType job) { Map<JobType, JobStatus> status = new HashMap<>(this.status); status.remove(job); - return new DeploymentJobs(projectId, status, jiraIssueId); + return new DeploymentJobs(projectId, status, issueId); } /** Returns an immutable map of the status entries in this */ @@ -158,7 +159,7 @@ public class DeploymentJobs { */ public Optional<Long> projectId() { return projectId; } - public Optional<String> jiraIssueId() { return jiraIssueId; } + public Optional<IssueId> issueId() { return issueId; } /** Job types that exist in the build system */ public enum JobType { diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ControllerMaintenance.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ControllerMaintenance.java index fd2e7496ec0..2fdce2802ab 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ControllerMaintenance.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ControllerMaintenance.java @@ -4,9 +4,7 @@ package com.yahoo.vespa.hosted.controller.maintenance; import com.yahoo.component.AbstractComponent; import com.yahoo.jdisc.Metric; import com.yahoo.vespa.hosted.controller.Controller; -import com.yahoo.vespa.hosted.controller.api.integration.Contacts; -import com.yahoo.vespa.hosted.controller.api.integration.Issues; -import com.yahoo.vespa.hosted.controller.api.integration.Properties; +import com.yahoo.vespa.hosted.controller.api.integration.organization.DeploymentIssues; import com.yahoo.vespa.hosted.controller.api.integration.chef.Chef; import com.yahoo.vespa.hosted.controller.maintenance.config.MaintainerConfig; import com.yahoo.vespa.hosted.controller.persistence.CuratorDb; @@ -40,12 +38,11 @@ public class ControllerMaintenance extends AbstractComponent { @SuppressWarnings("unused") // instantiated by Dependency Injection public ControllerMaintenance(MaintainerConfig maintainerConfig, Controller controller, CuratorDb curator, JobControl jobControl, Metric metric, Chef chefClient, - Contacts contactsClient, Properties propertiesClient, Issues issuesClient) { + DeploymentIssues deploymentIssues) { Duration maintenanceInterval = Duration.ofMinutes(maintainerConfig.intervalMinutes()); this.jobControl = jobControl; deploymentExpirer = new DeploymentExpirer(controller, maintenanceInterval, jobControl); - deploymentIssueReporter = new DeploymentIssueReporter(controller, contactsClient, propertiesClient, - issuesClient, maintenanceInterval, jobControl); + deploymentIssueReporter = new DeploymentIssueReporter(controller, deploymentIssues, maintenanceInterval, jobControl); metricsReporter = new MetricsReporter(controller, metric, chefClient, jobControl, controller.system()); failureRedeployer = new FailureRedeployer(controller, maintenanceInterval, jobControl); outstandingChangeDeployer = new OutstandingChangeDeployer(controller, maintenanceInterval, jobControl); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentIssueReporter.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentIssueReporter.java index b2f010eeb79..4ef92513393 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentIssueReporter.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentIssueReporter.java @@ -8,52 +8,35 @@ import com.yahoo.vespa.hosted.controller.api.Tenant; import com.yahoo.vespa.hosted.controller.api.application.v4.model.TenantType; import com.yahoo.vespa.hosted.controller.api.identifiers.PropertyId; import com.yahoo.vespa.hosted.controller.api.identifiers.TenantId; -import com.yahoo.vespa.hosted.controller.api.integration.Contacts; -import com.yahoo.vespa.hosted.controller.api.integration.Contacts.UserContact; -import com.yahoo.vespa.hosted.controller.api.integration.Issues; -import com.yahoo.vespa.hosted.controller.api.integration.Issues.Classification; -import com.yahoo.vespa.hosted.controller.api.integration.Issues.Issue; -import com.yahoo.vespa.hosted.controller.api.integration.Issues.IssueInfo; -import com.yahoo.vespa.hosted.controller.api.integration.Properties; +import com.yahoo.vespa.hosted.controller.api.integration.organization.DeploymentIssues; +import com.yahoo.vespa.hosted.controller.api.integration.organization.IssueId; +import com.yahoo.vespa.hosted.controller.api.integration.organization.User; import com.yahoo.vespa.hosted.controller.application.DeploymentJobs; import java.time.Duration; -import java.time.Instant; import java.util.ArrayList; import java.util.Collection; import java.util.List; import java.util.NoSuchElementException; import java.util.Optional; -import java.util.regex.Pattern; -import java.util.stream.Collectors; - -import static com.yahoo.vespa.hosted.controller.api.integration.Contacts.Category.admin; -import static com.yahoo.vespa.hosted.controller.api.integration.Issues.IssueInfo.Status.done; +import java.util.logging.Level; /** - * Maintenance job which creates Jira issues for tenants when they have jobs which fails continuously - * and escalates issues which are not handled. + * Maintenance job which files issues for tenants when they have jobs which fails continuously + * and escalates issues which are not handled in a timely manner. * * @author jvenstad */ public class DeploymentIssueReporter extends Maintainer { static final Duration maxFailureAge = Duration.ofDays(2); - static final Duration maxInactivityAge = Duration.ofDays(4); - static final String deploymentFailureLabel = "vespaDeploymentFailure"; - static final Classification vespaOps = new Classification("VESPA", "Services", deploymentFailureLabel, null); - static final UserContact terminalUser = new UserContact("frodelu", "Frode Lundgren", admin); + static final Duration maxInactivity = Duration.ofDays(4); - private final Contacts contacts; - private final Properties properties; - private final Issues issues; + private final DeploymentIssues deploymentIssues; - DeploymentIssueReporter(Controller controller, Contacts contacts, Properties properties, Issues issues, - Duration maintenanceInterval, JobControl jobControl) { + DeploymentIssueReporter(Controller controller, DeploymentIssues deploymentIssues, Duration maintenanceInterval, JobControl jobControl) { super(controller, maintenanceInterval, jobControl); - this.contacts = contacts; - this.properties = properties; - this.issues = issues; + this.deploymentIssues = deploymentIssues; } @Override @@ -63,182 +46,71 @@ public class DeploymentIssueReporter extends Maintainer { } /** - * File issues for applications which have failed deployment for longer than @maxFailureAge - * and store the issue id for the filed issues. Also, clear the @issueIds of applications + * File issues for applications which have failed deployment for longer than maxFailureAge + * and store the issue id for the filed issues. Also, clear the issueIds of applications * where deployment has not failed for this amount of time. */ private void maintainDeploymentIssues(List<Application> applications) { - Collection<Application> failingApplications = new ArrayList<>(); + List<ApplicationId> failingApplications = new ArrayList<>(); for (Application application : applications) - if (failingSinceBefore(application.deploymentJobs(), controller().clock().instant().minus(maxFailureAge))) - failingApplications.add(application); + if (hasFailuresOlderThanThreshold(application.deploymentJobs())) + failingApplications.add(application.id()); else - controller().applications().setJiraIssueId(application.id(), Optional.empty()); - - // TODO: Do this when version.confidence is BROKEN instead? Or, exclude above those upgrading to BROKEN version? - if (failingApplications.size() > 0.2 * applications.size()) { - fileOrUpdate(manyFailingDeploymentsIssueFrom(failingApplications)); // Problems with Vespa is the most likely cause when so many deployments fail. - } - else { - for (Application application : failingApplications) { - Issue deploymentIssue = deploymentIssueFrom(application); - Tenant applicationTenant = null; - Classification applicationOwner = null; - try { - applicationTenant= ownerOf(application); - applicationOwner = applicationTenant.tenantType() == TenantType.USER - ? vespaOps.withAssignee(applicationTenant.getId().id().replaceFirst("by-", "")) - : jiraClassificationOf(applicationTenant); - fileFor(application, deploymentIssue.with(applicationOwner)); - } - catch (RuntimeException e) { // Catch errors due to inconsistent or missing data in Sherpa, OpsDB, JIRA, and send to ourselves. - Pattern componentError = Pattern.compile(".*Component name '.*' is not valid.*", Pattern.DOTALL); - if (componentError.matcher(e.getMessage()).matches()) // Several properties seem to list invalid components, in which case we simply ignore this. - fileFor(application, - deploymentIssue - .with(applicationOwner.withComponent(null)) - .append("\n\nNote: The 'Queue Component' field in [opsdb|https://opsdb.ops.yahoo.com/properties.php?id=" + - applicationTenant.getPropertyId().get() + - "&action=view] for your property was rejected by JIRA. Please check your spelling.")); - else - fileFor(application, deploymentIssue.with(vespaOps).append(e.getMessage() + "\n\nAddressee:\n" + applicationOwner)); - } - } - } - } - - /** Returns whether @deploymentJobs has a job which has been failing since before @failureThreshold or not. */ - private boolean failingSinceBefore(DeploymentJobs deploymentJobs, Instant failureThreshold) { - return deploymentJobs.hasFailures() && deploymentJobs.failingSince().isBefore(failureThreshold); - } - - private Tenant ownerOf(Application application) { - return controller().tenants().tenant(new TenantId(application.id().tenant().value())).get(); - } - - /** Use the @propertyId of @tenant, if present, to look up JIRA information in OpsDB. */ - private Classification jiraClassificationOf(Tenant tenant) { - Long propertyId = tenant.getPropertyId().map(PropertyId::value).orElseThrow(() -> - new NoSuchElementException("No property id is listed for " + tenant)); - - Classification classification = properties.classificationFor(propertyId).orElseThrow(() -> - new NoSuchElementException("No property was found with id " + propertyId)); - - return classification.withLabel(deploymentFailureLabel); - } - - /** File @issue for @application, if @application doesn't already have an @Issue associated with it. */ - private void fileFor(Application application, Issue issue) { - Optional<String> ourIssueId = application.deploymentJobs().jiraIssueId() - .filter(jiraIssueId -> issues.fetch(jiraIssueId).status() != done); - - if ( ! ourIssueId.isPresent()) - controller().applications().setJiraIssueId(application.id(), Optional.of(issues.file(issue))); - } - - /** File @issue, or update a JIRA issue representing the same issue. */ - private void fileOrUpdate(Issue issue) { - Optional<String> jiraIssueId = issues.fetchSimilarTo(issue) - .stream().findFirst().map(Issues.IssueInfo::id); + controller().applications().setIssueId(application.id(), null); - if (jiraIssueId.isPresent()) - issues.update(jiraIssueId.get(), issue.description()); + // TODO: Change this logic, depending on the controller's definition of BROKEN, whether it updates applications + // TODO: to an older version when the system version is BROKEN, etc.. + if (failingApplications.size() > 0.2 * applications.size()) + deploymentIssues.fileUnlessOpen(failingApplications); else - issues.file(issue); + failingApplications.forEach(this::fileDeploymentIssueFor); } - /** Escalate JIRA issues for which there has been no activity for a set amount of time. */ - private void escalateInactiveDeploymentIssues(List<Application> applications) { - applications.forEach(application -> - application.deploymentJobs().jiraIssueId().ifPresent(jiraIssueId -> { - Issues.IssueInfo issueInfo = issues.fetch(jiraIssueId); - if (issueInfo.updated().isBefore(controller().clock().instant().minus(maxInactivityAge))) - escalateAndComment(issueInfo, application); - })); + /** Returns whether deploymentJobs has a job which has been failing since before failureThreshold. */ + private boolean hasFailuresOlderThanThreshold(DeploymentJobs deploymentJobs) { + return deploymentJobs.hasFailures() + && deploymentJobs.failingSince().isBefore(controller().clock().instant().minus(maxFailureAge)); } - /** Reassign the JIRA issue for @application one step up in the OpsDb escalation chain, and add an explanatory comment to it. */ - private void escalateAndComment(IssueInfo issueInfo, Application application) { - Optional<String> assignee = issueInfo.assignee(); - if (assignee.isPresent()) { - if (assignee.get().equals(terminalUser.username())) return; - issues.addWatcher(issueInfo.id(), assignee.get()); - } - - Long propertyId = ownerOf(application).getPropertyId().get().value(); - - UserContact escalationTarget = contacts.escalationTargetFor(propertyId, assignee.orElse("no one")); - if (escalationTarget.is(assignee.orElse("no one"))) - escalationTarget = terminalUser; - - String comment = deploymentIssueEscalationComment(application, propertyId, assignee.orElse("anyone")); - - issues.comment(issueInfo.id(), comment); - issues.reassign(issueInfo.id(), escalationTarget.username()); - } - - Issue deploymentIssueFrom(Application application) { - return new Issue(deploymentIssueSummary(application), deploymentIssueDescription(application)) - .with(vespaOps); - } - - Issue manyFailingDeploymentsIssueFrom(Collection<Application> applications) { - return new Issue( - "More than 20% of Hosted Vespa deployments are failing", - applications.stream() - .map(application -> "[" + application.id().toShortString() + "|" + toUrl(application.id()) + "]") - .collect(Collectors.joining("\n")), - vespaOps); + private Tenant ownerOf(ApplicationId applicationId) { + return controller().tenants().tenant(new TenantId(applicationId.tenant().value())) + .orElseThrow(() -> new IllegalStateException("No tenant found for application " + applicationId)); } - // TODO: Use the method of the same name in ApplicationId - private static String toShortString(ApplicationId id) { - return id.tenant().value() + "." + id.application().value() + - ( id.instance().isDefault() ? "" : "." + id.instance().value() ); + private User userFor(Tenant tenant) { + return User.from(tenant.getId().id().replaceFirst("by-", "")); } - private String toUrl(ApplicationId applicationId) { - return controller().zoneRegistry().getDashboardUri().resolve("/apps" + - "/tenant/" + applicationId.tenant().value() + - "/application/" + applicationId.application().value()).toString(); + private PropertyId propertyIdFor(Tenant tenant) { + return tenant.getPropertyId() + .orElseThrow(() -> new NoSuchElementException("No PropertyId is listed for non-user tenant " + tenant)); } - private String toOpsDbUrl(long propertyId) { - return contacts.contactsUri(propertyId).toString(); - - } - - /** Returns the summary text what will be assigned to a new issue */ - private static String deploymentIssueSummary(Application application) { - return "[" + toShortString(application.id()) + "] Action required: Repair deployment"; - } - - /** Returns the description text what will be assigned to a new issue */ - private String deploymentIssueDescription(Application application) { - return "Deployment jobs of the Vespa application " + - "[" + toShortString(application.id()) + "|" + toUrl(application.id()) + "] have been failing " + - "continuously for over 48 hours. This blocks any change to this application from being deployed " + - "and will also block global rollout of new Vespa versions for everybody.\n\n" + - "Please assign your highest priority to fixing this. If you need support, request it using " + - "[yo/vespa-support|http://yo/vespa-support]. " + - "If this application is not in use, please re-assign this issue to project \"VESPA\" " + - "with component \"Services\", and ask for the application to be removed.\n\n" + - "If we do not get a response on this issue, we will auto-escalate it."; + /** File an issue for applicationId, if it doesn't already have an open issue associated with it. */ + private void fileDeploymentIssueFor(ApplicationId applicationId) { + try { + Tenant tenant = ownerOf(applicationId); + Optional<IssueId> ourIssueId = controller().applications().require(applicationId).deploymentJobs().issueId(); + IssueId issueId = tenant.tenantType() == TenantType.USER + ? deploymentIssues.fileUnlessOpen(ourIssueId, applicationId, userFor(tenant)) + : deploymentIssues.fileUnlessOpen(ourIssueId, applicationId, propertyIdFor(tenant)); + controller().applications().setIssueId(applicationId, issueId); + } + catch (RuntimeException e) { // Catch errors due to wrong data in the controller, or issues client timeout. + log.log(Level.WARNING, "Exception caught when attempting to file an issue for " + applicationId, e); + } } - /** Returns the comment text that what will be added to an issue each time it is escalated */ - private String deploymentIssueEscalationComment(Application application, long propertyId, String priorAssignee) { - return "This issue tracks the failing deployment of Vespa application " + - "[" + toShortString(application.id()) + "|" + toUrl(application.id()) + "]. " + - "Since we have not received a response from " + priorAssignee + - ", we are escalating to you, " + - "based on [your OpsDb information|" + toOpsDbUrl(propertyId) + "]. " + - "Please acknowledge this issue and assign somebody to " + - "fix it as soon as possible.\n\n" + - "If we do not receive a response we will keep auto-escalating this issue. " + - "If we run out of escalation options for your OpsDb property, we will assume this application " + - "is not managed by anyone and DELETE it. In the meantime, this issue will block global deployment " + - "of Vespa for the entire company."; + /** Escalate issues for which there has been no activity for a certain amount of time. */ + private void escalateInactiveDeploymentIssues(Collection<Application> applications) { + applications.forEach(application -> application.deploymentJobs().issueId().ifPresent(issueId -> { + try { + deploymentIssues.escalateIfInactive(issueId, ownerOf(application.id()).getPropertyId(), maxInactivity); + } + catch (RuntimeException e) { + log.log(Level.WARNING, "Exception caught when attempting to escalate issue with id " + issueId, e); + } + })); } } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializer.java index 41d1a2d624c..fa4341b8e25 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializer.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializer.java @@ -15,6 +15,7 @@ import com.yahoo.slime.Inspector; import com.yahoo.slime.Slime; import com.yahoo.vespa.config.SlimeUtils; import com.yahoo.vespa.hosted.controller.Application; +import com.yahoo.vespa.hosted.controller.api.integration.organization.IssueId; import com.yahoo.vespa.hosted.controller.application.ApplicationRevision; import com.yahoo.vespa.hosted.controller.application.Change; import com.yahoo.vespa.hosted.controller.application.ClusterInfo; @@ -66,7 +67,7 @@ public class ApplicationSerializer { // DeploymentJobs fields private final String projectIdField = "projectId"; private final String jobStatusField = "jobStatus"; - private final String jiraIssueIdField = "jiraIssueId"; + private final String issueIdField = "jiraIssueId"; // JobStatus field private final String jobTypeField = "jobType"; @@ -203,7 +204,7 @@ public class ApplicationSerializer { .filter(id -> id > 0) // TODO: Discards invalid data. Remove filter after October 2017 .ifPresent(projectId -> cursor.setLong(projectIdField, projectId)); jobStatusToSlime(deploymentJobs.jobStatus().values(), cursor.setArray(jobStatusField)); - deploymentJobs.jiraIssueId().ifPresent(jiraIssueId -> cursor.setString(jiraIssueIdField, jiraIssueId)); + deploymentJobs.issueId().ifPresent(jiraIssueId -> cursor.setString(issueIdField, jiraIssueId.value())); } private void jobStatusToSlime(Collection<JobStatus> jobStatuses, Cursor jobStatusArray) { @@ -345,9 +346,9 @@ public class ApplicationSerializer { Optional<Long> projectId = optionalLong(object.field(projectIdField)) .filter(id -> id > 0); // TODO: Discards invalid data. Remove filter after October 2017 List<JobStatus> jobStatusList = jobStatusListFromSlime(object.field(jobStatusField)); - Optional<String> jiraIssueKey = optionalString(object.field(jiraIssueIdField)); + Optional<IssueId> issueId = optionalString(object.field(issueIdField)).map(IssueId::from); - return new DeploymentJobs(projectId, jobStatusList, jiraIssueKey); + return new DeploymentJobs(projectId, jobStatusList, issueId); } private Optional<Change> changeFromSlime(Inspector object) { diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java index c50f1464be7..6ae9761b305 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java @@ -53,6 +53,7 @@ import com.yahoo.vespa.hosted.controller.api.identifiers.UserId; import com.yahoo.vespa.hosted.controller.api.integration.MetricsService; import com.yahoo.vespa.hosted.controller.api.integration.configserver.ConfigServerException; import com.yahoo.vespa.hosted.controller.api.integration.configserver.Log; +import com.yahoo.vespa.hosted.controller.api.integration.organization.User; import com.yahoo.vespa.hosted.controller.api.integration.routing.RotationStatus; import com.yahoo.vespa.hosted.controller.application.ApplicationPackage; import com.yahoo.vespa.hosted.controller.application.ApplicationRevision; @@ -300,12 +301,28 @@ public class ApplicationApiHandler extends LoggingRequestHandler { } private HttpResponse tenant(String tenantName, HttpRequest request) { - Optional<Tenant> tenant = controller.tenants().tenant(new TenantId(tenantName)); - if ( ! tenant.isPresent()) - return ErrorResponse.notFoundError("Tenant '" + tenantName + "' does not exist"); - return new SlimeJsonResponse(toSlime(tenant.get(), request, true)); + return controller.tenants().tenant(new TenantId((tenantName))) + .map(tenant -> tenant(tenant, request, true)) + .orElseGet(() -> ErrorResponse.notFoundError("Tenant '" + tenantName + "' does not exist")); + } + + private HttpResponse tenant(Tenant tenant, HttpRequest request, boolean listApplications) { + Slime tenantSlime = toSlime(tenant, request, listApplications); + tenant.getPropertyId().ifPresent(propertyId -> { + try { + toSlime(tenantSlime.get(), + controller.organization().propertyUri(propertyId), + controller.organization().contactsUri(propertyId), + controller.organization().issueCreationUri(propertyId), + controller.organization().contactsFor(propertyId)); + } + catch (RuntimeException e) { + log.log(Level.WARNING, "Error fetching property info for " + tenant + " with propertyId " + propertyId, e); + } + }); + return new SlimeJsonResponse(tenantSlime); } - + private HttpResponse applications(String tenantName, HttpRequest request) { TenantName tenant = TenantName.from(tenantName); Slime slime = new Slime(); @@ -621,7 +638,7 @@ public class ApplicationApiHandler extends LoggingRequestHandler { throw new BadRequestException("Unknown tenant type: " + existingTenant.get().tenantType()); } } - return new SlimeJsonResponse(toSlime(updatedTenant, request, true)); + return tenant(updatedTenant, request, true); } private HttpResponse createTenant(String tenantName, HttpRequest request) { @@ -641,7 +658,7 @@ public class ApplicationApiHandler extends LoggingRequestHandler { throwIfNotAthenzDomainAdmin(new AthenzDomain(mandatory("athensDomain", requestData).asString()), request); controller.tenants().addTenant(tenant, authorizer.getNToken(request)); - return new SlimeJsonResponse(toSlime(tenant, request, true)); + return tenant(tenant, request, true); } private HttpResponse migrateTenant(String tenantName, HttpRequest request) { @@ -657,7 +674,7 @@ public class ApplicationApiHandler extends LoggingRequestHandler { .orElseThrow(() -> new BadRequestException("The NToken for a domain admin is required to migrate tenant to Athens")); Tenant tenant = controller.tenants().migrateTenantToAthenz(tenantid, tenantDomain, propertyId, property, nToken); - return new SlimeJsonResponse(toSlime(tenant, request, true)); + return tenant(tenant, request, true); } private HttpResponse createApplication(String tenantName, String applicationName, HttpRequest request) { @@ -802,7 +819,7 @@ public class ApplicationApiHandler extends LoggingRequestHandler { controller.tenants().deleteTenant(new TenantId(tenantName), authorizer.getNToken(request)); // TODO: Change to a message response saying the tenant was deleted - return new SlimeJsonResponse(toSlime(tenant.get(), request, false)); + return tenant(tenant.get(), request, false); } private HttpResponse deleteApplication(String tenantName, String applicationName, HttpRequest request) { @@ -990,6 +1007,18 @@ public class ApplicationApiHandler extends LoggingRequestHandler { return slime; } + private void toSlime(Cursor root, URI propertyUri, URI contactsUri, URI issueCreationUri, List<? extends List<? extends User>> contacts) { + root.setString("propertyUrl", propertyUri.toString()); + root.setString("contactsUrl", contactsUri.toString()); + root.setString("issueCreationUrl", issueCreationUri.toString()); + Cursor lists = root.setArray("contacts"); + for (List<? extends User> contactList : contacts) { + Cursor list = lists.addArray(); + for (User contact : contactList) + list.addString(contact.displayName()); + } + } + private void toSlime(Application application, Cursor object, HttpRequest request) { object.setString("application", application.id().application().value()); object.setString("instance", application.id().instance().value()); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTester.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTester.java index b49d55aeb3b..deb7b77eb56 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTester.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTester.java @@ -25,7 +25,7 @@ import com.yahoo.vespa.hosted.controller.api.integration.chef.ChefMock; import com.yahoo.vespa.hosted.controller.api.integration.dns.MemoryNameService; import com.yahoo.vespa.hosted.controller.api.integration.entity.MemoryEntityService; import com.yahoo.vespa.hosted.controller.api.integration.github.GitHubMock; -import com.yahoo.vespa.hosted.controller.api.integration.jira.JiraMock; +import com.yahoo.vespa.hosted.controller.api.integration.organization.MockOrganization; import com.yahoo.vespa.hosted.controller.api.integration.routing.MemoryGlobalRoutingService; import com.yahoo.vespa.hosted.controller.application.ApplicationPackage; import com.yahoo.vespa.hosted.controller.athenz.mock.AthenzDbMock; @@ -204,8 +204,8 @@ public final class ControllerTester { curator, new MemoryRotationRepository(), gitHubClientMock, - new JiraMock(), new MemoryEntityService(), + new MockOrganization(clock), new MemoryGlobalRoutingService(), zoneRegistryMock, configServerClientMock, diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentIssueReporterTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentIssueReporterTest.java index f8d09ac8b27..aa93fb1cfe2 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentIssueReporterTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentIssueReporterTest.java @@ -1,13 +1,11 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.controller.maintenance; +import com.yahoo.config.provision.ApplicationId; import com.yahoo.config.provision.Environment; import com.yahoo.vespa.hosted.controller.Application; -import com.yahoo.vespa.hosted.controller.api.integration.Contacts.UserContact; -import com.yahoo.vespa.hosted.controller.api.integration.Issues; -import com.yahoo.vespa.hosted.controller.api.integration.Issues.IssueInfo; -import com.yahoo.vespa.hosted.controller.api.integration.stubs.ContactsMock; -import com.yahoo.vespa.hosted.controller.api.integration.stubs.PropertiesMock; +import com.yahoo.vespa.hosted.controller.api.integration.organization.IssueId; +import com.yahoo.vespa.hosted.controller.api.integration.stubs.LoggingDeploymentIssues; import com.yahoo.vespa.hosted.controller.application.ApplicationPackage; import com.yahoo.vespa.hosted.controller.deployment.ApplicationPackageBuilder; import com.yahoo.vespa.hosted.controller.deployment.DeploymentTester; @@ -15,28 +13,18 @@ import com.yahoo.vespa.hosted.controller.persistence.MockCuratorDb; import org.junit.Before; import org.junit.Test; -import java.time.Clock; import java.time.Duration; -import java.util.Arrays; import java.util.HashMap; -import java.util.List; import java.util.Map; -import java.util.Optional; -import java.util.stream.Collectors; -import static com.yahoo.vespa.hosted.controller.api.integration.Contacts.Category.admin; -import static com.yahoo.vespa.hosted.controller.api.integration.Contacts.Category.engineeringOwner; -import static com.yahoo.vespa.hosted.controller.api.integration.Issues.IssueInfo.Status.done; -import static com.yahoo.vespa.hosted.controller.api.integration.Issues.IssueInfo.Status.toDo; import static com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType.component; import static com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType.productionCorpUsEast1; import static com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType.stagingTest; import static com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType.systemTest; import static com.yahoo.vespa.hosted.controller.maintenance.DeploymentIssueReporter.maxFailureAge; -import static com.yahoo.vespa.hosted.controller.maintenance.DeploymentIssueReporter.maxInactivityAge; -import static com.yahoo.vespa.hosted.controller.maintenance.DeploymentIssueReporter.terminalUser; -import static com.yahoo.vespa.hosted.controller.maintenance.DeploymentIssueReporter.vespaOps; +import static com.yahoo.vespa.hosted.controller.maintenance.DeploymentIssueReporter.maxInactivity; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; /** @@ -51,27 +39,18 @@ public class DeploymentIssueReporterTest { private DeploymentTester tester; private DeploymentIssueReporter reporter; - private ContactsMock contacts; - private PropertiesMock properties; - private MockIssues issues; + private MockDeploymentIssues issues; @Before public void setup() { tester = new DeploymentTester(); - contacts = new ContactsMock(); - properties = new PropertiesMock(); - issues = new MockIssues(tester.clock()); - reporter = new DeploymentIssueReporter(tester.controller(), contacts, properties, issues, Duration.ofMinutes(5), - new JobControl(new MockCuratorDb())); - } - - private List<IssueInfo> openIssuesFor(Application application) { - return issues.fetchSimilarTo(reporter.deploymentIssueFrom(tester.controller().applications().require(application.id()))); + issues = new MockDeploymentIssues(); + reporter = new DeploymentIssueReporter(tester.controller(), issues, Duration.ofMinutes(5), new JobControl(new MockCuratorDb())); } @Test public void testDeploymentFailureReporting() { - // All applications deploy from unique SD projects. + // All applications deploy from unique build projects. Long projectId1 = 10L; Long projectId2 = 20L; Long projectId3 = 30L; @@ -79,11 +58,12 @@ public class DeploymentIssueReporterTest { // Only the first two have propertyIds set now. Long propertyId1 = 1L; Long propertyId2 = 2L; + Long propertyId3 = 3L; // Create and deploy one application for each of three tenants. Application app1 = tester.createApplication("application1", "tenant1", projectId1, propertyId1); Application app2 = tester.createApplication("application2", "tenant2", projectId2, propertyId2); - Application app3 = tester.createApplication("application3", "tenant3", projectId3, null); + Application app3 = tester.createApplication("application3", "tenant3", projectId3, propertyId3); // And then we need lots of successful applications, so we won't assume we just have a faulty Vespa out. for (long i = 4; i <= 10; i++) { @@ -93,18 +73,6 @@ public class DeploymentIssueReporterTest { tester.deployAndNotify(app, applicationPackage, true, stagingTest); tester.deployAndNotify(app, applicationPackage, true, productionCorpUsEast1); } - - // Both the first tenants belong to the same JIRA queue. (Not sure if this is possible, but let's test it anyway. - String jiraQueue = "PROJECT"; - properties.addClassification(propertyId1, jiraQueue); - properties.addClassification(propertyId1, jiraQueue); - - // Only tenant1 has contacts listed in opsDb. - UserContact - alice = new UserContact("alice", "Alice", admin), - bob = new UserContact("bob", "Robert", engineeringOwner); - contacts.addContact(propertyId1, Arrays.asList(alice, bob)); - // end of setup. // NOTE: All maintenance should be idempotent within a small enough time interval, so maintain is called twice in succession throughout. @@ -125,7 +93,7 @@ public class DeploymentIssueReporterTest { reporter.maintain(); reporter.maintain(); - assertEquals("No deployments are detected as failing for a long time initially.", 0, issues.issues.size()); + assertEquals("No deployments are detected as failing for a long time initially.", 0, issues.size()); // Advance to where deployment issues should be detected. @@ -133,146 +101,103 @@ public class DeploymentIssueReporterTest { reporter.maintain(); reporter.maintain(); - assertEquals("One issue is produced for app1.", 1, openIssuesFor(app1).size()); - assertEquals("No issues are produced for app2.", 0, openIssuesFor(app2).size()); - assertEquals("One issue is produced for app3.", 1, openIssuesFor(app3).size()); - assertTrue("The issue for app1 is stored in their JIRA queue.", openIssuesFor(app1).get(0).key().startsWith(jiraQueue)); - assertTrue("The issue for an application without propertyId is addressed to vespaOps.", openIssuesFor(app3).get(0).key().startsWith(vespaOps.queue())); + assertTrue("One issue is produced for app1.", issues.isOpenFor(app1.id())); + assertFalse("No issues are produced for app2.", issues.isOpenFor(app2.id())); + assertTrue("One issue is produced for app3.", issues.isOpenFor(app3.id())); - // Verify idempotency of filing. - reporter.maintain(); - reporter.maintain(); - assertEquals("No issues are re-filed when still open.", 2, issues.issues.size()); - - - // tenant3 closes their issue prematurely; see that we get a new filing. - issues.complete(openIssuesFor(app3).get(0).id()); - assertEquals("The issue is removed (test of the tester, really...).", 0, openIssuesFor(app3).size()); + // app3 closes their issue prematurely; see that it is refiled. + issues.closeFor(app3.id()); + assertFalse("No issue is open for app3.", issues.isOpenFor(app3.id())); reporter.maintain(); reporter.maintain(); - assertTrue("Issue is re-produced for app3, addressed correctly.", openIssuesFor(app3).get(0).key().startsWith(vespaOps.queue())); + assertTrue("Issue is re-filed for app3.", issues.isOpenFor(app3.id())); // Some time passes; tenant1 leaves her issue unattended, while tenant3 starts work and updates the issue. // app2 also has an intermittent failure; see that we detect this as a Vespa problem, and file an issue to ourselves. tester.deployAndNotify(app2, applicationPackage, false, productionCorpUsEast1); - tester.clock().advance(maxInactivityAge.plus(maxFailureAge)); - issues.comment(openIssuesFor(app3).get(0).id(), "We are trying to fix it!"); - - reporter.maintain(); - reporter.maintain(); - assertEquals("The issue for app1 is escalated once.", alice.username(), openIssuesFor(app1).get(0).assignee().get()); - + tester.clock().advance(maxInactivity.plus(maxFailureAge)); + issues.touchFor(app3.id()); + assertFalse("We have no platform issues initially.", issues.platformIssue()); reporter.maintain(); reporter.maintain(); - assertEquals("We get an issue to vespaOps when more than 20% of applications have old failures.", 1, - issues.fetchSimilarTo(reporter.manyFailingDeploymentsIssueFrom(Arrays.asList( - tester.controller().applications().get(app1.id()).get(), - tester.controller().applications().get(app2.id()).get(), - tester.controller().applications().get(app3.id()).get()))).size()); - assertEquals("No issue is filed for app2 while Vespa is considered broken.", 0, openIssuesFor(app2).size()); + assertEquals("The issue for app1 is escalated once.", 1, issues.escalationLevelFor(app1.id())); + assertTrue("We get a platform issue when more than 20% of applications are failing.", issues.platformIssue()); + assertFalse("No issue is filed for app2 while Vespa is considered broken.", issues.isOpenFor(app2.id())); // app3 fixes its problem, but the ticket is left open; see the resolved ticket is not escalated when another escalation period has passed. tester.deployAndNotify(app2, applicationPackage, true, productionCorpUsEast1); tester.deployAndNotify(app3, applicationPackage, true, productionCorpUsEast1); - tester.clock().advance(maxInactivityAge.plus(Duration.ofDays(1))); + tester.clock().advance(maxInactivity.plus(Duration.ofDays(1))); reporter.maintain(); reporter.maintain(); - assertEquals("The issue for app1 is escalated once more.", bob.username(), openIssuesFor(app1).get(0).assignee().get()); - assertEquals("The issue for app3 is still unassigned.", Optional.empty(), openIssuesFor(app3).get(0).assignee()); + assertFalse("We no longer have a platform issue.", issues.platformIssue()); + assertEquals("The issue for app1 is escalated once more.", 2, issues.escalationLevelFor(app1.id())); + assertEquals("The issue for app3 is not escalated.", 0, issues.escalationLevelFor(app3.id())); - // app1 still does nothing with their issue; see the terminal user gets it in the end. // app3 now has a new failure past max failure age; see that a new issue is filed. tester.notifyJobCompletion(component, app3, true); tester.deployAndNotify(app3, applicationPackage, true, systemTest); tester.deployAndNotify(app3, applicationPackage, true, stagingTest); tester.deployAndNotify(app3, applicationPackage, false, productionCorpUsEast1); - tester.clock().advance(maxInactivityAge.plus(maxFailureAge)); + tester.clock().advance(maxInactivity.plus(maxFailureAge)); reporter.maintain(); reporter.maintain(); - assertEquals("The issue for app1 is escalated to the terminal user.", terminalUser.username(), openIssuesFor(app1).get(0).assignee().get()); - assertEquals("A new issue is filed for app3.", 2, openIssuesFor(app3).size()); + assertTrue("A new issue is filed for app3.", issues.isOpenFor(app3.id())); } - class MockIssues implements Issues { - final Map<String, Issue> issues = new HashMap<>(); - final Map<String, IssueInfo> metas = new HashMap<>(); - final Map<String, Long> counters = new HashMap<>(); - Clock clock; + class MockDeploymentIssues extends LoggingDeploymentIssues { - MockIssues(Clock clock) { this.clock = clock; } + Map<ApplicationId, IssueId> applicationIssues = new HashMap<>(); + Map<IssueId, Integer> issueLevels = new HashMap<>(); - public void addWatcher(String jiraIssueId, String watcher) { - touch(jiraIssueId); + MockDeploymentIssues() { + super(tester.clock()); } - public void reassign(String jiraIssueId, String assignee) { - metas.compute(jiraIssueId, (__, jiraIssueMeta) -> - new IssueInfo( - jiraIssueId, - jiraIssueMeta.key(), - clock.instant(), - Optional.of(assignee), - jiraIssueMeta.status())); + @Override + protected void escalateIssue(IssueId issueId) { + super.escalateIssue(issueId); + issueLevels.merge(issueId, 1, Integer::sum); } - public void comment(String jiraIssueId, String comment) { - touch(jiraIssueId); + @Override + protected IssueId fileIssue(ApplicationId applicationId) { + IssueId issueId = super.fileIssue(applicationId); + applicationIssues.put(applicationId, issueId); + return issueId; } - public void update(String jiraIssueId, String description) { - issues.compute(jiraIssueId, (__, issue) -> - new Issue(issue.summary(), description, issue.classification().orElse(null))); + void closeFor(ApplicationId applicationId) { + issueUpdates.remove(applicationIssues.remove(applicationId)); } - public String file(Issue issue) { - String jiraIssueId = (issues.size() + 1L) + ""; - Long counter = counters.merge(issue.classification().get().queue(), 0L, (old, __) -> old + 1); - String jiraIssueKey = issue.classification().get().queue() + '-' + counter; - issues.put(jiraIssueId, issue); - metas.put(jiraIssueId, new IssueInfo(jiraIssueId, jiraIssueKey, clock.instant(), null, toDo)); - return jiraIssueId; + void touchFor(ApplicationId applicationId) { + issueUpdates.put(applicationIssues.get(applicationId), tester.clock().instant()); } - public IssueInfo fetch(String jiraIssueId) { - return metas.get(jiraIssueId); + boolean isOpenFor(ApplicationId applicationId) { + return applicationIssues.containsKey(applicationId); } - public List<IssueInfo> fetchSimilarTo(Issue issue) { - return issues.entrySet().stream() - .filter(entry -> entry.getValue().summary().equals(issue.summary())) - .map(Map.Entry::getKey) - .map(metas::get) - .filter(meta -> meta.status() != done) - .collect(Collectors.toList()); + int escalationLevelFor(ApplicationId applicationId) { + return issueLevels.getOrDefault(applicationIssues.get(applicationId), 0); } - private void complete(String jiraIssueId) { - metas.compute(jiraIssueId, (__, jiraIssueMeta) -> - new IssueInfo( - jiraIssueId, - jiraIssueMeta.key(), - clock.instant(), - jiraIssueMeta.assignee(), - done)); + int size() { + return issueUpdates.size(); } - private void touch(String jiraIssueId) { - metas.compute(jiraIssueId, (__, jiraIssueMeta) -> - new IssueInfo( - jiraIssueId, - jiraIssueMeta.key(), - clock.instant(), - jiraIssueMeta.assignee(), - jiraIssueMeta.status())); + boolean platformIssue() { + return platformIssue.get(); } } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ControllerContainerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ControllerContainerTest.java index 5c102b0d9df..e6c0ce9027d 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ControllerContainerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ControllerContainerTest.java @@ -41,11 +41,9 @@ public class ControllerContainerTest { " <component id='com.yahoo.vespa.hosted.controller.api.integration.dns.MemoryNameService'/>" + " <component id='com.yahoo.vespa.hosted.controller.api.integration.entity.MemoryEntityService'/>" + " <component id='com.yahoo.vespa.hosted.controller.api.integration.github.GitHubMock'/>" + - " <component id='com.yahoo.vespa.hosted.controller.api.integration.jira.JiraMock'/>" + " <component id='com.yahoo.vespa.hosted.controller.api.integration.routing.MemoryGlobalRoutingService'/>" + - " <component id='com.yahoo.vespa.hosted.controller.api.integration.stubs.ContactsMock'/>" + - " <component id='com.yahoo.vespa.hosted.controller.api.integration.stubs.LoggingIssues'/>" + - " <component id='com.yahoo.vespa.hosted.controller.api.integration.stubs.PropertiesMock'/>" + + " <component id='com.yahoo.vespa.hosted.controller.api.integration.stubs.LoggingDeploymentIssues'/>" + + " <component id='com.yahoo.vespa.hosted.controller.api.integration.organization.MockOrganization'/>" + " <component id='com.yahoo.vespa.hosted.controller.ConfigServerClientMock'/>" + " <component id='com.yahoo.vespa.hosted.controller.ZoneRegistryMock'/>" + " <component id='com.yahoo.vespa.hosted.controller.Controller'/>" + diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java index 1ac5dfeb58a..b36a88ca1d4 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java @@ -9,8 +9,11 @@ import com.yahoo.vespa.curator.Lock; import com.yahoo.vespa.hosted.controller.Application; import com.yahoo.vespa.hosted.controller.ConfigServerClientMock; import com.yahoo.vespa.hosted.controller.api.identifiers.AthenzDomain; +import com.yahoo.vespa.hosted.controller.api.identifiers.PropertyId; import com.yahoo.vespa.hosted.controller.api.identifiers.UserId; import com.yahoo.vespa.hosted.controller.api.integration.configserver.ConfigServerException; +import com.yahoo.vespa.hosted.controller.api.integration.organization.MockOrganization; +import com.yahoo.vespa.hosted.controller.api.integration.organization.User; import com.yahoo.vespa.hosted.controller.application.ApplicationPackage; import com.yahoo.vespa.hosted.controller.application.ClusterInfo; import com.yahoo.vespa.hosted.controller.application.ClusterUtilization; @@ -37,6 +40,8 @@ import java.io.UncheckedIOException; import java.net.URI; import java.nio.charset.StandardCharsets; import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -79,7 +84,7 @@ public class ApplicationApiTest extends ControllerContainerTest { new File("cookiefreshness.json")); // POST (add) a tenant without property ID tester.assertResponse(request("/application/v4/tenant/tenant1", - "{\"athensDomain\":\"domain1\", \"property\":\"property1\"}", + "{\"athensDomain\":\"domain1\", \"property\":\"property1\"}", Request.Method.POST), new File("tenant-without-applications.json")); // PUT (modify) a tenant @@ -101,6 +106,7 @@ public class ApplicationApiTest extends ControllerContainerTest { // GET a tenant tester.assertResponse(request("/application/v4/tenant/tenant1", "", Request.Method.GET), new File("tenant-with-application.json")); + // GET tenant applications tester.assertResponse(request("/application/v4/tenant/tenant1/application/", "", Request.Method.GET), new File("application-list.json")); @@ -236,6 +242,8 @@ public class ApplicationApiTest extends ControllerContainerTest { // Add another Athens domain, so we can try to create more tenants addTenantAthenzDomain("domain2", "mytenant"); // New domain to test tenant w/property ID + // Add property info for that property id, as well, in the mock organization. + addPropertyData((MockOrganization) controllerTester.controller().organization(), "1234"); // POST (add) a tenant with property ID tester.assertResponse(request("/application/v4/tenant/tenant2", "{\"athensDomain\":\"domain2\", \"property\":\"property2\", \"propertyId\":\"1234\"}", @@ -286,6 +294,13 @@ public class ApplicationApiTest extends ControllerContainerTest { controllerTester.controller().deconstruct(); } + private void addPropertyData(MockOrganization organization, String propertyIdValue) { + PropertyId propertyId = new PropertyId(propertyIdValue); + organization.addProperty(propertyId); + organization.setContactsFor(propertyId, Arrays.asList(Collections.singletonList(User.from("alice")), + Collections.singletonList(User.from("bob")))); + } + @Test public void testDeployDirectly() throws Exception { // Setup @@ -755,4 +770,5 @@ public class ApplicationApiTest extends ControllerContainerTest { } } } + } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/opsdb-tenant-with-id-without-applications.json b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/opsdb-tenant-with-id-without-applications.json index 8de85754ab0..8acb4a045f3 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/opsdb-tenant-with-id-without-applications.json +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/opsdb-tenant-with-id-without-applications.json @@ -5,5 +5,16 @@ "userGroup": "group1", "applications": [ + ], + "propertyUrl": "www.properties.tld/1234", + "contactsUrl": "www.contacts.tld/1234", + "issueCreationUrl": "www.issues.tld/1234", + "contacts": [ + [ + "alice" + ], + [ + "bob" + ] ] -}
\ No newline at end of file +} diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/opsdb-tenant-with-new-id-without-applications.json b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/opsdb-tenant-with-new-id-without-applications.json index 9f0a7ec603e..f00b3c5bb1a 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/opsdb-tenant-with-new-id-without-applications.json +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/opsdb-tenant-with-new-id-without-applications.json @@ -6,4 +6,4 @@ "applications": [ ] -}
\ No newline at end of file +} diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/tenant-list.json b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/tenant-list.json index a9d9cd33ae8..d7ec9a738f2 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/tenant-list.json +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/tenant-list.json @@ -8,4 +8,4 @@ }, "url": "http://localhost:8080/application/v4/tenant/tenant1" } -]
\ No newline at end of file +] diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/tenant-without-applications-with-id.json b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/tenant-without-applications-with-id.json index 3deef01bb44..ede2413218d 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/tenant-without-applications-with-id.json +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/tenant-without-applications-with-id.json @@ -5,5 +5,16 @@ "propertyId": "1234", "applications": [ + ], + "propertyUrl": "www.properties.tld/1234", + "contactsUrl": "www.contacts.tld/1234", + "issueCreationUrl": "www.issues.tld/1234", + "contacts": [ + [ + "alice" + ], + [ + "bob" + ] ] -}
\ No newline at end of file +} diff --git a/documentapi/src/tests/systemstate/systemstate.cpp b/documentapi/src/tests/systemstate/systemstate.cpp index 7aadf82aeed..3e42c94950a 100644 --- a/documentapi/src/tests/systemstate/systemstate.cpp +++ b/documentapi/src/tests/systemstate/systemstate.cpp @@ -174,7 +174,7 @@ Test::testHandle() SystemStateHandle handle(*state); ASSERT_TRUE(handle.isValid()); - SystemStateHandle hoe(handle); + SystemStateHandle hoe(std::move(handle)); ASSERT_TRUE(!handle.isValid()); ASSERT_TRUE(hoe.isValid()); } diff --git a/documentapi/src/vespa/documentapi/messagebus/systemstate/systemstatehandle.cpp b/documentapi/src/vespa/documentapi/messagebus/systemstate/systemstatehandle.cpp index ad2f4485ac8..9ccaece4511 100644 --- a/documentapi/src/vespa/documentapi/messagebus/systemstate/systemstatehandle.cpp +++ b/documentapi/src/vespa/documentapi/messagebus/systemstate/systemstatehandle.cpp @@ -4,34 +4,28 @@ using namespace documentapi; -SystemStateHandover::SystemStateHandover(SystemState *state, vespalib::LockGuard &guard) : - _state(state), - _guard(guard) -{} - SystemStateHandle::SystemStateHandle(SystemState &state) : _state(&state), _guard(*state._lock) {} -SystemStateHandle::SystemStateHandle(SystemStateHandle &rhs) : +SystemStateHandle::SystemStateHandle(SystemStateHandle &&rhs) : _state(rhs._state), - _guard(rhs._guard) + _guard(std::move(rhs._guard)) { rhs._state = nullptr; } -SystemStateHandle::SystemStateHandle(const SystemStateHandover &rhs) : - _state(rhs._state), - _guard(rhs._guard) -{} +SystemStateHandle & +SystemStateHandle::operator=(SystemStateHandle &&rhs) +{ + if (this != &rhs) { + _state = rhs._state; + _guard = std::move(rhs._guard); + rhs._state = nullptr; + } + return *this; +} SystemStateHandle::~SystemStateHandle() {} -SystemStateHandle::operator -SystemStateHandover() { - SystemStateHandover ret(_state, _guard); - _state = nullptr; - return ret; -} - diff --git a/documentapi/src/vespa/documentapi/messagebus/systemstate/systemstatehandle.h b/documentapi/src/vespa/documentapi/messagebus/systemstate/systemstatehandle.h index aa87e1ee0ab..f0342cfb2de 100644 --- a/documentapi/src/vespa/documentapi/messagebus/systemstate/systemstatehandle.h +++ b/documentapi/src/vespa/documentapi/messagebus/systemstate/systemstatehandle.h @@ -7,31 +7,15 @@ namespace documentapi { /** - * Implements a handover class to enable the system state handler to be perform handover even on const objects - * such as occur when returning a handle by value from a function. - */ -class SystemStateHandover { - friend class SystemStateHandle; -private: - SystemStateHandover(const SystemStateHandover &); - SystemStateHandover &operator=(const SystemStateHandover &); - SystemStateHandover(SystemState *state, vespalib::LockGuard &guard); - -private: - SystemState *_state; - mutable vespalib::LockGuard _guard; -}; - -/** - * Implements a handle to grant synchronized access to the content of a system state object. This needs the - * above handover class to be able to return itself from methods that create it. + * Implements a handle to grant synchronized access to the content of a system state object. */ class SystemStateHandle { private: SystemState *_state; // The associated system state for which this object is a handler. vespalib::LockGuard _guard; // The lock guard for the system state's lock. - SystemState &operator=(const SystemStateHandle &rhs); // hide + SystemStateHandle &operator=(const SystemStateHandle &) = delete; + SystemStateHandle(const SystemStateHandle &) = delete; public: /** @@ -42,28 +26,19 @@ public: SystemStateHandle(SystemState &state); /** - * Implements the copy constructor. - * - * @param rhs The handle to copy to this. - */ - SystemStateHandle(SystemStateHandle &rhs); - - /** - * Implements the copy constructor for a const handle. + * Implements the move constructor. * - * @param rhs The handle to copy to this. + * @param rhs The handle to move to this. */ - SystemStateHandle(const SystemStateHandover &rhs); + SystemStateHandle(SystemStateHandle &&rhs); + SystemStateHandle &operator=(SystemStateHandle &&rhs); /** * Destructor. Releases the contained lock on the associated system state object. There is no unlock() * mechanism provided, since this will happen automatically as soon as this handle goes out of scope. */ ~SystemStateHandle(); - /** Implements a cast-operator for handover. */ - operator SystemStateHandover(); - /** Returns whether or not this handle is valid. */ bool isValid() const { return _state != NULL; } diff --git a/eval/src/apps/tensor_conformance/generate.cpp b/eval/src/apps/tensor_conformance/generate.cpp index d3374665fd4..1f621e33d26 100644 --- a/eval/src/apps/tensor_conformance/generate.cpp +++ b/eval/src/apps/tensor_conformance/generate.cpp @@ -49,7 +49,7 @@ void generate_tensor_reduce(TestBuilder &dst) { //----------------------------------------------------------------------------- -void generate_map_expr(const vespalib::string &expr, const UnaryOperation &ref_op, const Sequence &seq, TestBuilder &dst) { +void generate_map_expr(const vespalib::string &expr, map_fun_t ref_op, const Sequence &seq, TestBuilder &dst) { std::vector<Layout> layouts = { {}, {x(3)}, @@ -66,34 +66,34 @@ void generate_map_expr(const vespalib::string &expr, const UnaryOperation &ref_o } } -void generate_op1_map(const vespalib::string &op1_expr, const UnaryOperation &ref_op, const Sequence &seq, TestBuilder &dst) { +void generate_op1_map(const vespalib::string &op1_expr, map_fun_t ref_op, const Sequence &seq, TestBuilder &dst) { generate_map_expr(op1_expr, ref_op, seq, dst); generate_map_expr(vespalib::make_string("map(a,f(a)(%s))", op1_expr.c_str()), ref_op, seq, dst); } void generate_tensor_map(TestBuilder &dst) { - generate_op1_map("-a", operation::Neg(), Sub2(Div10(N())), dst); - generate_op1_map("!a", operation::Not(), Mask2Seq(SkipNth(3)), dst); - generate_op1_map("cos(a)", operation::Cos(), Div10(N()), dst); - generate_op1_map("sin(a)", operation::Sin(), Div10(N()), dst); - generate_op1_map("tan(a)", operation::Tan(), Div10(N()), dst); - generate_op1_map("cosh(a)", operation::Cosh(), Div10(N()), dst); - generate_op1_map("sinh(a)", operation::Sinh(), Div10(N()), dst); - generate_op1_map("tanh(a)", operation::Tanh(), Div10(N()), dst); - generate_op1_map("acos(a)", operation::Acos(), Sigmoid(Div10(N())), dst); - generate_op1_map("asin(a)", operation::Asin(), Sigmoid(Div10(N())), dst); - generate_op1_map("atan(a)", operation::Atan(), Div10(N()), dst); - generate_op1_map("exp(a)", operation::Exp(), Div10(N()), dst); - generate_op1_map("log10(a)", operation::Log10(), Div10(N()), dst); - generate_op1_map("log(a)", operation::Log(), Div10(N()), dst); - generate_op1_map("sqrt(a)", operation::Sqrt(), Div10(N()), dst); - generate_op1_map("ceil(a)", operation::Ceil(), Div10(N()), dst); - generate_op1_map("fabs(a)", operation::Fabs(), Div10(N()), dst); - generate_op1_map("floor(a)", operation::Floor(), Div10(N()), dst); - generate_op1_map("isNan(a)", operation::IsNan(), Mask2Seq(SkipNth(3), 1.0, my_nan), dst); - generate_op1_map("relu(a)", operation::Relu(), Sub2(Div10(N())), dst); - generate_op1_map("sigmoid(a)", operation::Sigmoid(), Sub2(Div10(N())), dst); - generate_map_expr("map(a,f(a)((a+1)*2))", MyOp(), Div10(N()), dst); + generate_op1_map("-a", operation::Neg::f, Sub2(Div10(N())), dst); + generate_op1_map("!a", operation::Not::f, Mask2Seq(SkipNth(3)), dst); + generate_op1_map("cos(a)", operation::Cos::f, Div10(N()), dst); + generate_op1_map("sin(a)", operation::Sin::f, Div10(N()), dst); + generate_op1_map("tan(a)", operation::Tan::f, Div10(N()), dst); + generate_op1_map("cosh(a)", operation::Cosh::f, Div10(N()), dst); + generate_op1_map("sinh(a)", operation::Sinh::f, Div10(N()), dst); + generate_op1_map("tanh(a)", operation::Tanh::f, Div10(N()), dst); + generate_op1_map("acos(a)", operation::Acos::f, Sigmoid(Div10(N())), dst); + generate_op1_map("asin(a)", operation::Asin::f, Sigmoid(Div10(N())), dst); + generate_op1_map("atan(a)", operation::Atan::f, Div10(N()), dst); + generate_op1_map("exp(a)", operation::Exp::f, Div10(N()), dst); + generate_op1_map("log10(a)", operation::Log10::f, Div10(N()), dst); + generate_op1_map("log(a)", operation::Log::f, Div10(N()), dst); + generate_op1_map("sqrt(a)", operation::Sqrt::f, Div10(N()), dst); + generate_op1_map("ceil(a)", operation::Ceil::f, Div10(N()), dst); + generate_op1_map("fabs(a)", operation::Fabs::f, Div10(N()), dst); + generate_op1_map("floor(a)", operation::Floor::f, Div10(N()), dst); + generate_op1_map("isNan(a)", operation::IsNan::f, Mask2Seq(SkipNth(3), 1.0, my_nan), dst); + generate_op1_map("relu(a)", operation::Relu::f, Sub2(Div10(N())), dst); + generate_op1_map("sigmoid(a)", operation::Sigmoid::f, Sub2(Div10(N())), dst); + generate_map_expr("map(a,f(a)((a+1)*2))", MyOp::f, Div10(N()), dst); } //----------------------------------------------------------------------------- diff --git a/eval/src/tests/eval/compiled_function/compiled_function_test.cpp b/eval/src/tests/eval/compiled_function/compiled_function_test.cpp index 357baa4d5c5..b887c6e45f9 100644 --- a/eval/src/tests/eval/compiled_function/compiled_function_test.cpp +++ b/eval/src/tests/eval/compiled_function/compiled_function_test.cpp @@ -49,7 +49,6 @@ TEST("require that lazy parameter passing works") { //----------------------------------------------------------------------------- std::vector<vespalib::string> unsupported = { - "sum(", "map(", "join(", "reduce(", diff --git a/eval/src/tests/eval/function/function_test.cpp b/eval/src/tests/eval/function/function_test.cpp index 621f68ffc62..eae58b04178 100644 --- a/eval/src/tests/eval/function/function_test.cpp +++ b/eval/src/tests/eval/function/function_test.cpp @@ -750,15 +750,9 @@ TEST("require that missing value gives parse error") { //----------------------------------------------------------------------------- -TEST("require that tensor sum can be parsed") { - EXPECT_EQUAL("sum(a)", Function::parse("sum(a)").dump()); - EXPECT_EQUAL("sum(a)", Function::parse(" sum ( a ) ").dump()); - EXPECT_EQUAL("sum(a,dim)", Function::parse("sum(a,dim)").dump()); - EXPECT_EQUAL("sum(a,dim)", Function::parse(" sum ( a , dim ) ").dump()); -} - TEST("require that tensor operations can be nested") { - EXPECT_EQUAL("sum(sum(sum(a)),dim)", Function::parse("sum(sum(sum(a)),dim)").dump()); + EXPECT_EQUAL("reduce(reduce(reduce(a,sum),sum),sum,dim)", + Function::parse("reduce(reduce(reduce(a,sum),sum),sum,dim)").dump()); } //----------------------------------------------------------------------------- @@ -795,6 +789,7 @@ TEST("require that tensor reduce can be parsed") { EXPECT_EQUAL("reduce(x,sum,a,b)", Function::parse({"x"}, "reduce(x,sum,a,b)").dump()); EXPECT_EQUAL("reduce(x,sum,a,b,c)", Function::parse({"x"}, "reduce(x,sum,a,b,c)").dump()); EXPECT_EQUAL("reduce(x,sum,a,b,c)", Function::parse({"x"}, " reduce ( x , sum , a , b , c ) ").dump()); + EXPECT_EQUAL("reduce(x,sum)", Function::parse({"x"}, "reduce(x,sum)").dump()); EXPECT_EQUAL("reduce(x,avg)", Function::parse({"x"}, "reduce(x,avg)").dump()); EXPECT_EQUAL("reduce(x,avg)", Function::parse({"x"}, "reduce( x , avg )").dump()); EXPECT_EQUAL("reduce(x,count)", Function::parse({"x"}, "reduce(x,count)").dump()); @@ -803,11 +798,6 @@ TEST("require that tensor reduce can be parsed") { EXPECT_EQUAL("reduce(x,max)", Function::parse({"x"}, "reduce(x,max)").dump()); } -TEST("require that tensor reduce is mapped to tensor sum for all dimensions/single dimension") { - EXPECT_EQUAL("sum(x)", Function::parse({"x"}, "reduce(x,sum)").dump()); - EXPECT_EQUAL("sum(x,d)", Function::parse({"x"}, "reduce(x,sum,d)").dump()); -} - TEST("require that tensor reduce with unknown aggregator fails") { verify_error("reduce(x,bogus)", "[reduce(x,bogus]...[unknown aggregator: 'bogus']...[)]"); } diff --git a/eval/src/tests/eval/interpreted_function/interpreted_function_test.cpp b/eval/src/tests/eval/interpreted_function/interpreted_function_test.cpp index 4497c5f5e70..0e548a3b82e 100644 --- a/eval/src/tests/eval/interpreted_function/interpreted_function_test.cpp +++ b/eval/src/tests/eval/interpreted_function/interpreted_function_test.cpp @@ -5,6 +5,7 @@ #include <vespa/eval/eval/interpreted_function.h> #include <vespa/eval/eval/test/eval_spec.h> #include <vespa/eval/eval/basic_nodes.h> +#include <vespa/eval/tensor/default_tensor_engine.h> #include <vespa/vespalib/util/stringfmt.h> #include <vespa/vespalib/util/stash.h> #include <vespa/vespalib/test/insertion_operators.h> @@ -12,6 +13,7 @@ using namespace vespalib::eval; using vespalib::Stash; +using vespalib::tensor::DefaultTensorEngine; //----------------------------------------------------------------------------- @@ -20,6 +22,7 @@ struct MyEvalTest : test::EvalSpec::EvalTest { size_t fail_cnt = 0; bool print_pass = false; bool print_fail = false; + virtual void next_expression(const std::vector<vespalib::string> ¶m_names, const vespalib::string &expression) override { @@ -35,6 +38,7 @@ struct MyEvalTest : test::EvalSpec::EvalTest { ++fail_cnt; } } + virtual void handle_case(const std::vector<vespalib::string> ¶m_names, const std::vector<double> ¶m_values, const vespalib::string &expression, @@ -45,23 +49,34 @@ struct MyEvalTest : test::EvalSpec::EvalTest { bool is_supported = true; bool has_issues = InterpretedFunction::detect_issues(function); if (is_supported && !has_issues) { - InterpretedFunction ifun(SimpleTensorEngine::ref(), function, NodeTypes()); - ASSERT_EQUAL(ifun.num_params(), param_values.size()); - InterpretedFunction::Context ictx(ifun); + vespalib::string desc = as_string(param_names, param_values, expression); InterpretedFunction::SimpleParams params(param_values); - const Value &result_value = ifun.eval(ictx, params); - double result = result_value.as_double(); - if (result_value.is_double() && is_same(expected_result, result)) { - print_pass && fprintf(stderr, "verifying: %s -> %g ... PASS\n", - as_string(param_names, param_values, expression).c_str(), - expected_result); - ++pass_cnt; - } else { - print_fail && fprintf(stderr, "verifying: %s -> %g ... FAIL: got %g\n", - as_string(param_names, param_values, expression).c_str(), - expected_result, result); - ++fail_cnt; - } + verify_result(SimpleTensorEngine::ref(), function, "[simple] "+desc, params, expected_result); + verify_result(DefaultTensorEngine::ref(), function, " [prod] "+desc, params, expected_result); + } + } + + void verify_result(const TensorEngine& engine, + const Function &function, + const vespalib::string &description, + const InterpretedFunction::SimpleParams ¶ms, + double expected_result) + { + InterpretedFunction ifun(engine, function, NodeTypes()); + ASSERT_EQUAL(ifun.num_params(), params.params.size()); + InterpretedFunction::Context ictx(ifun); + const Value &result_value = ifun.eval(ictx, params); + double result = result_value.as_double(); + if (result_value.is_double() && is_same(expected_result, result)) { + print_pass && fprintf(stderr, "verifying: %s -> %g ... PASS\n", + description.c_str(), + expected_result); + ++pass_cnt; + } else { + print_fail && fprintf(stderr, "verifying: %s -> %g ... FAIL: got %g\n", + description.c_str(), + expected_result, result); + ++fail_cnt; } } }; @@ -114,6 +129,10 @@ TEST("require that interpreted function instructions have expected size") { EXPECT_EQUAL(sizeof(InterpretedFunction::Instruction), 16u); } +TEST("require that function pointers can be passed as instruction parameters") { + EXPECT_EQUAL(sizeof(&operation::Add::f), sizeof(uint64_t)); +} + TEST("require that basic addition works") { Function function = Function::parse("a+10"); InterpretedFunction interpreted(SimpleTensorEngine::ref(), function, NodeTypes()); @@ -128,7 +147,7 @@ TEST("require that basic addition works") { TEST("require that dot product like expression is not optimized for unknown types") { const TensorEngine &engine = SimpleTensorEngine::ref(); - Function function = Function::parse("sum(a*b)"); + Function function = Function::parse("reduce(a*b,sum)"); DoubleValue a(2.0); DoubleValue b(3.0); double expect = (2.0 * 3.0); @@ -143,7 +162,7 @@ TEST("require that dot product like expression is not optimized for unknown type TEST("require that dot product works with tensor function") { const TensorEngine &engine = SimpleTensorEngine::ref(); - Function function = Function::parse("sum(a*b)"); + Function function = Function::parse("reduce(a*b,sum)"); auto a = TensorSpec("tensor(x[3])") .add({{"x", 0}}, 5.0) .add({{"x", 1}}, 3.0) @@ -167,7 +186,7 @@ TEST("require that dot product works with tensor function") { TEST("require that matrix multiplication works with tensor function") { const TensorEngine &engine = SimpleTensorEngine::ref(); - Function function = Function::parse("sum(a*b,y)"); + Function function = Function::parse("reduce(a*b,sum,y)"); auto a = TensorSpec("tensor(x[2],y[2])") .add({{"x", 0},{"y", 0}}, 1.0) .add({{"x", 0},{"y", 1}}, 2.0) diff --git a/eval/src/tests/eval/node_types/node_types_test.cpp b/eval/src/tests/eval/node_types/node_types_test.cpp index 555c2bd2ba4..f6afa543402 100644 --- a/eval/src/tests/eval/node_types/node_types_test.cpp +++ b/eval/src/tests/eval/node_types/node_types_test.cpp @@ -129,23 +129,6 @@ TEST("require that set membership resolves to double unless error") { TEST_DO(verify("any in [tensor,error,any]", "error")); } -TEST("require that sum resolves correct type") { - TEST_DO(verify("sum(error)", "error")); - TEST_DO(verify("sum(tensor)", "double")); - TEST_DO(verify("sum(double)", "double")); - TEST_DO(verify("sum(any)", "any")); -} - -TEST("require that dimension sum resolves correct type") { - TEST_DO(verify("sum(error,x)", "error")); - TEST_DO(verify("sum(tensor,x)", "any")); - TEST_DO(verify("sum(any,x)", "any")); - TEST_DO(verify("sum(double,x)", "error")); - TEST_DO(verify("sum(tensor(x{},y{},z{}),y)", "tensor(x{},z{})")); - TEST_DO(verify("sum(tensor(x{},y{},z{}),w)", "error")); - TEST_DO(verify("sum(tensor(x{}),x)", "double")); -} - TEST("require that reduce resolves correct type") { TEST_DO(verify("reduce(error,sum)", "error")); TEST_DO(verify("reduce(tensor,sum)", "double")); @@ -291,7 +274,7 @@ TEST("require that tensor concat resolves correct type") { TEST("require that double only expressions can be detected") { Function plain_fun = Function::parse("1+2"); - Function complex_fun = Function::parse("sum(a)"); + Function complex_fun = Function::parse("reduce(a,sum)"); NodeTypes plain_types(plain_fun, {}); NodeTypes complex_types(complex_fun, {ValueType::tensor_type({})}); EXPECT_TRUE(plain_types.get_type(plain_fun.root()).is_double()); diff --git a/eval/src/tests/tensor/tensor_performance/tensor_performance_test.cpp b/eval/src/tests/tensor/tensor_performance/tensor_performance_test.cpp index 735e9c9ab34..3daaa3f79b3 100644 --- a/eval/src/tests/tensor/tensor_performance/tensor_performance_test.cpp +++ b/eval/src/tests/tensor/tensor_performance/tensor_performance_test.cpp @@ -18,10 +18,10 @@ using namespace vespalib::tensor; //----------------------------------------------------------------------------- -const vespalib::string dot_product_match_expr = "sum(query*document)"; -const vespalib::string dot_product_multiply_expr = "sum(query*document)"; -const vespalib::string model_match_expr = "sum((query*document)*model)"; -const vespalib::string matrix_product_expr = "sum(sum((query+document)*model,x))"; +const vespalib::string dot_product_match_expr = "reduce(query*document,sum)"; +const vespalib::string dot_product_multiply_expr = "reduce(query*document,sum)"; +const vespalib::string model_match_expr = "reduce((query*document)*model,sum)"; +const vespalib::string matrix_product_expr = "reduce(reduce((query+document)*model,sum,x),sum)"; //----------------------------------------------------------------------------- diff --git a/eval/src/vespa/eval/eval/function.cpp b/eval/src/vespa/eval/eval/function.cpp index cb3d157c06f..58d61d33ea6 100644 --- a/eval/src/vespa/eval/eval/function.cpp +++ b/eval/src/vespa/eval/eval/function.cpp @@ -587,13 +587,7 @@ void parse_tensor_reduce(ParseContext &ctx) { return; } auto dimensions = get_ident_list(ctx, false); - if ((*maybe_aggr == Aggr::SUM) && dimensions.empty()) { - ctx.push_expression(std::make_unique<nodes::TensorSum>(std::move(child))); - } else if ((*maybe_aggr == Aggr::SUM) && (dimensions.size() == 1)) { - ctx.push_expression(std::make_unique<nodes::TensorSum>(std::move(child), dimensions[0])); - } else { - ctx.push_expression(std::make_unique<nodes::TensorReduce>(std::move(child), *maybe_aggr, std::move(dimensions))); - } + ctx.push_expression(std::make_unique<nodes::TensorReduce>(std::move(child), *maybe_aggr, std::move(dimensions))); } void parse_tensor_rename(ParseContext &ctx) { @@ -648,20 +642,6 @@ void parse_tensor_concat(ParseContext &ctx) { ctx.push_expression(std::make_unique<nodes::TensorConcat>(std::move(lhs), std::move(rhs), dimension)); } -// to be replaced with more generic 'reduce' -void parse_tensor_sum(ParseContext &ctx) { - parse_expression(ctx); - Node_UP child = ctx.pop_expression(); - if (ctx.get() == ',') { - ctx.next(); - vespalib::string dimension = get_ident(ctx, false); - ctx.skip_spaces(); - ctx.push_expression(Node_UP(new nodes::TensorSum(std::move(child), dimension))); - } else { - ctx.push_expression(Node_UP(new nodes::TensorSum(std::move(child)))); - } -} - bool try_parse_call(ParseContext &ctx, const vespalib::string &name) { ctx.skip_spaces(); if (ctx.get() == '(') { @@ -686,8 +666,6 @@ bool try_parse_call(ParseContext &ctx, const vespalib::string &name) { parse_tensor_lambda(ctx); } else if (name == "concat") { parse_tensor_concat(ctx); - } else if (name == "sum") { - parse_tensor_sum(ctx); } else { ctx.fail(make_string("unknown function: '%s'", name.c_str())); return false; diff --git a/eval/src/vespa/eval/eval/interpreted_function.cpp b/eval/src/vespa/eval/eval/interpreted_function.cpp index e6e11dbd7c3..09cc2d57e80 100644 --- a/eval/src/vespa/eval/eval/interpreted_function.cpp +++ b/eval/src/vespa/eval/eval/interpreted_function.cpp @@ -18,6 +18,8 @@ namespace { using namespace nodes; using State = InterpretedFunction::State; using Instruction = InterpretedFunction::Instruction; +using map_fun_t = double (*)(double); +using join_fun_t = double (*)(double, double); //----------------------------------------------------------------------------- @@ -32,6 +34,13 @@ const T &unwrap_param(uint64_t param) { return *((const T *)param); } //----------------------------------------------------------------------------- +uint64_t to_param(map_fun_t value) { return (uint64_t)value; } +uint64_t to_param(join_fun_t value) { return (uint64_t)value; } +map_fun_t to_map_fun(uint64_t param) { return (map_fun_t)param; } +join_fun_t to_join_fun(uint64_t param) { return (join_fun_t)param; } + +//----------------------------------------------------------------------------- + void op_load_const(State &state, uint64_t param) { state.stack.push_back(unwrap_param<Value>(param)); } @@ -46,18 +55,6 @@ void op_load_let(State &state, uint64_t param) { //----------------------------------------------------------------------------- -template <typename OP1> -void op_unary(State &state, uint64_t) { - state.replace(1, state.engine.map(state.peek(0), OP1::f, state.stash)); -} - -template <typename OP2> -void op_binary(State &state, uint64_t) { - state.replace(2, state.engine.join(state.peek(1), state.peek(0), OP2::f, state.stash)); -} - -//----------------------------------------------------------------------------- - void op_skip(State &state, uint64_t param) { state.program_offset += param; } @@ -101,25 +98,12 @@ void op_not_member(State &state, uint64_t) { //----------------------------------------------------------------------------- -void op_tensor_sum(State &state, uint64_t) { - state.replace(1, state.engine.reduce(state.peek(0), Aggr::SUM, {}, state.stash)); -} - -void op_tensor_sum_dimension(State &state, uint64_t param) { - const vespalib::string &dimension = unwrap_param<vespalib::string>(param); - state.replace(1, state.engine.reduce(state.peek(0), Aggr::SUM, {dimension}, state.stash)); -} - -//----------------------------------------------------------------------------- - void op_tensor_map(State &state, uint64_t param) { - const CompiledFunction &cfun = unwrap_param<CompiledFunction>(param); - state.replace(1, state.engine.map(state.peek(0), cfun.get_function<1>(), state.stash)); + state.replace(1, state.engine.map(state.peek(0), to_map_fun(param), state.stash)); } void op_tensor_join(State &state, uint64_t param) { - const CompiledFunction &cfun = unwrap_param<CompiledFunction>(param); - state.replace(2, state.engine.join(state.peek(1), state.peek(0), cfun.get_function<2>(), state.stash)); + state.replace(2, state.engine.join(state.peek(1), state.peek(0), to_join_fun(param), state.stash)); } using ReduceParams = std::pair<Aggr,std::vector<vespalib::string>>; @@ -227,10 +211,27 @@ struct ProgramBuilder : public NodeVisitor, public NodeTraverser { //------------------------------------------------------------------------- - void visit(const Number&node) override { - program.emplace_back(op_load_const, wrap_param<Value>(stash.create<DoubleValue>(node.value()))); + void make_const_op(const Node &node, const Value &value) { + (void) node; + program.emplace_back(op_load_const, wrap_param<Value>(value)); + } + + void make_map_op(const Node &node, map_fun_t function) { + (void) node; + program.emplace_back(op_tensor_map, to_param(function)); + } + + void make_join_op(const Node &node, join_fun_t function) { + (void) node; + program.emplace_back(op_tensor_join, to_param(function)); + } + + //------------------------------------------------------------------------- + + void visit(const Number &node) override { + make_const_op(node, stash.create<DoubleValue>(node.value())); } - void visit(const Symbol&node) override { + void visit(const Symbol &node) override { if (node.id() >= 0) { // param value program.emplace_back(op_load_param, node.id()); } else { // let binding @@ -238,19 +239,19 @@ struct ProgramBuilder : public NodeVisitor, public NodeTraverser { program.emplace_back(op_load_let, let_offset); } } - void visit(const String&node) override { - program.emplace_back(op_load_const, wrap_param<Value>(stash.create<DoubleValue>(node.hash()))); + void visit(const String &node) override { + make_const_op(node, stash.create<DoubleValue>(node.hash())); } - void visit(const Array&node) override { - program.emplace_back(op_load_const, wrap_param<Value>(stash.create<DoubleValue>(node.size()))); + void visit(const Array &node) override { + make_const_op(node, stash.create<DoubleValue>(node.size())); } - void visit(const Neg &) override { - program.emplace_back(op_unary<operation::Neg>); + void visit(const Neg &node) override { + make_map_op(node, operation::Neg::f); } - void visit(const Not &) override { - program.emplace_back(op_unary<operation::Not>); + void visit(const Not &node) override { + make_map_op(node, operation::Not::f); } - void visit(const If&node) override { + void visit(const If &node) override { node.cond().traverse(*this); size_t after_cond = program.size(); program.emplace_back(op_skip_if_false); @@ -261,58 +262,48 @@ struct ProgramBuilder : public NodeVisitor, public NodeTraverser { program[after_cond].update_param(after_true - after_cond); program[after_true].update_param(program.size() - after_true - 1); } - void visit(const Let&node) override { + void visit(const Let &node) override { node.value().traverse(*this); program.emplace_back(op_store_let); node.expr().traverse(*this); program.emplace_back(op_evict_let); } - void visit(const Error &) override { - program.emplace_back(op_load_const, wrap_param<Value>(stash.create<ErrorValue>())); + void visit(const Error &node) override { + make_const_op(node, ErrorValue::instance); + } + void visit(const TensorMap &node) override { + const auto &token = stash.create<CompileCache::Token::UP>(CompileCache::compile(node.lambda(), PassParams::SEPARATE)); + make_map_op(node, token.get()->get().get_function<1>()); } - void visit(const TensorSum&node) override { - if (is_typed(node) && is_typed_tensor_product_of_params(node.get_child(0))) { + void visit(const TensorJoin &node) override { + const auto &token = stash.create<CompileCache::Token::UP>(CompileCache::compile(node.lambda(), PassParams::SEPARATE)); + make_join_op(node, token.get()->get().get_function<2>()); + } + void visit(const TensorReduce &node) override { + if ((node.aggr() == Aggr::SUM) && is_typed(node) && is_typed_tensor_product_of_params(node.get_child(0))) { assert(program.size() >= 3); // load,load,mul program.pop_back(); // mul program.pop_back(); // load program.pop_back(); // load - std::vector<vespalib::string> dim_list; - if (!node.dimension().empty()) { - dim_list.push_back(node.dimension()); - } auto a = as<Symbol>(node.get_child(0).get_child(0)); auto b = as<Symbol>(node.get_child(0).get_child(1)); auto ir = tensor_function::reduce(tensor_function::join( tensor_function::inject(types.get_type(*a), 0), tensor_function::inject(types.get_type(*b), 1), - operation::Mul::f), Aggr::SUM, dim_list); + operation::Mul::f), node.aggr(), node.dimensions()); auto fun = tensor_engine.compile(std::move(ir)); const auto &meta = stash.create<TensorFunctionArgArgMeta>(std::move(fun), a->id(), b->id()); program.emplace_back(op_tensor_function_arg_arg, wrap_param<TensorFunctionArgArgMeta>(meta)); - } else if (node.dimension().empty()) { - program.emplace_back(op_tensor_sum); } else { - program.emplace_back(op_tensor_sum_dimension, - wrap_param<vespalib::string>(stash.create<vespalib::string>(node.dimension()))); + ReduceParams ¶ms = stash.create<ReduceParams>(node.aggr(), node.dimensions()); + program.emplace_back(op_tensor_reduce, wrap_param<ReduceParams>(params)); } } - void visit(const TensorMap&node) override { - const auto &token = stash.create<CompileCache::Token::UP>(CompileCache::compile(node.lambda(), PassParams::SEPARATE)); - program.emplace_back(op_tensor_map, wrap_param<CompiledFunction>(token.get()->get())); - } - void visit(const TensorJoin&node) override { - const auto &token = stash.create<CompileCache::Token::UP>(CompileCache::compile(node.lambda(), PassParams::SEPARATE)); - program.emplace_back(op_tensor_join, wrap_param<CompiledFunction>(token.get()->get())); - } - void visit(const TensorReduce&node) override { - ReduceParams ¶ms = stash.create<ReduceParams>(node.aggr(), node.dimensions()); - program.emplace_back(op_tensor_reduce, wrap_param<ReduceParams>(params)); - } - void visit(const TensorRename&node) override { + void visit(const TensorRename &node) override { RenameParams ¶ms = stash.create<RenameParams>(node.from(), node.to()); program.emplace_back(op_tensor_rename, wrap_param<RenameParams>(params)); } - void visit(const TensorLambda&node) override { + void visit(const TensorLambda &node) override { const auto &type = node.type(); TensorSpec spec(type.to_spec()); const auto &token = stash.create<CompileCache::Token::UP>(CompileCache::compile(node.lambda(), PassParams::ARRAY)); @@ -327,52 +318,52 @@ struct ProgramBuilder : public NodeVisitor, public NodeTraverser { spec.add(addr, fun(¶ms[0])); } while (step_labels(params, type)); auto tensor = tensor_engine.create(spec); - program.emplace_back(op_load_const, wrap_param<Value>(stash.create<TensorValue>(std::move(tensor)))); + make_const_op(node, stash.create<TensorValue>(std::move(tensor))); } - void visit(const TensorConcat&node) override { + void visit(const TensorConcat &node) override { vespalib::string &dimension = stash.create<vespalib::string>(node.dimension()); program.emplace_back(op_tensor_concat, wrap_param<vespalib::string>(dimension)); } - void visit(const Add &) override { - program.emplace_back(op_binary<operation::Add>); + void visit(const Add &node) override { + make_join_op(node, operation::Add::f); } - void visit(const Sub &) override { - program.emplace_back(op_binary<operation::Sub>); + void visit(const Sub &node) override { + make_join_op(node, operation::Sub::f); } - void visit(const Mul &) override { - program.emplace_back(op_binary<operation::Mul>); + void visit(const Mul &node) override { + make_join_op(node, operation::Mul::f); } - void visit(const Div &) override { - program.emplace_back(op_binary<operation::Div>); + void visit(const Div &node) override { + make_join_op(node, operation::Div::f); } - void visit(const Mod &) override { - program.emplace_back(op_binary<operation::Mod>); + void visit(const Mod &node) override { + make_join_op(node, operation::Mod::f); } - void visit(const Pow &) override { - program.emplace_back(op_binary<operation::Pow>); + void visit(const Pow &node) override { + make_join_op(node, operation::Pow::f); } - void visit(const Equal &) override { - program.emplace_back(op_binary<operation::Equal>); + void visit(const Equal &node) override { + make_join_op(node, operation::Equal::f); } - void visit(const NotEqual &) override { - program.emplace_back(op_binary<operation::NotEqual>); + void visit(const NotEqual &node) override { + make_join_op(node, operation::NotEqual::f); } - void visit(const Approx &) override { - program.emplace_back(op_binary<operation::Approx>); + void visit(const Approx &node) override { + make_join_op(node, operation::Approx::f); } - void visit(const Less &) override { - program.emplace_back(op_binary<operation::Less>); + void visit(const Less &node) override { + make_join_op(node, operation::Less::f); } - void visit(const LessEqual &) override { - program.emplace_back(op_binary<operation::LessEqual>); + void visit(const LessEqual &node) override { + make_join_op(node, operation::LessEqual::f); } - void visit(const Greater &) override { - program.emplace_back(op_binary<operation::Greater>); + void visit(const Greater &node) override { + make_join_op(node, operation::Greater::f); } - void visit(const GreaterEqual &) override { - program.emplace_back(op_binary<operation::GreaterEqual>); + void visit(const GreaterEqual &node) override { + make_join_op(node, operation::GreaterEqual::f); } - void visit(const In&node) override { + void visit(const In &node) override { std::vector<size_t> checks; node.lhs().traverse(*this); auto array = as<Array>(node.rhs()); @@ -392,91 +383,91 @@ struct ProgramBuilder : public NodeVisitor, public NodeTraverser { } program.emplace_back(op_not_member); } - void visit(const And &) override { - program.emplace_back(op_binary<operation::And>); + void visit(const And &node) override { + make_join_op(node, operation::And::f); } - void visit(const Or &) override { - program.emplace_back(op_binary<operation::Or>); + void visit(const Or &node) override { + make_join_op(node, operation::Or::f); } - void visit(const Cos &) override { - program.emplace_back(op_unary<operation::Cos>); + void visit(const Cos &node) override { + make_map_op(node, operation::Cos::f); } - void visit(const Sin &) override { - program.emplace_back(op_unary<operation::Sin>); + void visit(const Sin &node) override { + make_map_op(node, operation::Sin::f); } - void visit(const Tan &) override { - program.emplace_back(op_unary<operation::Tan>); + void visit(const Tan &node) override { + make_map_op(node, operation::Tan::f); } - void visit(const Cosh &) override { - program.emplace_back(op_unary<operation::Cosh>); + void visit(const Cosh &node) override { + make_map_op(node, operation::Cosh::f); } - void visit(const Sinh &) override { - program.emplace_back(op_unary<operation::Sinh>); + void visit(const Sinh &node) override { + make_map_op(node, operation::Sinh::f); } - void visit(const Tanh &) override { - program.emplace_back(op_unary<operation::Tanh>); + void visit(const Tanh &node) override { + make_map_op(node, operation::Tanh::f); } - void visit(const Acos &) override { - program.emplace_back(op_unary<operation::Acos>); + void visit(const Acos &node) override { + make_map_op(node, operation::Acos::f); } - void visit(const Asin &) override { - program.emplace_back(op_unary<operation::Asin>); + void visit(const Asin &node) override { + make_map_op(node, operation::Asin::f); } - void visit(const Atan &) override { - program.emplace_back(op_unary<operation::Atan>); + void visit(const Atan &node) override { + make_map_op(node, operation::Atan::f); } - void visit(const Exp &) override { - program.emplace_back(op_unary<operation::Exp>); + void visit(const Exp &node) override { + make_map_op(node, operation::Exp::f); } - void visit(const Log10 &) override { - program.emplace_back(op_unary<operation::Log10>); + void visit(const Log10 &node) override { + make_map_op(node, operation::Log10::f); } - void visit(const Log &) override { - program.emplace_back(op_unary<operation::Log>); + void visit(const Log &node) override { + make_map_op(node, operation::Log::f); } - void visit(const Sqrt &) override { - program.emplace_back(op_unary<operation::Sqrt>); + void visit(const Sqrt &node) override { + make_map_op(node, operation::Sqrt::f); } - void visit(const Ceil &) override { - program.emplace_back(op_unary<operation::Ceil>); + void visit(const Ceil &node) override { + make_map_op(node, operation::Ceil::f); } - void visit(const Fabs &) override { - program.emplace_back(op_unary<operation::Fabs>); + void visit(const Fabs &node) override { + make_map_op(node, operation::Fabs::f); } - void visit(const Floor &) override { - program.emplace_back(op_unary<operation::Floor>); + void visit(const Floor &node) override { + make_map_op(node, operation::Floor::f); } - void visit(const Atan2 &) override { - program.emplace_back(op_binary<operation::Atan2>); + void visit(const Atan2 &node) override { + make_join_op(node, operation::Atan2::f); } - void visit(const Ldexp &) override { - program.emplace_back(op_binary<operation::Ldexp>); + void visit(const Ldexp &node) override { + make_join_op(node, operation::Ldexp::f); } - void visit(const Pow2 &) override { - program.emplace_back(op_binary<operation::Pow>); + void visit(const Pow2 &node) override { + make_join_op(node, operation::Pow::f); } - void visit(const Fmod &) override { - program.emplace_back(op_binary<operation::Mod>); + void visit(const Fmod &node) override { + make_join_op(node, operation::Mod::f); } - void visit(const Min &) override { - program.emplace_back(op_binary<operation::Min>); + void visit(const Min &node) override { + make_join_op(node, operation::Min::f); } - void visit(const Max &) override { - program.emplace_back(op_binary<operation::Max>); + void visit(const Max &node) override { + make_join_op(node, operation::Max::f); } - void visit(const IsNan &) override { - program.emplace_back(op_unary<operation::IsNan>); + void visit(const IsNan &node) override { + make_map_op(node, operation::IsNan::f); } - void visit(const Relu &) override { - program.emplace_back(op_unary<operation::Relu>); + void visit(const Relu &node) override { + make_map_op(node, operation::Relu::f); } - void visit(const Sigmoid &) override { - program.emplace_back(op_unary<operation::Sigmoid>); + void visit(const Sigmoid &node) override { + make_map_op(node, operation::Sigmoid::f); } //------------------------------------------------------------------------- - bool open(const Node&node) override { + bool open(const Node &node) override { if (check_type<Array, If, Let, In>(node)) { node.accept(*this); return false; @@ -484,7 +475,7 @@ struct ProgramBuilder : public NodeVisitor, public NodeTraverser { return true; } - void close(const Node&node) override { + void close(const Node &node) override { node.accept(*this); } }; diff --git a/eval/src/vespa/eval/eval/key_gen.cpp b/eval/src/vespa/eval/eval/key_gen.cpp index 65be8d172fd..490f1c044ac 100644 --- a/eval/src/vespa/eval/eval/key_gen.cpp +++ b/eval/src/vespa/eval/eval/key_gen.cpp @@ -31,7 +31,6 @@ struct KeyGen : public NodeVisitor, public NodeTraverser { void visit(const If &node) override { add_byte( 7); add_double(node.p_true()); } void visit(const Let &) override { add_byte( 8); } void visit(const Error &) override { add_byte( 9); } - void visit(const TensorSum &) override { add_byte(10); } // dimensions should be part of key void visit(const TensorMap &) override { add_byte(11); } // lambda should be part of key void visit(const TensorJoin &) override { add_byte(12); } // lambda should be part of key void visit(const TensorReduce &) override { add_byte(13); } // aggr/dimensions should be part of key diff --git a/eval/src/vespa/eval/eval/llvm/compiled_function.cpp b/eval/src/vespa/eval/eval/llvm/compiled_function.cpp index fc016f6a1fd..a696f16f849 100644 --- a/eval/src/vespa/eval/eval/llvm/compiled_function.cpp +++ b/eval/src/vespa/eval/eval/llvm/compiled_function.cpp @@ -123,8 +123,7 @@ CompiledFunction::detect_issues(const Function &function) std::vector<vespalib::string> issues; bool open(const nodes::Node &) override { return true; } void close(const nodes::Node &node) override { - if (nodes::check_type<nodes::TensorSum, - nodes::TensorMap, + if (nodes::check_type<nodes::TensorMap, nodes::TensorJoin, nodes::TensorReduce, nodes::TensorRename, diff --git a/eval/src/vespa/eval/eval/llvm/llvm_wrapper.cpp b/eval/src/vespa/eval/eval/llvm/llvm_wrapper.cpp index 30c10464a35..77e23b44799 100644 --- a/eval/src/vespa/eval/eval/llvm/llvm_wrapper.cpp +++ b/eval/src/vespa/eval/eval/llvm/llvm_wrapper.cpp @@ -415,9 +415,6 @@ struct FunctionBuilder : public NodeVisitor, public NodeTraverser { // tensor nodes (not supported in compiled expressions) - void visit(const TensorSum &node) override { - make_error(node.num_children()); - } void visit(const TensorMap &node) override { make_error(node.num_children()); } diff --git a/eval/src/vespa/eval/eval/node_types.cpp b/eval/src/vespa/eval/eval/node_types.cpp index 24dd6196d64..3e942c7284e 100644 --- a/eval/src/vespa/eval/eval/node_types.cpp +++ b/eval/src/vespa/eval/eval/node_types.cpp @@ -152,14 +152,6 @@ struct TypeResolver : public NodeVisitor, public NodeTraverser { void visit(const Error &node) override { bind_type(ValueType::error_type(), node); } - void visit(const TensorSum &node) override { - const ValueType &child = state.peek(0); - if (node.dimension().empty()) { - bind_type(child.reduce({}), node); - } else { - bind_type(child.reduce({node.dimension()}), node); - } - } void visit(const TensorMap &node) override { resolve_op1(node); } void visit(const TensorJoin &node) override { resolve_op2(node); } void visit(const TensorReduce &node) override { diff --git a/eval/src/vespa/eval/eval/node_visitor.h b/eval/src/vespa/eval/eval/node_visitor.h index 1bbcef24107..b12689e9603 100644 --- a/eval/src/vespa/eval/eval/node_visitor.h +++ b/eval/src/vespa/eval/eval/node_visitor.h @@ -30,7 +30,6 @@ struct NodeVisitor { virtual void visit(const nodes::Error &) = 0; // tensor nodes - virtual void visit(const nodes::TensorSum &) = 0; virtual void visit(const nodes::TensorMap &) = 0; virtual void visit(const nodes::TensorJoin &) = 0; virtual void visit(const nodes::TensorReduce &) = 0; @@ -91,16 +90,15 @@ struct NodeVisitor { * of all types not specifically handled. **/ struct EmptyNodeVisitor : NodeVisitor { - virtual void visit(const nodes::Number &) override {} - virtual void visit(const nodes::Symbol &) override {} - virtual void visit(const nodes::String &) override {} - virtual void visit(const nodes::Array &) override {} - virtual void visit(const nodes::Neg &) override {} - virtual void visit(const nodes::Not &) override {} - virtual void visit(const nodes::If &) override {} - virtual void visit(const nodes::Let &) override {} + void visit(const nodes::Number &) override {} + void visit(const nodes::Symbol &) override {} + void visit(const nodes::String &) override {} + void visit(const nodes::Array &) override {} + void visit(const nodes::Neg &) override {} + void visit(const nodes::Not &) override {} + void visit(const nodes::If &) override {} + void visit(const nodes::Let &) override {} void visit(const nodes::Error &) override {} - void visit(const nodes::TensorSum &) override {} void visit(const nodes::TensorMap &) override {} void visit(const nodes::TensorJoin &) override {} void visit(const nodes::TensorReduce &) override {} diff --git a/eval/src/vespa/eval/eval/operation.cpp b/eval/src/vespa/eval/eval/operation.cpp index e417f434536..42b1a110497 100644 --- a/eval/src/vespa/eval/eval/operation.cpp +++ b/eval/src/vespa/eval/eval/operation.cpp @@ -2,41 +2,12 @@ #include "operation.h" #include "value.h" -#include "operation_visitor.h" #include <cmath> #include <assert.h> namespace vespalib { namespace eval { -template <typename T> void Op1<T>::accept(OperationVisitor &visitor) const { - visitor.visit(static_cast<const T&>(*this)); -} - -template <typename T> double Op1<T>::eval(double a) const { - return T::f(a); -} - -template <typename T> Operation::op1_fun_t Op1<T>::get_f() const { - return T::f; -} - -template <typename T> void Op2<T>::accept(OperationVisitor &visitor) const { - visitor.visit(static_cast<const T&>(*this)); -} - -template <typename T> std::unique_ptr<BinaryOperation> Op2<T>::clone() const { - return std::make_unique<T>(); -} - -template <typename T> double Op2<T>::eval(double a, double b) const { - return T::f(a, b); -} - -template <typename T> Operation::op2_fun_t Op2<T>::get_f() const { - return T::f; -} - namespace operation { double Neg::f(double a) { return -a; } double Not::f(double a) { return (a != 0.0) ? 0.0 : 1.0; } @@ -80,47 +51,5 @@ double Relu::f(double a) { return std::max(a, 0.0); } double Sigmoid::f(double a) { return 1.0 / (1.0 + std::exp(-1.0 * a)); } } // namespace vespalib::eval::operation -template struct Op1<operation::Neg>; -template struct Op1<operation::Not>; -template struct Op2<operation::Add>; -template struct Op2<operation::Sub>; -template struct Op2<operation::Mul>; -template struct Op2<operation::Div>; -template struct Op2<operation::Mod>; -template struct Op2<operation::Pow>; -template struct Op2<operation::Equal>; -template struct Op2<operation::NotEqual>; -template struct Op2<operation::Approx>; -template struct Op2<operation::Less>; -template struct Op2<operation::LessEqual>; -template struct Op2<operation::Greater>; -template struct Op2<operation::GreaterEqual>; -template struct Op2<operation::And>; -template struct Op2<operation::Or>; -template struct Op1<operation::Cos>; -template struct Op1<operation::Sin>; -template struct Op1<operation::Tan>; -template struct Op1<operation::Cosh>; -template struct Op1<operation::Sinh>; -template struct Op1<operation::Tanh>; -template struct Op1<operation::Acos>; -template struct Op1<operation::Asin>; -template struct Op1<operation::Atan>; -template struct Op1<operation::Exp>; -template struct Op1<operation::Log10>; -template struct Op1<operation::Log>; -template struct Op1<operation::Sqrt>; -template struct Op1<operation::Ceil>; -template struct Op1<operation::Fabs>; -template struct Op1<operation::Floor>; -template struct Op2<operation::Atan2>; -template struct Op2<operation::Ldexp>; -template struct Op2<operation::Min>; -template struct Op2<operation::Max>; -template struct Op1<operation::IsNan>; -template struct Op1<operation::Relu>; -template struct Op1<operation::Sigmoid>; -template struct Op1<CustomUnaryOperation>; - } // namespace vespalib::eval } // namespace vespalib diff --git a/eval/src/vespa/eval/eval/operation.h b/eval/src/vespa/eval/eval/operation.h index 6395412731c..12de7c3deb7 100644 --- a/eval/src/vespa/eval/eval/operation.h +++ b/eval/src/vespa/eval/eval/operation.h @@ -10,118 +10,47 @@ namespace vespalib { namespace eval { -struct OperationVisitor; - -/** - * An Operation represents the action taken based on what is described - * by an Operator or a Call AST node. All operations have underlying - * numeric meaning (that can be overridden for complex value - * types). They no longer have any textual counterpart and are only - * separated by the number of values they operate on. - **/ -struct Operation { - using op1_fun_t = double (*)(double); - using op2_fun_t = double (*)(double, double); - virtual void accept(OperationVisitor &visitor) const = 0; - virtual ~Operation() {} -}; - -/** - * Simple typecasting utility. - */ -template <typename T> -const T *as(const Operation &op) { return dynamic_cast<const T *>(&op); } - -//----------------------------------------------------------------------------- - -/** - * An Operation performing a calculation based on a single input - * value. - **/ -struct UnaryOperation : Operation { - virtual double eval(double a) const = 0; - virtual op1_fun_t get_f() const = 0; -}; - -/** - * An Operation performing a calculation based on two input values. - **/ -struct BinaryOperation : Operation { - virtual double eval(double a, double b) const = 0; - virtual std::unique_ptr<BinaryOperation> clone() const = 0; - virtual op2_fun_t get_f() const = 0; -}; - -//----------------------------------------------------------------------------- - -template <typename T> -struct Op1 : UnaryOperation { - virtual void accept(OperationVisitor &visitor) const override; - virtual double eval(double a) const override; - virtual op1_fun_t get_f() const override; -}; - -template <typename T> -struct Op2 : BinaryOperation { - virtual void accept(OperationVisitor &visitor) const override; - virtual std::unique_ptr<BinaryOperation> clone() const override; - virtual double eval(double a, double b) const final override; - virtual op2_fun_t get_f() const override; -}; - -//----------------------------------------------------------------------------- - -/** - * A non-trivial custom unary operation. Typically used for closures - * and lambdas. - **/ -struct CustomUnaryOperation : Op1<CustomUnaryOperation> { - static double f(double) { return error_value; } -}; - -//----------------------------------------------------------------------------- - namespace operation { -struct Neg : Op1<Neg> { static double f(double a); }; -struct Not : Op1<Not> { static double f(double a); }; -struct Add : Op2<Add> { static double f(double a, double b); }; -struct Sub : Op2<Sub> { static double f(double a, double b); }; -struct Mul : Op2<Mul> { static double f(double a, double b); }; -struct Div : Op2<Div> { static double f(double a, double b); }; -struct Mod : Op2<Mod> { static double f(double a, double b); }; -struct Pow : Op2<Pow> { static double f(double a, double b); }; -struct Equal : Op2<Equal> { static double f(double a, double b); }; -struct NotEqual : Op2<NotEqual> { static double f(double a, double b); }; -struct Approx : Op2<Approx> { static double f(double a, double b); }; -struct Less : Op2<Less> { static double f(double a, double b); }; -struct LessEqual : Op2<LessEqual> { static double f(double a, double b); }; -struct Greater : Op2<Greater> { static double f(double a, double b); }; -struct GreaterEqual : Op2<GreaterEqual> { static double f(double a, double b); }; -struct And : Op2<And> { static double f(double a, double b); }; -struct Or : Op2<Or> { static double f(double a, double b); }; -struct Cos : Op1<Cos> { static double f(double a); }; -struct Sin : Op1<Sin> { static double f(double a); }; -struct Tan : Op1<Tan> { static double f(double a); }; -struct Cosh : Op1<Cosh> { static double f(double a); }; -struct Sinh : Op1<Sinh> { static double f(double a); }; -struct Tanh : Op1<Tanh> { static double f(double a); }; -struct Acos : Op1<Acos> { static double f(double a); }; -struct Asin : Op1<Asin> { static double f(double a); }; -struct Atan : Op1<Atan> { static double f(double a); }; -struct Exp : Op1<Exp> { static double f(double a); }; -struct Log10 : Op1<Log10> { static double f(double a); }; -struct Log : Op1<Log> { static double f(double a); }; -struct Sqrt : Op1<Sqrt> { static double f(double a); }; -struct Ceil : Op1<Ceil> { static double f(double a); }; -struct Fabs : Op1<Fabs> { static double f(double a); }; -struct Floor : Op1<Floor> { static double f(double a); }; -struct Atan2 : Op2<Atan2> { static double f(double a, double b); }; -struct Ldexp : Op2<Ldexp> { static double f(double a, double b); }; -struct Min : Op2<Min> { static double f(double a, double b); }; -struct Max : Op2<Max> { static double f(double a, double b); }; -struct IsNan : Op1<IsNan> { static double f(double a); }; -struct Relu : Op1<Relu> { static double f(double a); }; -struct Sigmoid : Op1<Sigmoid> { static double f(double a); }; +struct Neg { static double f(double a); }; +struct Not { static double f(double a); }; +struct Add { static double f(double a, double b); }; +struct Sub { static double f(double a, double b); }; +struct Mul { static double f(double a, double b); }; +struct Div { static double f(double a, double b); }; +struct Mod { static double f(double a, double b); }; +struct Pow { static double f(double a, double b); }; +struct Equal { static double f(double a, double b); }; +struct NotEqual { static double f(double a, double b); }; +struct Approx { static double f(double a, double b); }; +struct Less { static double f(double a, double b); }; +struct LessEqual { static double f(double a, double b); }; +struct Greater { static double f(double a, double b); }; +struct GreaterEqual { static double f(double a, double b); }; +struct And { static double f(double a, double b); }; +struct Or { static double f(double a, double b); }; +struct Cos { static double f(double a); }; +struct Sin { static double f(double a); }; +struct Tan { static double f(double a); }; +struct Cosh { static double f(double a); }; +struct Sinh { static double f(double a); }; +struct Tanh { static double f(double a); }; +struct Acos { static double f(double a); }; +struct Asin { static double f(double a); }; +struct Atan { static double f(double a); }; +struct Exp { static double f(double a); }; +struct Log10 { static double f(double a); }; +struct Log { static double f(double a); }; +struct Sqrt { static double f(double a); }; +struct Ceil { static double f(double a); }; +struct Fabs { static double f(double a); }; +struct Floor { static double f(double a); }; +struct Atan2 { static double f(double a, double b); }; +struct Ldexp { static double f(double a, double b); }; +struct Min { static double f(double a, double b); }; +struct Max { static double f(double a, double b); }; +struct IsNan { static double f(double a); }; +struct Relu { static double f(double a); }; +struct Sigmoid { static double f(double a); }; } // namespace vespalib::eval::operation } // namespace vespalib::eval diff --git a/eval/src/vespa/eval/eval/operation_visitor.h b/eval/src/vespa/eval/eval/operation_visitor.h deleted file mode 100644 index 4b61b2fece7..00000000000 --- a/eval/src/vespa/eval/eval/operation_visitor.h +++ /dev/null @@ -1,109 +0,0 @@ -// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. - -#pragma once - -#include "operation.h" - -namespace vespalib { -namespace eval { - -/** - * Interface implemented by Operation visitors to resolve the actual - * type of an abstract Operation. - **/ -struct OperationVisitor { - virtual void visit(const operation::Neg &) = 0; - virtual void visit(const operation::Not &) = 0; - virtual void visit(const operation::Add &) = 0; - virtual void visit(const operation::Sub &) = 0; - virtual void visit(const operation::Mul &) = 0; - virtual void visit(const operation::Div &) = 0; - virtual void visit(const operation::Mod &) = 0; - virtual void visit(const operation::Pow &) = 0; - virtual void visit(const operation::Equal &) = 0; - virtual void visit(const operation::NotEqual &) = 0; - virtual void visit(const operation::Approx &) = 0; - virtual void visit(const operation::Less &) = 0; - virtual void visit(const operation::LessEqual &) = 0; - virtual void visit(const operation::Greater &) = 0; - virtual void visit(const operation::GreaterEqual &) = 0; - virtual void visit(const operation::And &) = 0; - virtual void visit(const operation::Or &) = 0; - virtual void visit(const operation::Cos &) = 0; - virtual void visit(const operation::Sin &) = 0; - virtual void visit(const operation::Tan &) = 0; - virtual void visit(const operation::Cosh &) = 0; - virtual void visit(const operation::Sinh &) = 0; - virtual void visit(const operation::Tanh &) = 0; - virtual void visit(const operation::Acos &) = 0; - virtual void visit(const operation::Asin &) = 0; - virtual void visit(const operation::Atan &) = 0; - virtual void visit(const operation::Exp &) = 0; - virtual void visit(const operation::Log10 &) = 0; - virtual void visit(const operation::Log &) = 0; - virtual void visit(const operation::Sqrt &) = 0; - virtual void visit(const operation::Ceil &) = 0; - virtual void visit(const operation::Fabs &) = 0; - virtual void visit(const operation::Floor &) = 0; - virtual void visit(const operation::Atan2 &) = 0; - virtual void visit(const operation::Ldexp &) = 0; - virtual void visit(const operation::Min &) = 0; - virtual void visit(const operation::Max &) = 0; - virtual void visit(const operation::IsNan &) = 0; - virtual void visit(const operation::Relu &) = 0; - virtual void visit(const operation::Sigmoid &) = 0; - virtual void visit(const CustomUnaryOperation &) = 0; - virtual ~OperationVisitor() {} -}; - -/** - * Operation visitor helper class that can be subclassed to implement - * common handling of all types not specifically handled. - **/ -struct DefaultOperationVisitor : OperationVisitor { - virtual void visitDefault(const Operation &) = 0; - virtual void visit(const operation::Neg &op) override { visitDefault(op); } - virtual void visit(const operation::Not &op) override { visitDefault(op); } - virtual void visit(const operation::Add &op) override { visitDefault(op); } - virtual void visit(const operation::Sub &op) override { visitDefault(op); } - virtual void visit(const operation::Mul &op) override { visitDefault(op); } - virtual void visit(const operation::Div &op) override { visitDefault(op); } - virtual void visit(const operation::Mod &op) override { visitDefault(op); } - virtual void visit(const operation::Pow &op) override { visitDefault(op); } - virtual void visit(const operation::Equal &op) override { visitDefault(op); } - virtual void visit(const operation::NotEqual &op) override { visitDefault(op); } - virtual void visit(const operation::Approx &op) override { visitDefault(op); } - virtual void visit(const operation::Less &op) override { visitDefault(op); } - virtual void visit(const operation::LessEqual &op) override { visitDefault(op); } - virtual void visit(const operation::Greater &op) override { visitDefault(op); } - virtual void visit(const operation::GreaterEqual &op) override { visitDefault(op); } - virtual void visit(const operation::And &op) override { visitDefault(op); } - virtual void visit(const operation::Or &op) override { visitDefault(op); } - virtual void visit(const operation::Cos &op) override { visitDefault(op); } - virtual void visit(const operation::Sin &op) override { visitDefault(op); } - virtual void visit(const operation::Tan &op) override { visitDefault(op); } - virtual void visit(const operation::Cosh &op) override { visitDefault(op); } - virtual void visit(const operation::Sinh &op) override { visitDefault(op); } - virtual void visit(const operation::Tanh &op) override { visitDefault(op); } - virtual void visit(const operation::Acos &op) override { visitDefault(op); } - virtual void visit(const operation::Asin &op) override { visitDefault(op); } - virtual void visit(const operation::Atan &op) override { visitDefault(op); } - virtual void visit(const operation::Exp &op) override { visitDefault(op); } - virtual void visit(const operation::Log10 &op) override { visitDefault(op); } - virtual void visit(const operation::Log &op) override { visitDefault(op); } - virtual void visit(const operation::Sqrt &op) override { visitDefault(op); } - virtual void visit(const operation::Ceil &op) override { visitDefault(op); } - virtual void visit(const operation::Fabs &op) override { visitDefault(op); } - virtual void visit(const operation::Floor &op) override { visitDefault(op); } - virtual void visit(const operation::Atan2 &op) override { visitDefault(op); } - virtual void visit(const operation::Ldexp &op) override { visitDefault(op); } - virtual void visit(const operation::Min &op) override { visitDefault(op); } - virtual void visit(const operation::Max &op) override { visitDefault(op); } - virtual void visit(const operation::IsNan &op) override { visitDefault(op); } - virtual void visit(const operation::Relu &op) override { visitDefault(op); } - virtual void visit(const operation::Sigmoid &op) override { visitDefault(op); } - virtual void visit(const CustomUnaryOperation &op) override { visitDefault(op); } -}; - -} // namespace vespalib::eval -} // namespace vespalib diff --git a/eval/src/vespa/eval/eval/tensor_nodes.cpp b/eval/src/vespa/eval/eval/tensor_nodes.cpp index ea468b463ff..6abcd8fb9bc 100644 --- a/eval/src/vespa/eval/eval/tensor_nodes.cpp +++ b/eval/src/vespa/eval/eval/tensor_nodes.cpp @@ -7,7 +7,6 @@ namespace vespalib { namespace eval { namespace nodes { -void TensorSum ::accept(NodeVisitor &visitor) const { visitor.visit(*this); } void TensorMap ::accept(NodeVisitor &visitor) const { visitor.visit(*this); } void TensorJoin ::accept(NodeVisitor &visitor) const { visitor.visit(*this); } void TensorReduce::accept(NodeVisitor &visitor) const { visitor.visit(*this); } diff --git a/eval/src/vespa/eval/eval/tensor_nodes.h b/eval/src/vespa/eval/eval/tensor_nodes.h index df686510c1a..bfa572f1d94 100644 --- a/eval/src/vespa/eval/eval/tensor_nodes.h +++ b/eval/src/vespa/eval/eval/tensor_nodes.h @@ -12,38 +12,6 @@ namespace vespalib { namespace eval { namespace nodes { -class TensorSum : public Node { -private: - Node_UP _child; - vespalib::string _dimension; -public: - TensorSum(Node_UP child) : _child(std::move(child)), _dimension() {} - TensorSum(Node_UP child, const vespalib::string &dimension_in) - : _child(std::move(child)), _dimension(dimension_in) {} - const vespalib::string &dimension() const { return _dimension; } - vespalib::string dump(DumpContext &ctx) const override { - vespalib::string str; - str += "sum("; - str += _child->dump(ctx); - if (!_dimension.empty()) { - str += ","; - str += _dimension; - } - str += ")"; - return str; - } - void accept(NodeVisitor &visitor) const override; - size_t num_children() const override { return 1; } - const Node &get_child(size_t idx) const override { - (void) idx; - assert(idx == 0); - return *_child; - } - void detach_children(NodeHandler &handler) override { - handler.handle(std::move(_child)); - } -}; - class TensorMap : public Node { private: Node_UP _child; diff --git a/eval/src/vespa/eval/eval/test/eval_spec.cpp b/eval/src/vespa/eval/eval/test/eval_spec.cpp index f9c36c9eb93..086e6570e0a 100644 --- a/eval/src/vespa/eval/eval/test/eval_spec.cpp +++ b/eval/src/vespa/eval/eval/test/eval_spec.cpp @@ -166,7 +166,6 @@ EvalSpec::add_function_call_cases() { void EvalSpec::add_tensor_operation_cases() { - add_rule({"a", -1.0, 1.0}, "sum(a)", [](double a){ return a; }); add_rule({"a", -1.0, 1.0}, "map(a,f(x)(sin(x)))", [](double x){ return std::sin(x); }); add_rule({"a", -1.0, 1.0}, "map(a,f(x)(x+x*3))", [](double x){ return (x + (x * 3)); }); add_rule({"a", -1.0, 1.0}, {"b", -1.0, 1.0}, "join(a,b,f(x,y)(x+y))", [](double x, double y){ return (x + y); }); diff --git a/eval/src/vespa/eval/eval/test/tensor_conformance.cpp b/eval/src/vespa/eval/eval/test/tensor_conformance.cpp index fc6a5006e82..7d84fc3684e 100644 --- a/eval/src/vespa/eval/eval/test/tensor_conformance.cpp +++ b/eval/src/vespa/eval/eval/test/tensor_conformance.cpp @@ -23,9 +23,6 @@ using slime::Cursor; using slime::Inspector; using slime::JsonFormat; -using map_fun_t = TensorEngine::map_fun_t; -using join_fun_t = TensorEngine::join_fun_t; - double as_double(const TensorSpec &spec) { return spec.cells().empty() ? 0.0 : spec.cells().begin()->second.value; } @@ -504,7 +501,7 @@ struct TestContext { //------------------------------------------------------------------------- - void test_map_op(const Eval &eval, const UnaryOperation &ref_op, const Sequence &seq) { + void test_map_op(const Eval &eval, map_fun_t ref_op, const Sequence &seq) { std::vector<Layout> layouts = { {}, {x(3)}, @@ -521,36 +518,36 @@ struct TestContext { } } - void test_map_op(const vespalib::string &expr, const UnaryOperation &op, const Sequence &seq) { - TEST_DO(test_map_op(ImmediateMap(op.get_f()), op, seq)); - TEST_DO(test_map_op(RetainedMap(op.get_f()), op, seq)); + void test_map_op(const vespalib::string &expr, map_fun_t op, const Sequence &seq) { + TEST_DO(test_map_op(ImmediateMap(op), op, seq)); + TEST_DO(test_map_op(RetainedMap(op), op, seq)); TEST_DO(test_map_op(Expr_T(expr), op, seq)); TEST_DO(test_map_op(Expr_T(make_string("map(x,f(a)(%s))", expr.c_str())), op, seq)); } void test_tensor_map() { - TEST_DO(test_map_op("-a", operation::Neg(), Sub2(Div10(N())))); - TEST_DO(test_map_op("!a", operation::Not(), Mask2Seq(SkipNth(3)))); - TEST_DO(test_map_op("cos(a)", operation::Cos(), Div10(N()))); - TEST_DO(test_map_op("sin(a)", operation::Sin(), Div10(N()))); - TEST_DO(test_map_op("tan(a)", operation::Tan(), Div10(N()))); - TEST_DO(test_map_op("cosh(a)", operation::Cosh(), Div10(N()))); - TEST_DO(test_map_op("sinh(a)", operation::Sinh(), Div10(N()))); - TEST_DO(test_map_op("tanh(a)", operation::Tanh(), Div10(N()))); - TEST_DO(test_map_op("acos(a)", operation::Acos(), Sigmoid(Div10(N())))); - TEST_DO(test_map_op("asin(a)", operation::Asin(), Sigmoid(Div10(N())))); - TEST_DO(test_map_op("atan(a)", operation::Atan(), Div10(N()))); - TEST_DO(test_map_op("exp(a)", operation::Exp(), Div10(N()))); - TEST_DO(test_map_op("log10(a)", operation::Log10(), Div10(N()))); - TEST_DO(test_map_op("log(a)", operation::Log(), Div10(N()))); - TEST_DO(test_map_op("sqrt(a)", operation::Sqrt(), Div10(N()))); - TEST_DO(test_map_op("ceil(a)", operation::Ceil(), Div10(N()))); - TEST_DO(test_map_op("fabs(a)", operation::Fabs(), Div10(N()))); - TEST_DO(test_map_op("floor(a)", operation::Floor(), Div10(N()))); - TEST_DO(test_map_op("isNan(a)", operation::IsNan(), Mask2Seq(SkipNth(3), 1.0, my_nan))); - TEST_DO(test_map_op("relu(a)", operation::Relu(), Sub2(Div10(N())))); - TEST_DO(test_map_op("sigmoid(a)", operation::Sigmoid(), Sub2(Div10(N())))); - TEST_DO(test_map_op("(a+1)*2", MyOp(), Div10(N()))); + TEST_DO(test_map_op("-a", operation::Neg::f, Sub2(Div10(N())))); + TEST_DO(test_map_op("!a", operation::Not::f, Mask2Seq(SkipNth(3)))); + TEST_DO(test_map_op("cos(a)", operation::Cos::f, Div10(N()))); + TEST_DO(test_map_op("sin(a)", operation::Sin::f, Div10(N()))); + TEST_DO(test_map_op("tan(a)", operation::Tan::f, Div10(N()))); + TEST_DO(test_map_op("cosh(a)", operation::Cosh::f, Div10(N()))); + TEST_DO(test_map_op("sinh(a)", operation::Sinh::f, Div10(N()))); + TEST_DO(test_map_op("tanh(a)", operation::Tanh::f, Div10(N()))); + TEST_DO(test_map_op("acos(a)", operation::Acos::f, Sigmoid(Div10(N())))); + TEST_DO(test_map_op("asin(a)", operation::Asin::f, Sigmoid(Div10(N())))); + TEST_DO(test_map_op("atan(a)", operation::Atan::f, Div10(N()))); + TEST_DO(test_map_op("exp(a)", operation::Exp::f, Div10(N()))); + TEST_DO(test_map_op("log10(a)", operation::Log10::f, Div10(N()))); + TEST_DO(test_map_op("log(a)", operation::Log::f, Div10(N()))); + TEST_DO(test_map_op("sqrt(a)", operation::Sqrt::f, Div10(N()))); + TEST_DO(test_map_op("ceil(a)", operation::Ceil::f, Div10(N()))); + TEST_DO(test_map_op("fabs(a)", operation::Fabs::f, Div10(N()))); + TEST_DO(test_map_op("floor(a)", operation::Floor::f, Div10(N()))); + TEST_DO(test_map_op("isNan(a)", operation::IsNan::f, Mask2Seq(SkipNth(3), 1.0, my_nan))); + TEST_DO(test_map_op("relu(a)", operation::Relu::f, Sub2(Div10(N())))); + TEST_DO(test_map_op("sigmoid(a)", operation::Sigmoid::f, Sub2(Div10(N())))); + TEST_DO(test_map_op("(a+1)*2", MyOp::f, Div10(N()))); } //------------------------------------------------------------------------- @@ -563,26 +560,26 @@ struct TestContext { } void test_fixed_sparse_cases_apply_op(const Eval &eval, - const BinaryOperation &op) + join_fun_t op) { TEST_DO(test_apply_op(eval, spec("x{}", {}), spec("x{}", { { {{"x","1"}}, 3 } }), spec("x{}", { { {{"x","2"}}, 5 } }))); TEST_DO(test_apply_op(eval, - spec("x{}", { { {{"x","1"}}, op.eval(3,5) } }), + spec("x{}", { { {{"x","1"}}, op(3,5) } }), spec("x{}", { { {{"x","1"}}, 3 } }), spec("x{}", { { {{"x","1"}}, 5 } }))); TEST_DO(test_apply_op(eval, - spec("x{}", { { {{"x","1"}}, op.eval(3,-5) } }), + spec("x{}", { { {{"x","1"}}, op(3,-5) } }), spec("x{}", { { {{"x","1"}}, 3 } }), spec("x{}", { { {{"x","1"}}, -5 } }))); TEST_DO(test_apply_op(eval, spec("x{},y{},z{}", { { {{"x","-"},{"y","2"},{"z","-"}}, - op.eval(5,7) }, + op(5,7) }, { {{"x","1"},{"y","-"},{"z","3"}}, - op.eval(3,11) } }), + op(3,11) } }), spec("x{},y{}", { { {{"x","-"},{"y","2"}}, 5 }, { {{"x","1"},{"y","-"}}, 3 } }), @@ -592,9 +589,9 @@ struct TestContext { TEST_DO(test_apply_op(eval, spec("x{},y{},z{}", { { {{"x","-"},{"y","2"},{"z","-"}}, - op.eval(7,5) }, + op(7,5) }, { {{"x","1"},{"y","-"},{"z","3"}}, - op.eval(11,3) } }), + op(11,3) } }), spec("y{},z{}", { { {{"y","-"},{"z","3"}}, 11 }, { {{"y","2"},{"z","-"}}, 7 } }), @@ -604,7 +601,7 @@ struct TestContext { TEST_DO(test_apply_op(eval, spec("y{},z{}", { { {{"y","2"},{"z","-"}}, - op.eval(5,7) } }), + op(5,7) } }), spec("y{}", { { {{"y","2"}}, 5 } }), spec("y{},z{}", { { {{"y","-"},{"z","3"}}, 11 }, @@ -612,7 +609,7 @@ struct TestContext { TEST_DO(test_apply_op(eval, spec("y{},z{}", { { {{"y","2"},{"z","-"}}, - op.eval(7,5) } }), + op(7,5) } }), spec("y{},z{}", { { {{"y","-"},{"z","3"}}, 11 }, { {{"y","2"},{"z","-"}}, 7 } }), @@ -620,7 +617,7 @@ struct TestContext { TEST_DO(test_apply_op(eval, spec("x{},y{}", { { {{"x","-"},{"y","2"}}, - op.eval(5,7) } }), + op(5,7) } }), spec("x{},y{}", { { {{"x","-"},{"y","2"}}, 5 }, { {{"x","1"},{"y","-"}}, 3 } }), @@ -628,7 +625,7 @@ struct TestContext { TEST_DO(test_apply_op(eval, spec("x{},y{}", { { {{"x","-"},{"y","2"}}, - op.eval(7,5) } }), + op(7,5) } }), spec("y{}", { { {{"y","2"}}, 7 } }), spec("x{},y{}", { { {{"x","-"},{"y","2"}}, 5 }, @@ -636,21 +633,21 @@ struct TestContext { TEST_DO(test_apply_op(eval, spec("x{},z{}", { { {{"x","1"},{"z","3"}}, - op.eval(3,11) } }), + op(3,11) } }), spec("x{}", { { {{"x","1"}}, 3 } }), spec("z{}", { { {{"z","3"}}, 11 } }))); TEST_DO(test_apply_op(eval, spec("x{},z{}", { { {{"x","1"},{"z","3"}}, - op.eval(11,3) } }), + op(11,3) } }), spec("z{}",{ { {{"z","3"}}, 11 } }), spec("x{}",{ { {{"x","1"}}, 3 } }))); TEST_DO(test_apply_op(eval, spec("x{},y{}", { { {{"x","1"},{"y","1"}}, - op.eval(3,5) }, + op(3,5) }, { {{"x","2"},{"y","1"}}, - op.eval(7,5) } }), + op(7,5) } }), spec("x{}", { { {{"x","1"}}, 3 }, { {{"x","2"}}, 7 } }), @@ -659,15 +656,15 @@ struct TestContext { TEST_DO(test_apply_op(eval, spec("x{},y{},z{}", { { {{"x","1"},{"y","1"},{"z","1"}}, - op.eval(1,7) }, + op(1,7) }, { {{"x","1"},{"y","1"},{"z","2"}}, - op.eval(1,13) }, + op(1,13) }, { {{"x","1"},{"y","2"},{"z","1"}}, - op.eval(5,11) }, + op(5,11) }, { {{"x","2"},{"y","1"},{"z","1"}}, - op.eval(3,7) }, + op(3,7) }, { {{"x","2"},{"y","1"},{"z","2"}}, - op.eval(3,13) } }), + op(3,13) } }), spec("x{},y{}", { { {{"x","1"},{"y","1"}}, 1 }, { {{"x","1"},{"y","2"}}, 5 }, @@ -679,7 +676,7 @@ struct TestContext { TEST_DO(test_apply_op(eval, spec("x{},y{},z{}", { { {{"x","1"},{"y","1"},{"z","1"}}, - op.eval(1,7) } }), + op(1,7) } }), spec("x{},y{}", { { {{"x","1"},{"y","-"}}, 5 }, { {{"x","1"},{"y","1"}}, 1 } }), @@ -688,9 +685,9 @@ struct TestContext { TEST_DO(test_apply_op(eval, spec("x{},y{},z{}", { { {{"x","1"},{"y","-"},{"z","1"}}, - op.eval(5,11) }, + op(5,11) }, { {{"x","1"},{"y","1"},{"z","1"}}, - op.eval(1,7) } }), + op(1,7) } }), spec("x{},y{}", { { {{"x","1"},{"y","-"}}, 5 }, { {{"x","1"},{"y","1"}}, 1 } }), @@ -700,7 +697,7 @@ struct TestContext { TEST_DO(test_apply_op(eval, spec("x{},y{},z{}", { { {{"x","1"},{"y","1"},{"z","1"}}, - op.eval(1,7) } }), + op(1,7) } }), spec("x{},y{}", { { {{"x","-"},{"y","-"}}, 5 }, { {{"x","1"},{"y","1"}}, 1 } }), @@ -709,9 +706,9 @@ struct TestContext { TEST_DO(test_apply_op(eval, spec("x{},y{},z{}", { { {{"x","-"},{"y","-"},{"z", "-"}}, - op.eval(5,11) }, + op(5,11) }, { {{"x","1"},{"y","1"},{"z","1"}}, - op.eval(1,7) } }), + op(1,7) } }), spec("x{},y{}", { { {{"x","-"},{"y","-"}}, 5 }, { {{"x","1"},{"y","1"}}, 1 } }), @@ -721,40 +718,40 @@ struct TestContext { } void test_fixed_dense_cases_apply_op(const Eval &eval, - const BinaryOperation &op) + join_fun_t op) { TEST_DO(test_apply_op(eval, - spec(op.eval(0.1,0.2)), spec(0.1), spec(0.2))); + spec(op(0.1,0.2)), spec(0.1), spec(0.2))); TEST_DO(test_apply_op(eval, - spec(x(1), Seq({ op.eval(3,5) })), + spec(x(1), Seq({ op(3,5) })), spec(x(1), Seq({ 3 })), spec(x(1), Seq({ 5 })))); TEST_DO(test_apply_op(eval, - spec(x(1), Seq({ op.eval(3,-5) })), + spec(x(1), Seq({ op(3,-5) })), spec(x(1), Seq({ 3 })), spec(x(1), Seq({ -5 })))); TEST_DO(test_apply_op(eval, - spec(x(2), Seq({ op.eval(3,7), op.eval(5,11) })), + spec(x(2), Seq({ op(3,7), op(5,11) })), spec(x(2), Seq({ 3, 5 })), spec(x(2), Seq({ 7, 11 })))); TEST_DO(test_apply_op(eval, - spec({x(1),y(1)}, Seq({ op.eval(3,5) })), + spec({x(1),y(1)}, Seq({ op(3,5) })), spec({x(1),y(1)}, Seq({ 3 })), spec({x(1),y(1)}, Seq({ 5 })))); TEST_DO(test_apply_op(eval, - spec(x(1), Seq({ op.eval(3, 0) })), + spec(x(1), Seq({ op(3, 0) })), spec(x(1), Seq({ 3 })), spec(x(2), Seq({ 0, 7 })))); TEST_DO(test_apply_op(eval, - spec(x(1), Seq({ op.eval(0, 5) })), + spec(x(1), Seq({ op(0, 5) })), spec(x(2), Seq({ 0, 3 })), spec(x(1), Seq({ 5 })))); TEST_DO(test_apply_op(eval, spec({x(2),y(2),z(2)}, - Seq({ op.eval(1, 7), op.eval(1, 11), - op.eval(2, 13), op.eval(2, 17), - op.eval(3, 7), op.eval(3, 11), - op.eval(5, 13), op.eval(5, 17) + Seq({ op(1, 7), op(1, 11), + op(2, 13), op(2, 17), + op(3, 7), op(3, 11), + op(5, 13), op(5, 17) })), spec({x(2),y(2)}, Seq({ 1, 2, @@ -764,7 +761,7 @@ struct TestContext { 13, 17 })))); } - void test_apply_op(const Eval &eval, const BinaryOperation &op, const Sequence &seq) { + void test_apply_op(const Eval &eval, join_fun_t op, const Sequence &seq) { std::vector<Layout> layouts = { {}, {}, {x(5)}, {x(5)}, @@ -789,42 +786,42 @@ struct TestContext { TEST_STATE(make_string("lhs shape: %s, rhs shape: %s", lhs_input.type().c_str(), rhs_input.type().c_str()).c_str()); - Eval::Result expect = ImmediateJoin(op.get_f()).eval(ref_engine, lhs_input, rhs_input); + Eval::Result expect = ImmediateJoin(op).eval(ref_engine, lhs_input, rhs_input); TEST_DO(verify_result(safe(eval).eval(engine, lhs_input, rhs_input), expect)); } TEST_DO(test_fixed_sparse_cases_apply_op(eval, op)); TEST_DO(test_fixed_dense_cases_apply_op(eval, op)); } - void test_apply_op(const vespalib::string &expr, const BinaryOperation &op, const Sequence &seq) { - TEST_DO(test_apply_op(ImmediateJoin(op.get_f()), op, seq)); - TEST_DO(test_apply_op(RetainedJoin(op.get_f()), op, seq)); + void test_apply_op(const vespalib::string &expr, join_fun_t op, const Sequence &seq) { + TEST_DO(test_apply_op(ImmediateJoin(op), op, seq)); + TEST_DO(test_apply_op(RetainedJoin(op), op, seq)); TEST_DO(test_apply_op(Expr_TT(expr), op, seq)); TEST_DO(test_apply_op(Expr_TT(make_string("join(x,y,f(a,b)(%s))", expr.c_str())), op, seq)); } void test_tensor_apply() { - TEST_DO(test_apply_op("a+b", operation::Add(), Div10(N()))); - TEST_DO(test_apply_op("a-b", operation::Sub(), Div10(N()))); - TEST_DO(test_apply_op("a*b", operation::Mul(), Div10(N()))); - TEST_DO(test_apply_op("a/b", operation::Div(), Div10(N()))); - TEST_DO(test_apply_op("a%b", operation::Mod(), Div10(N()))); - TEST_DO(test_apply_op("a^b", operation::Pow(), Div10(N()))); - TEST_DO(test_apply_op("pow(a,b)", operation::Pow(), Div10(N()))); - TEST_DO(test_apply_op("a==b", operation::Equal(), Div10(N()))); - TEST_DO(test_apply_op("a!=b", operation::NotEqual(), Div10(N()))); - TEST_DO(test_apply_op("a~=b", operation::Approx(), Div10(N()))); - TEST_DO(test_apply_op("a<b", operation::Less(), Div10(N()))); - TEST_DO(test_apply_op("a<=b", operation::LessEqual(), Div10(N()))); - TEST_DO(test_apply_op("a>b", operation::Greater(), Div10(N()))); - TEST_DO(test_apply_op("a>=b", operation::GreaterEqual(), Div10(N()))); - TEST_DO(test_apply_op("a&&b", operation::And(), Mask2Seq(SkipNth(3)))); - TEST_DO(test_apply_op("a||b", operation::Or(), Mask2Seq(SkipNth(3)))); - TEST_DO(test_apply_op("atan2(a,b)", operation::Atan2(), Div10(N()))); - TEST_DO(test_apply_op("ldexp(a,b)", operation::Ldexp(), Div10(N()))); - TEST_DO(test_apply_op("fmod(a,b)", operation::Mod(), Div10(N()))); - TEST_DO(test_apply_op("min(a,b)", operation::Min(), Div10(N()))); - TEST_DO(test_apply_op("max(a,b)", operation::Max(), Div10(N()))); + TEST_DO(test_apply_op("a+b", operation::Add::f, Div10(N()))); + TEST_DO(test_apply_op("a-b", operation::Sub::f, Div10(N()))); + TEST_DO(test_apply_op("a*b", operation::Mul::f, Div10(N()))); + TEST_DO(test_apply_op("a/b", operation::Div::f, Div10(N()))); + TEST_DO(test_apply_op("a%b", operation::Mod::f, Div10(N()))); + TEST_DO(test_apply_op("a^b", operation::Pow::f, Div10(N()))); + TEST_DO(test_apply_op("pow(a,b)", operation::Pow::f, Div10(N()))); + TEST_DO(test_apply_op("a==b", operation::Equal::f, Div10(N()))); + TEST_DO(test_apply_op("a!=b", operation::NotEqual::f, Div10(N()))); + TEST_DO(test_apply_op("a~=b", operation::Approx::f, Div10(N()))); + TEST_DO(test_apply_op("a<b", operation::Less::f, Div10(N()))); + TEST_DO(test_apply_op("a<=b", operation::LessEqual::f, Div10(N()))); + TEST_DO(test_apply_op("a>b", operation::Greater::f, Div10(N()))); + TEST_DO(test_apply_op("a>=b", operation::GreaterEqual::f, Div10(N()))); + TEST_DO(test_apply_op("a&&b", operation::And::f, Mask2Seq(SkipNth(3)))); + TEST_DO(test_apply_op("a||b", operation::Or::f, Mask2Seq(SkipNth(3)))); + TEST_DO(test_apply_op("atan2(a,b)", operation::Atan2::f, Div10(N()))); + TEST_DO(test_apply_op("ldexp(a,b)", operation::Ldexp::f, Div10(N()))); + TEST_DO(test_apply_op("fmod(a,b)", operation::Mod::f, Div10(N()))); + TEST_DO(test_apply_op("min(a,b)", operation::Min::f, Div10(N()))); + TEST_DO(test_apply_op("max(a,b)", operation::Max::f, Div10(N()))); } //------------------------------------------------------------------------- @@ -833,7 +830,7 @@ struct TestContext { const TensorSpec &lhs, const TensorSpec &rhs) { - Expr_TT eval("sum(a*b)"); + Expr_TT eval("reduce(a*b,sum)"); TEST_DO(verify_result(safe(eval).eval(engine, lhs, rhs), spec(expect))); } diff --git a/eval/src/vespa/eval/eval/test/tensor_model.hpp b/eval/src/vespa/eval/eval/test/tensor_model.hpp index 53a4f85a5f4..761d7c751aa 100644 --- a/eval/src/vespa/eval/eval/test/tensor_model.hpp +++ b/eval/src/vespa/eval/eval/test/tensor_model.hpp @@ -4,11 +4,15 @@ #include <vespa/eval/eval/tensor_spec.h> #include <vespa/eval/eval/value_type.h> #include <vespa/eval/eval/operation.h> +#include <vespa/eval/eval/tensor_engine.h> namespace vespalib { namespace eval { namespace test { +using map_fun_t = TensorEngine::map_fun_t; +using join_fun_t = TensorEngine::join_fun_t; + // Random access sequence of numbers struct Sequence { virtual double operator[](size_t i) const = 0; @@ -37,16 +41,16 @@ struct Sub2 : Sequence { // Sequence of a unary operator applied to a sequence struct OpSeq : Sequence { const Sequence &seq; - const UnaryOperation &op; - OpSeq(const Sequence &seq_in, const UnaryOperation &op_in) : seq(seq_in), op(op_in) {} - double operator[](size_t i) const override { return op.eval(seq[i]); } + map_fun_t op; + OpSeq(const Sequence &seq_in, map_fun_t op_in) : seq(seq_in), op(op_in) {} + double operator[](size_t i) const override { return op(seq[i]); } }; // Sequence of applying sigmoid to another sequence struct Sigmoid : Sequence { const Sequence &seq; Sigmoid(const Sequence &seq_in) : seq(seq_in) {} - double operator[](size_t i) const override { return operation::Sigmoid().eval(seq[i]); } + double operator[](size_t i) const override { return operation::Sigmoid::f(seq[i]); } }; // pre-defined sequence of numbers @@ -105,12 +109,10 @@ struct Mask2Seq : Sequence { }; // custom op1 -struct MyOp : CustomUnaryOperation { +struct MyOp { static double f(double a) { return ((a + 1) * 2); } - double eval(double a) const override { return f(a); } - op1_fun_t get_f() const override { return f; } }; // A collection of labels for a single dimension diff --git a/eval/src/vespa/eval/eval/value.cpp b/eval/src/vespa/eval/eval/value.cpp index e11d5f01985..2a0a80b8547 100644 --- a/eval/src/vespa/eval/eval/value.cpp +++ b/eval/src/vespa/eval/eval/value.cpp @@ -1,7 +1,6 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. #include "value.h" -#include "operation_visitor.h" #include "tensor_engine.h" namespace vespalib { diff --git a/eval/src/vespa/eval/tensor/default_tensor_engine.cpp b/eval/src/vespa/eval/tensor/default_tensor_engine.cpp index 6a536497bdd..2082b7efd25 100644 --- a/eval/src/vespa/eval/tensor/default_tensor_engine.cpp +++ b/eval/src/vespa/eval/tensor/default_tensor_engine.cpp @@ -10,7 +10,6 @@ #include "dense/dense_tensor_function_compiler.h" #include <vespa/eval/eval/value.h> #include <vespa/eval/eval/tensor_spec.h> -#include <vespa/eval/eval/operation_visitor.h> #include <vespa/eval/eval/simple_tensor_engine.h> #include <cassert> @@ -288,9 +287,13 @@ const Value & DefaultTensorEngine::reduce(const Value &a, Aggr aggr, const std::vector<vespalib::string> &dimensions, Stash &stash) const { if (a.is_double()) { - Aggregator &aggregator = Aggregator::create(aggr, stash); - aggregator.first(a.as_double()); - return stash.create<DoubleValue>(aggregator.result()); + if (dimensions.empty()) { + Aggregator &aggregator = Aggregator::create(aggr, stash); + aggregator.first(a.as_double()); + return stash.create<DoubleValue>(aggregator.result()); + } else { + return ErrorValue::instance; + } } else if (auto tensor = a.as_tensor()) { assert(&tensor->engine() == this); const tensor::Tensor &my_a = static_cast<const tensor::Tensor &>(*tensor); @@ -298,17 +301,17 @@ DefaultTensorEngine::reduce(const Value &a, Aggr aggr, const std::vector<vespali return fallback_reduce(a, aggr, dimensions, stash); } switch (aggr) { - case Aggr::PROD: return to_value(my_a.reduce(eval::operation::Mul(), dimensions), stash); + case Aggr::PROD: return to_value(my_a.reduce(eval::operation::Mul::f, dimensions), stash); case Aggr::SUM: if (dimensions.empty()) { return stash.create<eval::DoubleValue>(my_a.sum()); } else if (dimensions.size() == 1) { return to_value(my_a.sum(dimensions[0]), stash); } else { - return to_value(my_a.reduce(eval::operation::Add(), dimensions), stash); + return to_value(my_a.reduce(eval::operation::Add::f, dimensions), stash); } - case Aggr::MAX: return to_value(my_a.reduce(eval::operation::Max(), dimensions), stash); - case Aggr::MIN: return to_value(my_a.reduce(eval::operation::Min(), dimensions), stash); + case Aggr::MAX: return to_value(my_a.reduce(eval::operation::Max::f, dimensions), stash); + case Aggr::MIN: return to_value(my_a.reduce(eval::operation::Min::f, dimensions), stash); default: return fallback_reduce(a, aggr, dimensions, stash); } diff --git a/eval/src/vespa/eval/tensor/dense/dense_tensor_function_compiler.cpp b/eval/src/vespa/eval/tensor/dense/dense_tensor_function_compiler.cpp index ac7b5d79a11..a22307c25ad 100644 --- a/eval/src/vespa/eval/tensor/dense/dense_tensor_function_compiler.cpp +++ b/eval/src/vespa/eval/tensor/dense/dense_tensor_function_compiler.cpp @@ -2,8 +2,6 @@ #include "dense_dot_product_function.h" #include "dense_tensor_function_compiler.h" -#include <vespa/eval/eval/operation_visitor.h> -#include <vespa/eval/eval/operation_visitor.h> #include <vespa/vespalib/test/insertion_operators.h> #include <iostream> diff --git a/eval/src/vespa/eval/tensor/dense/dense_tensor_view.cpp b/eval/src/vespa/eval/tensor/dense/dense_tensor_view.cpp index e0babad36aa..4f3e49f8ec1 100644 --- a/eval/src/vespa/eval/tensor/dense/dense_tensor_view.cpp +++ b/eval/src/vespa/eval/tensor/dense/dense_tensor_view.cpp @@ -328,29 +328,18 @@ DenseTensorView::accept(TensorVisitor &visitor) const } Tensor::UP -DenseTensorView::apply(const eval::BinaryOperation &op, const Tensor &arg) const -{ - return dense::apply(*this, arg, - [&op](double lhsValue, double rhsValue) - { return op.eval(lhsValue, rhsValue); }); -} - -Tensor::UP DenseTensorView::join(join_fun_t function, const Tensor &arg) const { - return dense::apply(*this, arg, - [function](double lhsValue, double rhsValue) - { return function(lhsValue, rhsValue); }); + return dense::apply(*this, arg, function); } Tensor::UP -DenseTensorView::reduce(const eval::BinaryOperation &op, +DenseTensorView::reduce(join_fun_t op, const std::vector<vespalib::string> &dimensions) const { return dense::reduce(*this, (dimensions.empty() ? _typeRef.dimension_names() : dimensions), - [&op](double lhsValue, double rhsValue) - { return op.eval(lhsValue, rhsValue); }); + op); } } // namespace vespalib::tensor diff --git a/eval/src/vespa/eval/tensor/dense/dense_tensor_view.h b/eval/src/vespa/eval/tensor/dense/dense_tensor_view.h index 56f26b4ba7c..aa447eb42af 100644 --- a/eval/src/vespa/eval/tensor/dense/dense_tensor_view.h +++ b/eval/src/vespa/eval/tensor/dense/dense_tensor_view.h @@ -57,11 +57,9 @@ public: virtual Tensor::UP match(const Tensor &arg) const override; virtual Tensor::UP apply(const CellFunction &func) const override; virtual Tensor::UP sum(const vespalib::string &dimension) const override; - virtual Tensor::UP apply(const eval::BinaryOperation &op, - const Tensor &arg) const override; virtual Tensor::UP join(join_fun_t function, const Tensor &arg) const override; - virtual Tensor::UP reduce(const eval::BinaryOperation &op, + virtual Tensor::UP reduce(join_fun_t op, const std::vector<vespalib::string> &dimensions) const override; virtual bool equals(const Tensor &arg) const override; diff --git a/eval/src/vespa/eval/tensor/sparse/sparse_tensor.cpp b/eval/src/vespa/eval/tensor/sparse/sparse_tensor.cpp index 05fb39f03ab..84aab8826c3 100644 --- a/eval/src/vespa/eval/tensor/sparse/sparse_tensor.cpp +++ b/eval/src/vespa/eval/tensor/sparse/sparse_tensor.cpp @@ -286,37 +286,20 @@ SparseTensor::accept(TensorVisitor &visitor) const } Tensor::UP -SparseTensor::apply(const eval::BinaryOperation &op, const Tensor &arg) const -{ - const SparseTensor *rhs = dynamic_cast<const SparseTensor *>(&arg); - if (!rhs) { - return Tensor::UP(); - } - return sparse::apply(*this, *rhs, - [&op](double lhsValue, double rhsValue) - { return op.eval(lhsValue, rhsValue); }); -} - -Tensor::UP SparseTensor::join(join_fun_t function, const Tensor &arg) const { const SparseTensor *rhs = dynamic_cast<const SparseTensor *>(&arg); if (!rhs) { return Tensor::UP(); } - return sparse::apply(*this, *rhs, - [function](double lhsValue, double rhsValue) - { return function(lhsValue, rhsValue); }); + return sparse::apply(*this, *rhs, function); } Tensor::UP -SparseTensor::reduce(const eval::BinaryOperation &op, +SparseTensor::reduce(join_fun_t op, const std::vector<vespalib::string> &dimensions) const { - return sparse::reduce(*this, - dimensions, - [&op](double lhsValue, double rhsValue) - { return op.eval(lhsValue, rhsValue); }); + return sparse::reduce(*this, dimensions, op); } } diff --git a/eval/src/vespa/eval/tensor/sparse/sparse_tensor.h b/eval/src/vespa/eval/tensor/sparse/sparse_tensor.h index b23cf5fee0d..ad460f4849c 100644 --- a/eval/src/vespa/eval/tensor/sparse/sparse_tensor.h +++ b/eval/src/vespa/eval/tensor/sparse/sparse_tensor.h @@ -51,11 +51,9 @@ public: virtual Tensor::UP match(const Tensor &arg) const override; virtual Tensor::UP apply(const CellFunction &func) const override; virtual Tensor::UP sum(const vespalib::string &dimension) const override; - virtual Tensor::UP apply(const eval::BinaryOperation &op, - const Tensor &arg) const override; virtual Tensor::UP join(join_fun_t function, const Tensor &arg) const override; - virtual Tensor::UP reduce(const eval::BinaryOperation &op, + virtual Tensor::UP reduce(join_fun_t op, const std::vector<vespalib::string> &dimensions) const override; virtual bool equals(const Tensor &arg) const override; diff --git a/eval/src/vespa/eval/tensor/tensor.h b/eval/src/vespa/eval/tensor/tensor.h index 8965accf57f..1f4d599ac18 100644 --- a/eval/src/vespa/eval/tensor/tensor.h +++ b/eval/src/vespa/eval/tensor/tensor.h @@ -40,11 +40,9 @@ struct Tensor : public eval::Tensor virtual Tensor::UP match(const Tensor &arg) const = 0; virtual Tensor::UP apply(const CellFunction &func) const = 0; virtual Tensor::UP sum(const vespalib::string &dimension) const = 0; - virtual Tensor::UP apply(const eval::BinaryOperation &op, - const Tensor &arg) const = 0; virtual Tensor::UP join(join_fun_t function, const Tensor &arg) const = 0; - virtual Tensor::UP reduce(const eval::BinaryOperation &op, + virtual Tensor::UP reduce(join_fun_t op, const std::vector<vespalib::string> &dimensions) const = 0; virtual bool equals(const Tensor &arg) const = 0; diff --git a/eval/src/vespa/eval/tensor/wrapped_simple_tensor.cpp b/eval/src/vespa/eval/tensor/wrapped_simple_tensor.cpp index 110fb446e95..a407a46610b 100644 --- a/eval/src/vespa/eval/tensor/wrapped_simple_tensor.cpp +++ b/eval/src/vespa/eval/tensor/wrapped_simple_tensor.cpp @@ -129,13 +129,6 @@ WrappedSimpleTensor::sum(const vespalib::string &) const } Tensor::UP -WrappedSimpleTensor::apply(const eval::BinaryOperation &, const Tensor &) const -{ - abort(); - return Tensor::UP(); -} - -Tensor::UP WrappedSimpleTensor::join(join_fun_t, const Tensor &) const { abort(); @@ -143,7 +136,7 @@ WrappedSimpleTensor::join(join_fun_t, const Tensor &) const } Tensor::UP -WrappedSimpleTensor::reduce(const eval::BinaryOperation &, const std::vector<vespalib::string> &) const +WrappedSimpleTensor::reduce(join_fun_t, const std::vector<vespalib::string> &) const { abort(); return Tensor::UP(); diff --git a/eval/src/vespa/eval/tensor/wrapped_simple_tensor.h b/eval/src/vespa/eval/tensor/wrapped_simple_tensor.h index 68b3b332ef4..ef3cb6425c1 100644 --- a/eval/src/vespa/eval/tensor/wrapped_simple_tensor.h +++ b/eval/src/vespa/eval/tensor/wrapped_simple_tensor.h @@ -45,9 +45,8 @@ public: Tensor::UP match(const Tensor &) const override; Tensor::UP apply(const CellFunction &) const override; Tensor::UP sum(const vespalib::string &) const override; - Tensor::UP apply(const eval::BinaryOperation &, const Tensor &) const override; Tensor::UP join(join_fun_t, const Tensor &) const override; - Tensor::UP reduce(const eval::BinaryOperation &, const std::vector<vespalib::string> &) const override; + Tensor::UP reduce(join_fun_t, const std::vector<vespalib::string> &) const override; }; } // namespace vespalib::tensor diff --git a/fastlib/src/vespa/fastlib/net/httpserver.cpp b/fastlib/src/vespa/fastlib/net/httpserver.cpp index 807b169323d..a9bba95a8ff 100644 --- a/fastlib/src/vespa/fastlib/net/httpserver.cpp +++ b/fastlib/src/vespa/fastlib/net/httpserver.cpp @@ -338,6 +338,7 @@ Fast_HTTPServer::Fast_HTTPServer(int portNumber, int stackSize, int maxThreads, int clientReadTimeout /* = -1 no timeout */) : _connections(10), + _connectionLock(), _connectionCond(), _threadPool(NULL), _acceptThread(NULL), @@ -365,22 +366,23 @@ int Fast_HTTPServer::Start(void) { int retCode = FASTLIB_SUCCESS; - _runningMutex.Lock(); - if (!_isRunning) { - // Try listening - retCode = Listen(); - - // Start worker thread - if (retCode == FASTLIB_SUCCESS) { - _acceptThread = static_cast<FastOS_Thread *>(_threadPool->NewThread(this)); - if (_acceptThread == NULL) { - retCode = FASTLIB_HTTPSERVER_NEWTHREADFAILED; + { + std::unique_lock<std::mutex> runningGuard(_runningMutex); + if (!_isRunning) { + // Try listening + retCode = Listen(); + + // Start worker thread + if (retCode == FASTLIB_SUCCESS) { + _acceptThread = static_cast<FastOS_Thread *>(_threadPool->NewThread(this)); + if (_acceptThread == NULL) { + retCode = FASTLIB_HTTPSERVER_NEWTHREADFAILED; + } } + } else { + retCode = FASTLIB_HTTPSERVER_ALREADYSTARTED; } - } else { - retCode = FASTLIB_HTTPSERVER_ALREADYSTARTED; } - _runningMutex.Unlock(); return retCode; } @@ -388,12 +390,13 @@ int Fast_HTTPServer::Start(void) void Fast_HTTPServer::Stop(void) { - _runningMutex.Lock(); - _stopSignalled = true; - if (_acceptThread) { - _acceptThread->SetBreakFlag(); + { + std::unique_lock<std::mutex> runningGuard(_runningMutex); + _stopSignalled = true; + if (_acceptThread) { + _acceptThread->SetBreakFlag(); + } } - _runningMutex.Unlock(); if (_acceptThread) { _acceptThread->Join(); } @@ -404,9 +407,8 @@ Fast_HTTPServer::Stop(void) { bool Fast_HTTPServer::StopSignalled(void) { bool retVal; - _runningMutex.Lock(); + std::unique_lock<std::mutex> runningGuard(_runningMutex); retVal = _stopSignalled; - _runningMutex.Unlock(); return retVal; } @@ -416,15 +418,16 @@ Fast_HTTPServer::~Fast_HTTPServer(void) { Stop(); - _connectionCond.Lock(); + { + std::unique_lock<std::mutex> connectionGuard(_connectionLock); - for (Fast_BagIterator<Fast_HTTPConnection*> i(_connections); !i.End(); i.Next()) - i.GetCurrent()->Interrupt(); + for (Fast_BagIterator<Fast_HTTPConnection*> i(_connections); !i.End(); i.Next()) + i.GetCurrent()->Interrupt(); - while (_connections.NumberOfElements() > 0) - _connectionCond.Wait(); - - _connectionCond.Unlock(); + while (_connections.NumberOfElements() > 0) { + _connectionCond.wait(connectionGuard); + } + } delete _threadPool; } @@ -454,11 +457,11 @@ void Fast_HTTPServer::Run(FastOS_ThreadInterface *thisThread, void *params) (void) params; Fast_Socket *mySocket; - - _runningMutex.Lock(); - _isRunning = true; - _stopSignalled = false; - _runningMutex.Unlock(); + { + std::unique_lock<std::mutex> runningGuard(_runningMutex); + _isRunning = true; + _stopSignalled = false; + } if (Listen() == FASTLIB_SUCCESS) { FastOS_SocketEvent socketEvent; @@ -513,9 +516,8 @@ void Fast_HTTPServer::Run(FastOS_ThreadInterface *thisThread, void *params) _serverSocket.SetSocketEvent(NULL); } - _runningMutex.Lock(); + std::unique_lock<std::mutex> runningGuard(_runningMutex); _isRunning = false; - _runningMutex.Unlock(); } void Fast_HTTPConnection::Run(FastOS_ThreadInterface *thisThread, void *params) @@ -1038,7 +1040,7 @@ void Fast_HTTPServer::HandleFileRequest(const string & url, Fast_HTTPConnection& void Fast_HTTPServer::SetBaseDir(const char *baseDir) { - _runningMutex.Lock(); + std::unique_lock<std::mutex> runningGuard(_runningMutex); if (!_isRunning) { _baseDir = baseDir; @@ -1051,7 +1053,6 @@ void Fast_HTTPServer::SetBaseDir(const char *baseDir) } else { fprintf(stderr, "HTTPServer: Tried to set base dir after the server had been started. Request denied.\r\n"); } - _runningMutex.Unlock(); } void @@ -1177,19 +1178,17 @@ void Fast_HTTPServer::OutputNotFound(Fast_HTTPConnection& conn, void Fast_HTTPServer::AddConnection(Fast_HTTPConnection* connection) { - _connectionCond.Lock(); + std::unique_lock<std::mutex> connectionGuard(_connectionLock); _connections.Insert(connection); - _connectionCond.Unlock(); } void Fast_HTTPServer::RemoveConnection(Fast_HTTPConnection* connection) { - _connectionCond.Lock(); + std::unique_lock<std::mutex> connectionGuard(_connectionLock); _connections.RemoveElement(connection); - _connectionCond.Signal(); - _connectionCond.Unlock(); -} + _connectionCond.notify_one(); + } void Fast_HTTPConnection::Interrupt() diff --git a/fastlib/src/vespa/fastlib/net/httpserver.h b/fastlib/src/vespa/fastlib/net/httpserver.h index 571a6108181..54803fe86d4 100644 --- a/fastlib/src/vespa/fastlib/net/httpserver.h +++ b/fastlib/src/vespa/fastlib/net/httpserver.h @@ -14,8 +14,9 @@ #include <vespa/fastlib/util/bag.h> #include <vespa/vespalib/stllike/string.h> #include <vespa/fastos/thread.h> -#include <vespa/fastos/cond.h> #include <vespa/fastos/serversocket.h> +#include <mutex> +#include <condition_variable> class FastOS_FileInterface; class Fast_HTTPServer; @@ -128,7 +129,8 @@ private: Fast_HTTPServer& operator=(const Fast_HTTPServer&); Fast_Bag<Fast_HTTPConnection*> _connections; - FastOS_Cond _connectionCond; + std::mutex _connectionLock; + std::condition_variable _connectionCond; protected: typedef vespalib::string string; @@ -143,7 +145,7 @@ protected: bool _isRunning; bool _isListening; bool _stopSignalled; - FastOS_Mutex _runningMutex; + std::mutex _runningMutex; /** Max number of concurrent threads */ int _maxThreads; diff --git a/fastlib/src/vespa/fastlib/text/normwordfolder.cpp b/fastlib/src/vespa/fastlib/text/normwordfolder.cpp index 767431e5531..f383ff85df5 100644 --- a/fastlib/src/vespa/fastlib/text/normwordfolder.cpp +++ b/fastlib/src/vespa/fastlib/text/normwordfolder.cpp @@ -2,11 +2,11 @@ #include <vespa/fastlib/text/unicodeutil.h> #include <vespa/fastlib/text/normwordfolder.h> -#include <vespa/fastos/mutex.h> +#include <mutex> #include <cstring> bool Fast_NormalizeWordFolder::_isInitialized = false; -FastOS_Mutex _initMutex; +std::mutex _initMutex; bool Fast_NormalizeWordFolder::_doAccentRemoval = false; bool Fast_NormalizeWordFolder::_doSmallToNormalKana = false; bool Fast_NormalizeWordFolder::_doKatakanaToHiragana = false; @@ -28,17 +28,18 @@ void Fast_NormalizeWordFolder::Setup(uint32_t flags) { // Only allow setting these when not initialized or initializing... - _initMutex.Lock(); - _doAccentRemoval = (DO_ACCENT_REMOVAL & flags) != 0; -// _doSmallToNormalKana = (DO_SMALL_TO_NORMAL_KANA & flags) != 0; -// _doKatakanaToHiragana = (DO_KATAKANA_TO_HIRAGANA & flags) != 0; -// _doKanaAccentCollapsing = (DO_KANA_ACCENT_COLLAPSING & flags) != 0; // Not implemented - _doFullwidthToBasicLatin = (DO_FULLWIDTH_TO_BASIC_LATIN & flags) != 0; // Not implemented - _doSharpSSubstitution = (DO_SHARP_S_SUBSTITUTION & flags) != 0; - _doLigatureSubstitution = (DO_LIGATURE_SUBSTITUTION & flags) != 0; - _doMulticharExpansion = (DO_MULTICHAR_EXPANSION & flags) != 0; - _isInitialized = false; - _initMutex.Unlock(); + { + std::unique_lock<std::mutex> initGuard(_initMutex); + _doAccentRemoval = (DO_ACCENT_REMOVAL & flags) != 0; +// _doSmallToNormalKana = (DO_SMALL_TO_NORMAL_KANA & flags) != 0; +// _doKatakanaToHiragana = (DO_KATAKANA_TO_HIRAGANA & flags) != 0; +// _doKanaAccentCollapsing = (DO_KANA_ACCENT_COLLAPSING & flags) != 0; // Not implemented + _doFullwidthToBasicLatin = (DO_FULLWIDTH_TO_BASIC_LATIN & flags) != 0; // Not implemented + _doSharpSSubstitution = (DO_SHARP_S_SUBSTITUTION & flags) != 0; + _doLigatureSubstitution = (DO_LIGATURE_SUBSTITUTION & flags) != 0; + _doMulticharExpansion = (DO_MULTICHAR_EXPANSION & flags) != 0; + _isInitialized = false; + } Initialize(); } @@ -47,7 +48,7 @@ Fast_NormalizeWordFolder::Initialize() { unsigned int i; if (!_isInitialized) { - _initMutex.Lock(); + std::unique_lock<std::mutex> initGuard(_initMutex); if (!_isInitialized) { for (i = 0; i < 128; i++) @@ -707,7 +708,6 @@ Fast_NormalizeWordFolder::Initialize() // _isInitialized = true; } - _initMutex.Unlock(); } } diff --git a/messagebus/src/vespa/messagebus/messagebus.cpp b/messagebus/src/vespa/messagebus/messagebus.cpp index 5a8f510ddcf..fa646b55221 100644 --- a/messagebus/src/vespa/messagebus/messagebus.cpp +++ b/messagebus/src/vespa/messagebus/messagebus.cpp @@ -83,7 +83,7 @@ namespace mbus { MessageBus::MessageBus(INetwork &net, ProtocolSet protocols) : _network(net), - _lock("mbus::MessageBus::_lock", false), + _lock(), _routingTables(), _sessions(), _protocolRepository(std::make_unique<ProtocolRepository>()), @@ -106,7 +106,7 @@ MessageBus::MessageBus(INetwork &net, ProtocolSet protocols) : MessageBus::MessageBus(INetwork &net, const MessageBusParams ¶ms) : _network(net), - _lock("mbus::MessageBus::_lock", false), + _lock(), _routingTables(), _sessions(), _protocolRepository(std::make_unique<ProtocolRepository>()), diff --git a/messagebus/src/vespa/messagebus/routablequeue.cpp b/messagebus/src/vespa/messagebus/routablequeue.cpp index 863cbdb3180..a3ba2ffadd3 100644 --- a/messagebus/src/vespa/messagebus/routablequeue.cpp +++ b/messagebus/src/vespa/messagebus/routablequeue.cpp @@ -5,7 +5,7 @@ namespace mbus { RoutableQueue::RoutableQueue() - : _monitor("mbus::RoutableQueue::_monitor", true), + : _monitor(), _queue() { } diff --git a/messagebus/src/vespa/messagebus/sequencer.cpp b/messagebus/src/vespa/messagebus/sequencer.cpp index 60fb3bdd39e..97fa2641504 100644 --- a/messagebus/src/vespa/messagebus/sequencer.cpp +++ b/messagebus/src/vespa/messagebus/sequencer.cpp @@ -8,7 +8,7 @@ using vespalib::make_string; namespace mbus { Sequencer::Sequencer(IMessageHandler &sender) : - _lock("mbus::Sequencer::_lock", false), + _lock(), _sender(sender), _seqMap() { diff --git a/messagebus/src/vespa/messagebus/sourcesession.cpp b/messagebus/src/vespa/messagebus/sourcesession.cpp index 1dbdd307e17..dafae0b9b1d 100644 --- a/messagebus/src/vespa/messagebus/sourcesession.cpp +++ b/messagebus/src/vespa/messagebus/sourcesession.cpp @@ -12,7 +12,7 @@ using vespalib::make_string; namespace mbus { SourceSession::SourceSession(MessageBus &mbus, const SourceSessionParams ¶ms) - : _monitor("mbus::SourceSession::_monitor", false), + : _monitor(), _mbus(mbus), _gate(new ReplyGate(_mbus)), _sequencer(*_gate), diff --git a/messagebus/src/vespa/messagebus/testlib/receptor.cpp b/messagebus/src/vespa/messagebus/testlib/receptor.cpp index 71ceff6b2fe..f821021a482 100644 --- a/messagebus/src/vespa/messagebus/testlib/receptor.cpp +++ b/messagebus/src/vespa/messagebus/testlib/receptor.cpp @@ -7,7 +7,7 @@ namespace mbus { Receptor::Receptor() : IMessageHandler(), IReplyHandler(), - _mon("mbus::Receptor::_mon", true), + _mon(), _msg(), _reply() { } diff --git a/orchestrator-restapi/src/main/java/com/yahoo/vespa/orchestrator/restapi/wire/GetHostResponse.java b/orchestrator-restapi/src/main/java/com/yahoo/vespa/orchestrator/restapi/wire/GetHostResponse.java index 38801c4c817..775ab3a6c5c 100644 --- a/orchestrator-restapi/src/main/java/com/yahoo/vespa/orchestrator/restapi/wire/GetHostResponse.java +++ b/orchestrator-restapi/src/main/java/com/yahoo/vespa/orchestrator/restapi/wire/GetHostResponse.java @@ -26,14 +26,6 @@ public class GetHostResponse { private final String applicationUrl; private final List<HostService> services; - // Deprecated - kept for backwards compatibility until clients have migrated away - public GetHostResponse(String hostname, String state) { - this.hostname = hostname; - this.state = state; - this.applicationUrl = null; - this.services = null; - } - @JsonCreator public GetHostResponse( @JsonProperty(FIELD_NAME_HOSTNAME) String hostname, diff --git a/searchcore/src/vespa/searchcore/fdispatch/common/search.cpp b/searchcore/src/vespa/searchcore/fdispatch/common/search.cpp index e3c2e79fd1a..7685ddcc328 100644 --- a/searchcore/src/vespa/searchcore/fdispatch/common/search.cpp +++ b/searchcore/src/vespa/searchcore/fdispatch/common/search.cpp @@ -165,11 +165,11 @@ void FastS_SyncSearchAdapter::DoneQuery(FastS_ISearch *, FastS_SearchContext) { - Lock(); + std::unique_lock<std::mutex> guard(_lock); _queryDone = true; - if (_waitQuery) - Signal(); - Unlock(); + if (_waitQuery) { + _cond.notify_one(); + } } @@ -177,33 +177,33 @@ void FastS_SyncSearchAdapter::DoneDocsums(FastS_ISearch *, FastS_SearchContext) { - Lock(); + std::unique_lock<std::mutex> guard(_lock); _docsumsDone = true; - if (_waitDocsums) - Signal(); - Unlock(); + if (_waitDocsums) { + _cond.notify_one(); + } } void FastS_SyncSearchAdapter::WaitQueryDone() { - Lock(); + std::unique_lock<std::mutex> guard(_lock); _waitQuery = true; - while (!_queryDone) - Wait(); - Unlock(); + while (!_queryDone) { + _cond.wait(guard); + } } void FastS_SyncSearchAdapter::WaitDocsumsDone() { - Lock(); + std::unique_lock<std::mutex> guard(_lock); _waitDocsums = true; - while (!_docsumsDone) - Wait(); - Unlock(); + while (!_docsumsDone) { + _cond.wait(guard); + } } diff --git a/searchcore/src/vespa/searchcore/fdispatch/common/search.h b/searchcore/src/vespa/searchcore/fdispatch/common/search.h index a6f22efd8cd..8ca63838c5f 100644 --- a/searchcore/src/vespa/searchcore/fdispatch/common/search.h +++ b/searchcore/src/vespa/searchcore/fdispatch/common/search.h @@ -8,8 +8,9 @@ #include <vespa/searchlib/engine/searchrequest.h> #include <vespa/searchlib/common/packets.h> #include <vespa/document/base/globalid.h> -#include <vespa/fastos/cond.h> #include <limits> +#include <mutex> +#include <condition_variable> class FastS_ISearch; @@ -469,7 +470,8 @@ class FastS_SyncSearchAdapter : public FastS_SearchAdapter, public FastS_ISearchOwner { private: - FastOS_Cond _cond; + std::mutex _lock; + std::condition_variable _cond; bool _waitQuery; bool _queryDone; bool _waitDocsums; @@ -483,11 +485,6 @@ public: static FastS_ISearch *Adapt(FastS_ISearch *search); - void Lock() { _cond.Lock(); } - void Unlock() { _cond.Unlock(); } - void Wait() { _cond.Wait(); } - void Signal() { _cond.Signal(); } - virtual void DoneQuery(FastS_ISearch *, FastS_SearchContext) override; virtual void DoneDocsums(FastS_ISearch *, FastS_SearchContext) override; diff --git a/searchcore/src/vespa/searchcore/fdispatch/search/dataset_base.cpp b/searchcore/src/vespa/searchcore/fdispatch/search/dataset_base.cpp index 0374ac52b65..816bd69012a 100644 --- a/searchcore/src/vespa/searchcore/fdispatch/search/dataset_base.cpp +++ b/searchcore/src/vespa/searchcore/fdispatch/search/dataset_base.cpp @@ -117,7 +117,7 @@ FastS_DataSetBase::DeQueueHeadWakeup_HasLock() queryQueued_t *queued; queued = _queryQueue.GetFirst(); FastS_assert(queued->IsQueued()); - queued->LockCond(); + auto queuedGuard(queued->getQueuedGuard()); //SetNowFromMonitor(); _queryQueue.DeQueueHead(); queued->UnmarkQueued(); @@ -127,7 +127,6 @@ FastS_DataSetBase::DeQueueHeadWakeup_HasLock() } else { queued->SignalCond(); } - queued->UnlockCond(); } @@ -141,9 +140,8 @@ FastS_DataSetBase::SetActiveQuery_HasLock() void FastS_DataSetBase::SetActiveQuery() { - LockDataset(); + auto dsGuard(getDsGuard()); SetActiveQuery_HasLock(); - UnlockDataset(); } @@ -160,9 +158,8 @@ FastS_DataSetBase::ClearActiveQuery_HasLock(FastS_TimeKeeper *timeKeeper) void FastS_DataSetBase::ClearActiveQuery(FastS_TimeKeeper *timeKeeper) { - LockDataset(); + auto dsGuard(getDsGuard()); ClearActiveQuery_HasLock(timeKeeper); - UnlockDataset(); } @@ -272,7 +269,7 @@ FastS_DataSetBase::UpdateSearchTime(double tnow, double elapsed, bool timedout) { int slot; - LockDataset(); + auto dsGuard(getDsGuard()); slot = (int) (elapsed * 10); if (slot >= _total._timestatslots) slot = _total._timestatslots - 1; @@ -280,7 +277,6 @@ FastS_DataSetBase::UpdateSearchTime(double tnow, slot = 0; _total._timestats[slot]++; _total._normalTimeStat.Update(tnow, elapsed, timedout); - UnlockDataset(); } @@ -302,7 +298,7 @@ void FastS_DataSetBase::addPerformance(FastS_QueryPerf &qp) { FastS_TimeStatTotals totals; - LockDataset(); + auto dsGuard(getDsGuard()); _total._normalTimeStat.AddTotal(&totals); qp.queueLen += _queryQueue.GetQueueLen(); qp.activeCnt += _queryQueue.GetActiveQueries(); @@ -310,7 +306,6 @@ FastS_DataSetBase::addPerformance(FastS_QueryPerf &qp) qp.queryTime += totals._totalAccTime; qp.dropCnt += _total._nOverload; qp.timeoutCnt += _total._nTimedOut; - UnlockDataset(); } diff --git a/searchcore/src/vespa/searchcore/fdispatch/search/dataset_base.h b/searchcore/src/vespa/searchcore/fdispatch/search/dataset_base.h index 75c47fffc27..e1a8a5e3f35 100644 --- a/searchcore/src/vespa/searchcore/fdispatch/search/dataset_base.h +++ b/searchcore/src/vespa/searchcore/fdispatch/search/dataset_base.h @@ -7,7 +7,8 @@ #include <vespa/searchcore/util/log.h> #include <atomic> #include <vespa/fastos/time.h> -#include <vespa/fastos/cond.h> +#include <mutex> +#include <condition_variable> class FastS_TimeKeeper; @@ -84,14 +85,16 @@ public: queryQueued_t(const queryQueued_t &); queryQueued_t& operator=(const queryQueued_t &); - FastOS_Cond _queueCond; + std::mutex _queuedLock; + std::condition_variable _queuedCond; queryQueued_t *_next; bool _isAborted; bool _isQueued; FNET_Task *const _deQueuedTask; public: queryQueued_t(FNET_Task *const deQueuedTask) - : _queueCond(), + : _queuedLock(), + _queuedCond(), _next(NULL), _isAborted(false), _isQueued(false), @@ -104,20 +107,18 @@ public: FastS_assert(!_isQueued); } void Wait() { - _queueCond.Lock(); + std::unique_lock<std::mutex> queuedGuard(_queuedLock); while (_isQueued) { - _queueCond.Wait(); + _queuedCond.wait(queuedGuard); } - _queueCond.Unlock(); } bool IsAborted() const { return _isAborted; } void MarkAbort() { _isAborted = true; } void MarkQueued() { _isQueued = true; } void UnmarkQueued() { _isQueued = false; } bool IsQueued() const { return _isQueued; } - void LockCond() { _queueCond.Lock(); } - void UnlockCond() { _queueCond.Unlock(); } - void SignalCond() { _queueCond.Signal(); } + std::unique_lock<std::mutex> getQueuedGuard() { return std::unique_lock<std::mutex>(_queuedLock); } + void SignalCond() { _queuedCond.notify_one(); } FNET_Task * getDequeuedTask() const @@ -164,7 +165,7 @@ public: protected: FastS_AppContext *_appCtx; - FastOS_Mutex _lock; + std::mutex _lock; FastOS_Time _createtime; queryQueue_t _queryQueue; total_t _total; @@ -182,9 +183,7 @@ public: // locking stuff //-------------- - void LockDataset() { _lock.Lock(); } - void UnlockDataset() { _lock.Unlock(); } - FastOS_Mutex & getMutex() { return _lock; } + std::unique_lock<std::mutex> getDsGuard() { return std::unique_lock<std::mutex>(_lock); } // query queue related methods //---------------------------- diff --git a/searchcore/src/vespa/searchcore/fdispatch/search/datasetcollection.cpp b/searchcore/src/vespa/searchcore/fdispatch/search/datasetcollection.cpp index 95ef4899646..cf7cd21d9b9 100644 --- a/searchcore/src/vespa/searchcore/fdispatch/search/datasetcollection.cpp +++ b/searchcore/src/vespa/searchcore/fdispatch/search/datasetcollection.cpp @@ -229,9 +229,10 @@ FastS_DataSetCollection::CreateSearch(uint32_t dataSetID, ret = new FastS_FailedSearch(dataSetID, false, search::engine::ECODE_ILLEGAL_DATASET, NULL); } else { - dataset->LockDataset(); - dataset->SetActiveQuery_HasLock(); - dataset->UnlockDataset(); + { + auto dsGuard(dataset->getDsGuard()); + dataset->SetActiveQuery_HasLock(); + } /* XXX: Semantic change: precounted as active in dataset */ ret = dataset->CreateSearch(this, timeKeeper, /* async = */ false); } @@ -247,9 +248,8 @@ FastS_DataSetCollection::CheckQueryQueues(FastS_TimeKeeper *timeKeeper) FastS_DataSetBase *dataset = PeekDataSet(datasetidx); if (dataset != NULL) { - dataset->LockDataset(); + auto dsGuard(dataset->getDsGuard()); dataset->CheckQueryQueue_HasLock(timeKeeper); - dataset->UnlockDataset(); } } } @@ -262,9 +262,8 @@ FastS_DataSetCollection::AbortQueryQueues() FastS_DataSetBase *dataset = PeekDataSet(datasetidx); if (dataset != NULL) { - dataset->LockDataset(); + auto dsGuard(dataset->getDsGuard()); dataset->AbortQueryQueue_HasLock(); - dataset->UnlockDataset(); } } } diff --git a/searchcore/src/vespa/searchcore/fdispatch/search/engine_base.cpp b/searchcore/src/vespa/searchcore/fdispatch/search/engine_base.cpp index 83ec3d73b11..83312a41875 100644 --- a/searchcore/src/vespa/searchcore/fdispatch/search/engine_base.cpp +++ b/searchcore/src/vespa/searchcore/fdispatch/search/engine_base.cpp @@ -91,7 +91,8 @@ FastS_EngineBase::FastS_EngineBase(FastS_EngineDesc *desc, _dataset(dataset), _nextds(NULL), _prevpart(NULL), - _nextpart(NULL) + _nextpart(NULL), + _lock() { FastS_assert(_dataset != NULL); } @@ -110,10 +111,11 @@ FastS_EngineBase::~FastS_EngineBase() void FastS_EngineBase::SlowQuery(double limit, double secs, bool silent) { - LockEngine(); - _stats._slowQueryCnt++; - _stats._slowQuerySecs += secs; - UnlockEngine(); + { + std::unique_lock<std::mutex> engineGuard(_lock); + _stats._slowQueryCnt++; + _stats._slowQuerySecs += secs; + } if (!silent) LOG(warning, "engine %s query slow by %.3fs + %.3fs", @@ -124,10 +126,11 @@ FastS_EngineBase::SlowQuery(double limit, double secs, bool silent) void FastS_EngineBase::SlowDocsum(double limit, double secs) { - LockEngine(); - _stats._slowDocsumCnt++; - _stats._slowDocsumSecs += secs; - UnlockEngine(); + { + std::unique_lock<std::mutex> engineGuard(_lock); + _stats._slowDocsumCnt++; + _stats._slowDocsumSecs += secs; + } LOG(warning, "engine %s docsum slow by %.3fs + %.3fs", _config._name, limit, secs); @@ -170,7 +173,7 @@ FastS_EngineBase::SampleQueueLens() double queueLen; double activecnt; - LockEngine(); + std::unique_lock<std::mutex> engineGuard(_lock); if (_stats._queueLenSampleCnt > 0) queueLen = (double) _stats._queueLenSampleAcc / (double) _stats._queueLenSampleCnt; else @@ -198,7 +201,6 @@ FastS_EngineBase::SampleQueueLens() _stats._queueLenIdx = 0; if (_stats._queueLenValid < _stats._queuestatsize) _stats._queueLenValid++; - UnlockEngine(); } void @@ -214,12 +216,13 @@ FastS_EngineBase::MarkBad(uint32_t badness) { bool worse = false; - LockEngine(); - if (badness > _badness) { - _badness = badness; - worse = true; + { + std::unique_lock<std::mutex> engineGuard(_lock); + if (badness > _badness) { + _badness = badness; + worse = true; + } } - UnlockEngine(); if (worse) { if (badness <= BAD_NOT) { @@ -233,16 +236,17 @@ FastS_EngineBase::MarkBad(uint32_t badness) void FastS_EngineBase::ClearBad() { - LockEngine(); - if (_badness >= BAD_CONFIG) { - UnlockEngine(); - LOG(warning, - "engine %s still bad due to illegal config", - _config._name); - return; + { + std::unique_lock<std::mutex> engineGuard(_lock); + if (_badness >= BAD_CONFIG) { + engineGuard.unlock(); + LOG(warning, + "engine %s still bad due to illegal config", + _config._name); + return; + } + _badness = BAD_NOT; } - _badness = BAD_NOT; - UnlockEngine(); HandleClearedBad(); } @@ -337,28 +341,29 @@ FastS_EngineBase::HandlePingResponse(uint32_t partid, } } - _dataset->LockDataset(); - if (changed) - _dataset->LinkOutPart_HasLock(this); + { + auto dsGuard(_dataset->getDsGuard()); + if (changed) + _dataset->LinkOutPart_HasLock(this); - _partid = partid; - if (docstamp != _reported._docstamp) { - _reported._docstamp = docstamp; - } - _reported._mld = mld; - _reported._maxNodes = maxnodes; - _reported._actNodes = nodes; - _reported._maxParts = maxparts; - _reported._actParts = parts; - if (_reported._activeDocs != activeDocs) { - _dataset->updateActiveDocs_HasLock(GetConfRowID(), activeDocs, _reported._activeDocs); - _reported._activeDocs = activeDocs; - } - _isUp = true; + _partid = partid; + if (docstamp != _reported._docstamp) { + _reported._docstamp = docstamp; + } + _reported._mld = mld; + _reported._maxNodes = maxnodes; + _reported._actNodes = nodes; + _reported._maxParts = maxparts; + _reported._actParts = parts; + if (_reported._activeDocs != activeDocs) { + _dataset->updateActiveDocs_HasLock(GetConfRowID(), activeDocs, _reported._activeDocs); + _reported._activeDocs = activeDocs; + } + _isUp = true; - _dataset->LinkInPart_HasLock(this); + _dataset->LinkInPart_HasLock(this); - _dataset->UnlockDataset(); + } _dataset->ScheduleCheckTempFail(); if (onlined) { @@ -383,13 +388,14 @@ FastS_EngineBase::HandleLostConnection() _stats._floptime.SetNow(); LOG(warning, "Search node %s down", _config._name); - _dataset->LockDataset(); - _dataset->LinkOutPart_HasLock(this); - PossCount noDocs; - noDocs.valid = true; - _dataset->updateActiveDocs_HasLock(GetConfRowID(), noDocs, _reported._activeDocs); - _reported._activeDocs = noDocs; - _dataset->UnlockDataset(); + { + auto dsGuard(_dataset->getDsGuard()); + _dataset->LinkOutPart_HasLock(this); + PossCount noDocs; + noDocs.valid = true; + _dataset->updateActiveDocs_HasLock(GetConfRowID(), noDocs, _reported._activeDocs); + _reported._activeDocs = noDocs; + } _dataset->ScheduleCheckTempFail(); HandleDown(); // classic: NotifyVirtualConnsDown } diff --git a/searchcore/src/vespa/searchcore/fdispatch/search/engine_base.h b/searchcore/src/vespa/searchcore/fdispatch/search/engine_base.h index 5dbbddb3bb7..7c109cb99c0 100644 --- a/searchcore/src/vespa/searchcore/fdispatch/search/engine_base.h +++ b/searchcore/src/vespa/searchcore/fdispatch/search/engine_base.h @@ -138,6 +138,7 @@ protected: FastS_EngineBase *_nextds; // list of engines in dataset FastS_EngineBase *_prevpart; // list of engines in partition FastS_EngineBase *_nextpart; // list of engines in partition + std::mutex _lock; public: FastS_EngineBase(FastS_EngineDesc *desc, FastS_PlainDataSet *dataset); @@ -181,8 +182,6 @@ public: // common engine API //------------------ - virtual void LockEngine() = 0; - virtual void UnlockEngine() = 0; virtual void Ping(); virtual void HandleClearedBad() {} virtual void HandleUp() {} diff --git a/searchcore/src/vespa/searchcore/fdispatch/search/fnet_dataset.cpp b/searchcore/src/vespa/searchcore/fdispatch/search/fnet_dataset.cpp index 92ef41cc636..f6fb46a67ca 100644 --- a/searchcore/src/vespa/searchcore/fdispatch/search/fnet_dataset.cpp +++ b/searchcore/src/vespa/searchcore/fdispatch/search/fnet_dataset.cpp @@ -108,7 +108,7 @@ FastS_FNET_DataSet::Free() bool FastS_FNET_DataSet::isGoodRow(uint32_t rowId) { - LockDataset(); + auto dsGuard(getDsGuard()); uint64_t rowBit = 1ul << rowId; bool wasBad = ((_failedRowsBitmask & rowBit) != 0); bool isBad = false; @@ -146,6 +146,5 @@ FastS_FNET_DataSet::isGoodRow(uint32_t rowId) LOG(info, "Row %d is now good again (%lu/%g active docs, coverage %ld/%ld)", rowId, candDocs, restAvg, nodesUp, configuredParts); } - UnlockDataset(); return !isBad; } diff --git a/searchcore/src/vespa/searchcore/fdispatch/search/fnet_engine.cpp b/searchcore/src/vespa/searchcore/fdispatch/search/fnet_engine.cpp index 5b444427c4d..c2f8cbfbae5 100644 --- a/searchcore/src/vespa/searchcore/fdispatch/search/fnet_engine.cpp +++ b/searchcore/src/vespa/searchcore/fdispatch/search/fnet_engine.cpp @@ -14,19 +14,14 @@ using namespace search::fs4transport; void FastS_StaticMonitorQuery::Free() { - _lock.Lock(); - _refcnt--; - if (_refcnt == 0) { - _lock.Unlock(); + if (_refcnt-- == 1) { delete this; - } else - _lock.Unlock(); + } } FastS_StaticMonitorQuery::FastS_StaticMonitorQuery() : FS4Packet_MONITORQUERYX(), - _lock(), _refcnt(1) { } @@ -64,10 +59,12 @@ FastS_FNET_Engine::Connect() _transport->Connect(_spec.c_str(), &FS4PersistentPacketStreamer::Instance, this); - LockDataSet(); - FNET_Connection *oldConn = _conn; - _conn = newConn; - UnlockDataSet(); + FNET_Connection *oldConn; + { + auto dsGuard(getDsGuard()); + oldConn = _conn; + _conn = newConn; + } if (oldConn != NULL) oldConn->SubRef(); if (newConn == NULL && !IsRealBad()) @@ -81,10 +78,12 @@ FastS_FNET_Engine::Disconnect() { if (_conn != NULL) { _conn->CloseAdminChannel(); - LockDataSet(); - FNET_Connection *conn = _conn; - _conn = NULL; - UnlockDataSet(); + FNET_Connection *conn; + { + auto dsGuard(getDsGuard()); + conn = _conn; + _conn = NULL; + } _transport->Close(conn, /* needref = */ false); } } @@ -93,7 +92,6 @@ FastS_FNET_Engine::Disconnect() FastS_FNET_Engine::FastS_FNET_Engine(FastS_EngineDesc *desc, FastS_FNET_DataSet *dataset) : FastS_EngineBase(desc, dataset), - _lock(), _spec(), _transport(dataset->GetTransport()), _conn(NULL), @@ -116,9 +114,8 @@ FastS_FNET_Engine::~FastS_FNET_Engine() _connectTask.Kill(); Disconnect(); if (IsUp()) { - LockDataSet(); + auto dsGuard(getDsGuard()); _dataset->LinkOutPart_HasLock(this); - UnlockDataSet(); } if (_monitorQuery != NULL) { _monitorQuery->Free(); diff --git a/searchcore/src/vespa/searchcore/fdispatch/search/fnet_engine.h b/searchcore/src/vespa/searchcore/fdispatch/search/fnet_engine.h index 9f03f0319db..4374deb2642 100644 --- a/searchcore/src/vespa/searchcore/fdispatch/search/fnet_engine.h +++ b/searchcore/src/vespa/searchcore/fdispatch/search/fnet_engine.h @@ -6,6 +6,7 @@ #include "engine_base.h" #include <vespa/searchlib/common/packets.h> #include <vespa/fnet/ipackethandler.h> +#include <atomic> //---------------------------------------------------------------------- @@ -13,22 +14,11 @@ using search::fs4transport::FS4Packet_MONITORQUERYX; class FastS_StaticMonitorQuery : public FS4Packet_MONITORQUERYX { - FastOS_Mutex _lock; - int _refcnt; + std::atomic<int> _refcnt; public: virtual void Free() override; - - bool getBusy() const - { - return _refcnt > 1; - } - - void markBusy() - { - _lock.Lock(); - _refcnt++; - _lock.Unlock(); - } + bool getBusy() const { return _refcnt > 1; } + void markBusy() { _refcnt++; } FastS_StaticMonitorQuery(); ~FastS_StaticMonitorQuery(); }; @@ -77,7 +67,6 @@ public: friend class FastS_FNET_Engine::ConnectTask; private: - FastOS_Mutex _lock; std::string _hostName; int _portNumber; std::string _spec; @@ -95,8 +84,7 @@ public: FastS_FNET_DataSet *dataset); virtual ~FastS_FNET_Engine(); - void LockDataSet() { _dataset->LockDataset(); } - void UnlockDataSet() { _dataset->UnlockDataset(); } + std::unique_lock<std::mutex> getDsGuard() { return _dataset->getDsGuard(); } void StartWarnTimer(); void ScheduleConnect(double delay); @@ -108,8 +96,6 @@ public: // common engine API //------------------ - virtual void LockEngine() override { _lock.Lock(); } - virtual void UnlockEngine() override { _lock.Unlock(); } virtual void Ping() override; virtual void HandleClearedBad() override; virtual void HandleUp() override; diff --git a/searchcore/src/vespa/searchcore/fdispatch/search/fnet_search.cpp b/searchcore/src/vespa/searchcore/fdispatch/search/fnet_search.cpp index c5e3d001210..0cfbdc8b69a 100644 --- a/searchcore/src/vespa/searchcore/fdispatch/search/fnet_search.cpp +++ b/searchcore/src/vespa/searchcore/fdispatch/search/fnet_search.cpp @@ -103,11 +103,11 @@ FastS_FNET_SearchNode::NT_InitMerge(uint32_t *numDocs, FastS_EngineBase * -FastS_FNET_SearchNode::getPartition(const FastOS_Mutex & dsMutex, bool userow, FastS_FNET_DataSet *dataset) +FastS_FNET_SearchNode::getPartition(const std::unique_lock<std::mutex> &dsGuard, bool userow, FastS_FNET_DataSet *dataset) { return ((userow) - ? dataset->getPartitionMLD(dsMutex, getPartID(), _flags._docsumMld, _docsumRow) - : dataset->getPartitionMLD(dsMutex, getPartID(), _flags._docsumMld)); + ? dataset->getPartitionMLD(dsGuard, getPartID(), _flags._docsumMld, _docsumRow) + : dataset->getPartitionMLD(dsGuard, getPartID(), _flags._docsumMld)); } @@ -335,26 +335,26 @@ FastS_FNET_Search::ConnectQueryNodes() } EngineNodeMap engines; engines.reserve(_nodes.size()); - FastOS_Mutex & dsLock = _dataset->getMutex(); - dsLock.Lock(); - for (uint32_t i = 0; i < _nodes.size(); i++) { - FastS_EngineBase *engine = NULL; - if (_dataset->useFixedRowDistribution()) { - engine = _dataset->getPartition(dsLock, i, fixedRow); - LOG(debug, "FixedRow: getPartition(part=%u, row=%u) -> engine(%s)", i, fixedRow, (engine != nullptr ? engine->GetName() : "null")); - } else { - engine = _dataset->getPartition(dsLock, i); - } - if (engine != nullptr) { - LOG(debug, "Wanted part=%d, engine={name=%s, row=%d, partid=%d}", i, engine->GetName(), engine->GetConfRowID(), engine->GetPartID()); + { + auto dsGuard(_dataset->getDsGuard()); + for (uint32_t i = 0; i < _nodes.size(); i++) { + FastS_EngineBase *engine = NULL; + if (_dataset->useFixedRowDistribution()) { + engine = _dataset->getPartition(dsGuard, i, fixedRow); + LOG(debug, "FixedRow: getPartition(part=%u, row=%u) -> engine(%s)", i, fixedRow, (engine != nullptr ? engine->GetName() : "null")); + } else { + engine = _dataset->getPartition(dsGuard, i); + } if (engine != nullptr) { - engines.emplace_back(engine, getNode(i)); + LOG(debug, "Wanted part=%d, engine={name=%s, row=%d, partid=%d}", i, engine->GetName(), engine->GetConfRowID(), engine->GetPartID()); + if (engine != nullptr) { + engines.emplace_back(engine, getNode(i)); + } + } else { + LOG(debug, "No engine for part %d", i); } - } else { - LOG(debug, "No engine for part %d", i); } } - dsLock.Unlock(); connectNodes(engines); } @@ -370,19 +370,19 @@ FastS_FNET_Search::ConnectEstimateNodes() uint32_t partcnt = 0; EngineNodeMap engines; - FastOS_Mutex & dsLock = _dataset->getMutex(); - dsLock.Lock(); - while (partcnt < _dataset->GetEstimateParts() && trycnt < _estPartCutoff) { - FastS_EngineBase *engine = _dataset->getPartition(dsLock, partid); - if (engine != NULL) { - engines.emplace_back(engine, getNode(partid)); - partcnt++; + { + auto dsGuard(_dataset->getDsGuard()); + while (partcnt < _dataset->GetEstimateParts() && trycnt < _estPartCutoff) { + FastS_EngineBase *engine = _dataset->getPartition(dsGuard, partid); + if (engine != NULL) { + engines.emplace_back(engine, getNode(partid)); + partcnt++; + } + trycnt++; + partid = (partid + 1) % _estPartCutoff; } - trycnt++; - partid = (partid + 1) % _estPartCutoff; + _estParts = partcnt; } - _estParts = partcnt; - dsLock.Unlock(); connectNodes(engines); } @@ -394,11 +394,10 @@ void FastS_FNET_SearchNode::Connect(FastS_FNET_Engine *engine) _engine = engine; _flags._needSubCost = true; - _engine->LockDataSet(); + auto dsGuard(_engine->getDsGuard()); _channel = _engine->OpenChannel_HasDSLock(this); _rowid = _engine->GetConfRowID(); _stamp = _engine->GetTimeStamp(); - _engine->UnlockDataSet(); } void FastS_FNET_SearchNode::Connect_HasDSLock(FastS_FNET_Engine *engine) @@ -434,30 +433,30 @@ void FastS_FNET_Search::connectSearchPath(const SearchPath::Element &elem, uint32_t dispatchLevel) { EngineNodeMap engines; - FastOS_Mutex & dsLock = _dataset->getMutex(); - dsLock.Lock(); - if (!elem.hasRow()) { - for (size_t partId : elem.nodes()) { - if (partId < _nodes.size()) { - FastS_EngineBase *engine = _dataset->getPartition(dsLock, partId); - LOG(debug, "searchpath='%s', partId=%ld, dispatchLevel=%u", spec.c_str(), partId, dispatchLevel); - if (engine != NULL) { - engines.emplace_back(engine, getNode(partId)); + { + auto dsGuard(_dataset->getDsGuard()); + if (!elem.hasRow()) { + for (size_t partId : elem.nodes()) { + if (partId < _nodes.size()) { + FastS_EngineBase *engine = _dataset->getPartition(dsGuard, partId); + LOG(debug, "searchpath='%s', partId=%ld, dispatchLevel=%u", spec.c_str(), partId, dispatchLevel); + if (engine != NULL) { + engines.emplace_back(engine, getNode(partId)); + } } } - } - } else { - for (size_t partId : elem.nodes()) { - if (partId < _nodes.size()) { - FastS_EngineBase *engine = _dataset->getPartition(dsLock, partId, elem.row()); - LOG(debug, "searchpath='%s', partId=%ld, row=%ld, dispatchLevel=%u", spec.c_str(), partId, elem.row(), dispatchLevel); - if (engine != NULL) { - engines.emplace_back(engine, getNode(partId)); + } else { + for (size_t partId : elem.nodes()) { + if (partId < _nodes.size()) { + FastS_EngineBase *engine = _dataset->getPartition(dsGuard, partId, elem.row()); + LOG(debug, "searchpath='%s', partId=%ld, row=%ld, dispatchLevel=%u", spec.c_str(), partId, elem.row(), dispatchLevel); + if (engine != NULL) { + engines.emplace_back(engine, getNode(partId)); + } } } } } - dsLock.Unlock(); connectNodes(engines); } @@ -471,26 +470,26 @@ FastS_FNET_Search::ConnectDocsumNodes(bool ignoreRow) bool userow = (_dataset->GetRowBits() > 0) && !ignoreRow; EngineNodeMap engines; - FastOS_Mutex & dsLock = _dataset->getMutex(); - dsLock.Lock(); - for (auto & node : _nodes) { - if (node._gdx != NULL) { - FastS_EngineBase *engine = node.getPartition(dsLock, userow, _dataset); - if (engine != nullptr) { - engines.emplace_back(engine, &node); - } - } - for (FastS_FNET_SearchNode::ExtraDocsumNodesIter iter(&node); iter.valid(); ++iter) { - FastS_FNET_SearchNode *eNode = *iter; - if (eNode->_gdx != NULL) { - FastS_EngineBase *engine = eNode->getPartition(dsLock, userow, _dataset); + { + auto dsGuard(_dataset->getDsGuard()); + for (auto & node : _nodes) { + if (node._gdx != NULL) { + FastS_EngineBase *engine = node.getPartition(dsGuard, userow, _dataset); if (engine != nullptr) { - engines.emplace_back(engine, eNode); + engines.emplace_back(engine, &node); + } + } + for (FastS_FNET_SearchNode::ExtraDocsumNodesIter iter(&node); iter.valid(); ++iter) { + FastS_FNET_SearchNode *eNode = *iter; + if (eNode->_gdx != NULL) { + FastS_EngineBase *engine = eNode->getPartition(dsGuard, userow, _dataset); + if (engine != nullptr) { + engines.emplace_back(engine, eNode); + } } } } } - dsLock.Unlock(); connectNodes(engines); } @@ -598,7 +597,8 @@ void FastS_FNET_Search::GotQueryResult(FastS_FNET_SearchNode *node, FS4Packet_QUERYRESULTX *qrx) { - if (!BeginFNETWork()) { + auto searchGuard(BeginFNETWork()); + if (!searchGuard) { qrx->Free(); return; } @@ -622,14 +622,15 @@ FastS_FNET_Search::GotQueryResult(FastS_FNET_SearchNode *node, } else { qrx->Free(); } - EndFNETWork(); + EndFNETWork(std::move(searchGuard)); } void FastS_FNET_Search::GotDocsum(FastS_FNET_SearchNode *node, FS4Packet_DOCSUM *docsum) { - if (!BeginFNETWork()) { + auto searchGuard(BeginFNETWork()); + if (!searchGuard) { docsum->Free(); return; } @@ -650,14 +651,16 @@ FastS_FNET_Search::GotDocsum(FastS_FNET_SearchNode *node, adjustDocsumTimeout(); } docsum->Free(); - EndFNETWork(); + EndFNETWork(std::move(searchGuard)); } void FastS_FNET_Search::LostSearchNode(FastS_FNET_SearchNode *node) { - if (!BeginFNETWork()) + auto searchGuard(BeginFNETWork()); + if (!searchGuard) { return; + } if (_FNET_mode == FNET_QUERY && node->_flags._pendingQuery) { FastS_assert(_pendingQueries > 0); @@ -673,15 +676,17 @@ FastS_FNET_Search::LostSearchNode(FastS_FNET_SearchNode *node) _pendingDocsumNodes--; adjustDocsumTimeout(); } - EndFNETWork(); + EndFNETWork(std::move(searchGuard)); } void FastS_FNET_Search::GotEOL(FastS_FNET_SearchNode *node) { - if (!BeginFNETWork()) + auto searchGuard(BeginFNETWork()); + if (!searchGuard) { return; + } LOG(spam, "Got EOL from row(%d), part(%d) = pendingQ(%d) pendingDocsum(%d)", node->GetRowID(), node->getPartID(), node->_flags._pendingQuery, node->_pendingDocsums); if (_FNET_mode == FNET_QUERY && node->_flags._pendingQuery) { @@ -698,7 +703,7 @@ FastS_FNET_Search::GotEOL(FastS_FNET_SearchNode *node) _pendingDocsumNodes--; adjustDocsumTimeout(); } - EndFNETWork(); + EndFNETWork(std::move(searchGuard)); } @@ -706,7 +711,8 @@ void FastS_FNET_Search::GotError(FastS_FNET_SearchNode *node, FS4Packet_ERROR *error) { - if (!BeginFNETWork()) { + auto searchGuard(BeginFNETWork()); + if (!searchGuard) { error->Free(); return; } @@ -741,15 +747,17 @@ FastS_FNET_Search::GotError(FastS_FNET_SearchNode *node, adjustDocsumTimeout(); } error->Free(); - EndFNETWork(); + EndFNETWork(std::move(searchGuard)); } void FastS_FNET_Search::HandleTimeout() { - if (!BeginFNETWork()) + auto searchGuard(BeginFNETWork()); + if (!searchGuard) { return; + } if (_FNET_mode == FNET_QUERY) { for (FastS_FNET_SearchNode & node : _nodes) { @@ -793,32 +801,30 @@ FastS_FNET_Search::HandleTimeout() } _docsumTimeout = true; } - EndFNETWork(); + EndFNETWork(std::move(searchGuard)); } -bool +std::unique_lock<std::mutex> FastS_FNET_Search::BeginFNETWork() { - Lock(); - if (_FNET_mode != FNET_NONE) - return true; - Unlock(); - return false; + std::unique_lock<std::mutex> searchGuard(_lock); + if (_FNET_mode == FNET_NONE) { + searchGuard.unlock(); + } + return searchGuard; } void -FastS_FNET_Search::EndFNETWork() +FastS_FNET_Search::EndFNETWork(std::unique_lock<std::mutex> searchGuard) { if (_FNET_mode == FNET_QUERY && _pendingQueries == 0) { _FNET_mode = FNET_NONE; - Unlock(); + searchGuard.unlock(); _searchOwner->DoneQuery(this, _searchContext); } else if (_FNET_mode == FNET_DOCSUMS && _pendingDocsums == 0) { _FNET_mode = FNET_NONE; - Unlock(); + searchGuard.unlock(); _searchOwner->DoneDocsums(this, _searchContext); - } else { - Unlock(); } } @@ -1070,13 +1076,14 @@ FastS_FNET_Search::Search(uint32_t searchOffset, std::vector<uint32_t> send_failed; // partitions where packet send failed // allow FNET responses while requests are being sent - Lock(); - ++_pendingQueries; // add Elephant query node to avoid early query done - ++_queryNodes; // add Elephant query node to avoid early query done - _FNET_mode = FNET_QUERY; - _queryStartTime = GetTimeKeeper()->GetTime(); - _timeout.Schedule(_adjustedQueryTimeOut); - Unlock(); + { + std::unique_lock<std::mutex> searchGuard(_lock); + ++_pendingQueries; // add Elephant query node to avoid early query done + ++_queryNodes; // add Elephant query node to avoid early query done + _FNET_mode = FNET_QUERY; + _queryStartTime = GetTimeKeeper()->GetTime(); + _timeout.Schedule(_adjustedQueryTimeOut); + } FNET_Packet::SP shared(new FS4Packet_PreSerialized(*setupQueryPacket(hitsPerNode, qflags, _queryArgs->propertiesMap))); for (uint32_t i = 0; i < _nodes.size(); i++) { FastS_FNET_SearchNode & node = _nodes[i]; @@ -1093,30 +1100,32 @@ FastS_FNET_Search::Search(uint32_t searchOffset, } // finalize setup and check if query is still in progress - Lock(); - assert(_queryNodes >= _pendingQueries); - for (uint32_t i: send_failed) { - // conditional revert of state for failed nodes - if (_nodes[i]._flags._pendingQuery) { - _nodes[i]._flags._pendingQuery = false; - assert(_pendingQueries > 0); - --_pendingQueries; - --_queryNodes; + bool done; + { + std::unique_lock<std::mutex> searchGuard(_lock); + assert(_queryNodes >= _pendingQueries); + for (uint32_t i: send_failed) { + // conditional revert of state for failed nodes + if (_nodes[i]._flags._pendingQuery) { + _nodes[i]._flags._pendingQuery = false; + assert(_pendingQueries > 0); + --_pendingQueries; + --_queryNodes; + } } - } - // revert Elephant query node to allow search to complete - assert(_pendingQueries > 0); - --_pendingQueries; - --_queryNodes; - bool done = (_pendingQueries == 0); - bool all_down = (num_send_ok == 0); - if (done) { - _FNET_mode = FNET_NONE; - if (all_down) { - SetError(search::engine::ECODE_ALL_PARTITIONS_DOWN, NULL); + // revert Elephant query node to allow search to complete + assert(_pendingQueries > 0); + --_pendingQueries; + --_queryNodes; + done = (_pendingQueries == 0); + bool all_down = (num_send_ok == 0); + if (done) { + _FNET_mode = FNET_NONE; + if (all_down) { + SetError(search::engine::ECODE_ALL_PARTITIONS_DOWN, NULL); + } } } - Unlock(); return (done) ? RET_OK : RET_INPROGRESS; } @@ -1387,31 +1396,33 @@ FastS_FNET_Search::GetDocsums(const FastS_hitresult *hits, uint32_t hitcnt) FastS_assert(p == hits + hitcnt); ConnectDocsumNodes(ignoreRow); - Lock(); + bool done; + { + std::unique_lock<std::mutex> searchGuard(_lock); - // patch in engine dependent features and send docsum requests + // patch in engine dependent features and send docsum requests - for (FastS_FNET_SearchNode & node : _nodes) { - if (node._gdx != NULL) - node.postGDX(&_pendingDocsums, &_docsumNodes); - for (FastS_FNET_SearchNode::ExtraDocsumNodesIter iter(&node); iter.valid(); ++iter) { - FastS_FNET_SearchNode *eNode = *iter; - if (eNode->_gdx != NULL) - eNode->postGDX(&_pendingDocsums, &_docsumNodes); + for (FastS_FNET_SearchNode & node : _nodes) { + if (node._gdx != NULL) + node.postGDX(&_pendingDocsums, &_docsumNodes); + for (FastS_FNET_SearchNode::ExtraDocsumNodesIter iter(&node); iter.valid(); ++iter) { + FastS_FNET_SearchNode *eNode = *iter; + if (eNode->_gdx != NULL) + eNode->postGDX(&_pendingDocsums, &_docsumNodes); + } } - } - _pendingDocsumNodes = _docsumNodes; - _requestedDocsums = _pendingDocsums; + _pendingDocsumNodes = _docsumNodes; + _requestedDocsums = _pendingDocsums; - bool done = (_pendingDocsums == 0); - if (!done) { - _FNET_mode = FNET_DOCSUMS; // FNET; do your thing + done = (_pendingDocsums == 0); + if (!done) { + _FNET_mode = FNET_DOCSUMS; // FNET; do your thing - _adjustedDocSumTimeOut = args->getTimeout().sec(); - _docSumStartTime = GetTimeKeeper()->GetTime(); - _timeout.Schedule(_adjustedDocSumTimeOut); + _adjustedDocSumTimeOut = args->getTimeout().sec(); + _docSumStartTime = GetTimeKeeper()->GetTime(); + _timeout.Schedule(_adjustedDocSumTimeOut); + } } - Unlock(); return (done) ? RET_OK : RET_INPROGRESS; } diff --git a/searchcore/src/vespa/searchcore/fdispatch/search/fnet_search.h b/searchcore/src/vespa/searchcore/fdispatch/search/fnet_search.h index 3f9fc3db077..a19dcff025d 100644 --- a/searchcore/src/vespa/searchcore/fdispatch/search/fnet_search.h +++ b/searchcore/src/vespa/searchcore/fdispatch/search/fnet_search.h @@ -112,7 +112,7 @@ public: bool IsConnected() const { return _channel != NULL; } void Connect(FastS_FNET_Engine *engine); void Connect_HasDSLock(FastS_FNET_Engine *engine); - FastS_EngineBase * getPartition(const FastOS_Mutex & dsMutex, bool userow, FastS_FNET_DataSet *dataset); + FastS_EngineBase * getPartition(const std::unique_lock<std::mutex> &dsGuard, bool userow, FastS_FNET_DataSet *dataset); void allocGDX(search::docsummary::GetDocsumArgs *args, const search::engine::PropertiesMap &properties); void postGDX(uint32_t *pendingDocsums, uint32_t *pendingDocsumNodes); vespalib::string toString() const; @@ -214,7 +214,7 @@ public: }; private: - FastOS_Mutex _lock; + std::mutex _lock; FastS_TimeKeeper *_timeKeeper; double _startTime; Timeout _timeout; @@ -272,11 +272,8 @@ private: uint32_t getFixedRowCandidate(); uint32_t getHashedRow() const; - void Lock() { _lock.Lock(); } - void Unlock() { _lock.Unlock(); } - - bool BeginFNETWork(); - void EndFNETWork(); + std::unique_lock<std::mutex> BeginFNETWork(); + void EndFNETWork(std::unique_lock<std::mutex> searchGuard); void EncodePartIDs(uint32_t partid, uint32_t rowid, bool mld, FS4Packet_QUERYRESULTX::FS4_hit *pt, diff --git a/searchcore/src/vespa/searchcore/fdispatch/search/nodemanager.cpp b/searchcore/src/vespa/searchcore/fdispatch/search/nodemanager.cpp index 10f70b2e518..6fddfae2ab0 100644 --- a/searchcore/src/vespa/searchcore/fdispatch/search/nodemanager.cpp +++ b/searchcore/src/vespa/searchcore/fdispatch/search/nodemanager.cpp @@ -83,7 +83,6 @@ FastS_NodeManager::FastS_NodeManager(vespalib::SimpleComponentConfigProducer &co : _componentConfig(componentConfig), _managerLock(), _configLock(), - _stampLock(), _appCtx(appCtx), _mldPartit(partition), _mldDocStamp(0), @@ -125,19 +124,20 @@ FastS_NodeManager::CheckTempFail() _checkTempFailScheduled = false; tempfail = false; - LockManager(); - FastS_DataSetCollection *dsc = PeekDataSetCollection(); - for (unsigned int i = 0; i < dsc->GetMaxNumDataSets(); i++) { - FastS_DataSetBase *ds; - FastS_PlainDataSet *ds_plain; - if ((ds = dsc->PeekDataSet(i)) != NULL && - (ds_plain = ds->GetPlainDataSet()) != NULL && - ds_plain->GetTempFail()) { - tempfail = true; - break; + { + std::unique_lock<std::mutex> mangerGuard(_managerLock); + FastS_DataSetCollection *dsc = PeekDataSetCollection(); + for (unsigned int i = 0; i < dsc->GetMaxNumDataSets(); i++) { + FastS_DataSetBase *ds; + FastS_PlainDataSet *ds_plain; + if ((ds = dsc->PeekDataSet(i)) != NULL && + (ds_plain = ds->GetPlainDataSet()) != NULL && + ds_plain->GetTempFail()) { + tempfail = true; + break; + } } } - UnlockManager(); _tempFail = tempfail; } @@ -166,16 +166,14 @@ uint32_t FastS_NodeManager::SetPartMap(const PartitionsConfig& partmap, unsigned int waitms) { - LockConfig(); + std::unique_lock<std::mutex> configGuard(_configLock); FastS_DataSetCollDesc *configDesc = new FastS_DataSetCollDesc(); if (!configDesc->ReadConfig(partmap)) { LOG(error, "NodeManager::SetPartMap: Failed to load configuration"); delete configDesc; - UnlockConfig(); return 0; } int retval = SetCollDesc(configDesc, waitms); - UnlockConfig(); return retval; } @@ -276,23 +274,23 @@ FastS_NodeManager::SetDataSetCollection(FastS_DataSetCollection *dsc) dsc->subRef(); } else { - - LockManager(); - _gencnt++; - gencnt = _gencnt; - - old_dsc = _datasetCollection; - _datasetCollection = dsc; - - // put old config on service list - FastS_assert(old_dsc != NULL); - if (!old_dsc->IsLastRef()) { - old_dsc->_nextOld = _oldDSCList; - _oldDSCList = old_dsc; - old_dsc = NULL; + { + std::unique_lock<std::mutex> managerGuard(_managerLock); + _gencnt++; + gencnt = _gencnt; + + old_dsc = _datasetCollection; + _datasetCollection = dsc; + + // put old config on service list + FastS_assert(old_dsc != NULL); + if (!old_dsc->IsLastRef()) { + old_dsc->_nextOld = _oldDSCList; + _oldDSCList = old_dsc; + old_dsc = NULL; + } + _hasDsc = true; } - _hasDsc = true; - UnlockManager(); if (old_dsc != NULL) old_dsc->subRef(); @@ -306,11 +304,10 @@ FastS_NodeManager::GetDataSetCollection() { FastS_DataSetCollection *ret; - LockManager(); + std::unique_lock<std::mutex> managerGuard(_managerLock); ret = _datasetCollection; FastS_assert(ret != NULL); ret->addRef(); - UnlockManager(); return ret; } @@ -322,16 +319,16 @@ FastS_NodeManager::ShutdownConfig() FastS_DataSetCollection *dsc; FastS_DataSetCollection *old_dsc; - LockConfig(); - LockManager(); - _shutdown = true; // disallow SetPartMap - dsc = _datasetCollection; - _datasetCollection = new FastS_DataSetCollection(_appCtx); - _datasetCollection->Configure(NULL, 0); - old_dsc = _oldDSCList; - _oldDSCList = NULL; - UnlockManager(); - UnlockConfig(); + { + std::unique_lock<std::mutex> configGuard(_configLock); + std::unique_lock<std::mutex> managerGuard(_managerLock); + _shutdown = true; // disallow SetPartMap + dsc = _datasetCollection; + _datasetCollection = new FastS_DataSetCollection(_appCtx); + _datasetCollection->Configure(NULL, 0); + old_dsc = _oldDSCList; + _oldDSCList = NULL; + } dsc->AbortQueryQueues(); dsc->subRef(); while (old_dsc != NULL) { @@ -350,7 +347,7 @@ FastS_NodeManager::GetTotalPartitions() uint32_t ret; ret = 0; - LockManager(); + std::unique_lock<std::mutex> managerGuard(_managerLock); FastS_DataSetCollection *dsc = PeekDataSetCollection(); for (unsigned int i = 0; i < dsc->GetMaxNumDataSets(); i++) { FastS_DataSetBase *ds; @@ -359,7 +356,6 @@ FastS_NodeManager::GetTotalPartitions() (ds_plain = ds->GetPlainDataSet()) != NULL) ret += ds_plain->GetPartitions(); } - UnlockManager(); return ret; } @@ -432,23 +428,20 @@ FastS_NodeManager::CheckEvents(FastS_TimeKeeper *timeKeeper) FastS_DataSetCollection *prev = NULL; FastS_DataSetCollection *tmp; - LockManager(); - old_dsc = _oldDSCList; - UnlockManager(); + { + std::unique_lock<std::mutex> managerGuard(_managerLock); + old_dsc = _oldDSCList; + } while (old_dsc != NULL) { if (old_dsc->IsLastRef()) { if (prev == NULL) { - LockManager(); + std::unique_lock<std::mutex> managerGuard(_managerLock); if (_oldDSCList == old_dsc) { - _oldDSCList = old_dsc->_nextOld; - UnlockManager(); - } else { - prev = _oldDSCList; - UnlockManager(); + managerGuard.unlock(); while (prev->_nextOld != old_dsc) prev = prev->_nextOld; diff --git a/searchcore/src/vespa/searchcore/fdispatch/search/nodemanager.h b/searchcore/src/vespa/searchcore/fdispatch/search/nodemanager.h index e42964d82da..e0396b46748 100644 --- a/searchcore/src/vespa/searchcore/fdispatch/search/nodemanager.h +++ b/searchcore/src/vespa/searchcore/fdispatch/search/nodemanager.h @@ -8,6 +8,7 @@ #include <vespa/searchcore/fdispatch/common/queryperf.h> #include <vespa/vespalib/net/simple_component_config_producer.h> #include <vespa/config/subscription/configuri.h> +#include <mutex> using vespa::config::search::core::PartitionsConfig; @@ -24,9 +25,8 @@ private: vespalib::SimpleComponentConfigProducer &_componentConfig; - FastOS_Mutex _managerLock; - FastOS_Mutex _configLock; - FastOS_Mutex _stampLock; + std::mutex _managerLock; + std::mutex _configLock; FastS_AppContext *_appCtx; uint32_t _mldPartit; uint32_t _mldDocStamp; // Bumped for all cache flushes @@ -63,15 +63,6 @@ public: void SubscribePartMap(const config::ConfigUri & configUri); - void LockManager() { _managerLock.Lock(); } - void UnlockManager() { _managerLock.Unlock(); } - - void LockConfig() { _configLock.Lock(); } - void UnlockConfig() { _configLock.Unlock(); } - - void LockStamp() { _stampLock.Lock(); } - void UnlockStamp() { _stampLock.Unlock(); } - uint32_t GetMldPartition() const { return _mldPartit; } uint32_t GetMldDocstamp(); uint32_t GetGenCnt() const { return _gencnt; } diff --git a/searchcore/src/vespa/searchcore/fdispatch/search/plain_dataset.cpp b/searchcore/src/vespa/searchcore/fdispatch/search/plain_dataset.cpp index 1d3c26b8e57..aa1a783a7be 100644 --- a/searchcore/src/vespa/searchcore/fdispatch/search/plain_dataset.cpp +++ b/searchcore/src/vespa/searchcore/fdispatch/search/plain_dataset.cpp @@ -241,13 +241,12 @@ FastS_PlainDataSet::~FastS_PlainDataSet() void FastS_PlainDataSet::UpdateMaxHitsPerNodeLog(bool incomplete, bool fuzzy) { - LockDataset(); + auto dsGuard(getDsGuard()); _MHPN_log._cnt++; if (incomplete) _MHPN_log._incompleteCnt++; if (fuzzy) _MHPN_log._fuzzyCnt++; - UnlockDataset(); } @@ -272,9 +271,8 @@ FastS_PlainDataSet::RefCostUseNewEngine(FastS_EngineBase *oldEngine, void FastS_PlainDataSet::updateSearchTime(double searchTime, uint32_t rowId) { - LockDataset(); + auto dsGuard(getDsGuard()); _stateOfRows.updateSearchTime(searchTime, rowId); - UnlockDataset(); } uint32_t @@ -316,9 +314,9 @@ FastS_PlainDataSet::UseNewEngine(FastS_EngineBase *oldEngine, } FastS_EngineBase * -FastS_PlainDataSet::getPartition(const FastOS_Mutex & dsMutex, uint32_t partindex, uint32_t rowid) +FastS_PlainDataSet::getPartition(const std::unique_lock<std::mutex> &dsGuard, uint32_t partindex, uint32_t rowid) { - (void) dsMutex; + (void) dsGuard; FastS_EngineBase* ret = NULL; if (IsValidPartIndex_HasLock(partindex)) { @@ -365,9 +363,9 @@ FastS_PlainDataSet::countNodesUpInRow_HasLock(uint32_t rowid) } FastS_EngineBase * -FastS_PlainDataSet::getPartition(const FastOS_Mutex & dsMutex, uint32_t partindex) +FastS_PlainDataSet::getPartition(const std::unique_lock<std::mutex> &dsGuard, uint32_t partindex) { - (void) dsMutex; + (void) dsGuard; FastS_EngineBase* ret = NULL; unsigned int oldCount = 1; unsigned int engineCount = 0; @@ -400,9 +398,9 @@ FastS_PlainDataSet::getPartition(const FastOS_Mutex & dsMutex, uint32_t partinde } FastS_EngineBase * -FastS_PlainDataSet::getPartitionMLD(const FastOS_Mutex & dsMutex, uint32_t partindex, bool mld) +FastS_PlainDataSet::getPartitionMLD(const std::unique_lock<std::mutex> &dsGuard, uint32_t partindex, bool mld) { - (void) dsMutex; + (void) dsGuard; FastS_EngineBase* ret = NULL; unsigned int oldCount = 1; if (partindex < _partMap._num_partitions) { @@ -429,9 +427,9 @@ FastS_PlainDataSet::getPartitionMLD(const FastOS_Mutex & dsMutex, uint32_t parti } FastS_EngineBase * -FastS_PlainDataSet::getPartitionMLD(const FastOS_Mutex & dsMutex, uint32_t partindex, bool mld, uint32_t rowid) +FastS_PlainDataSet::getPartitionMLD(const std::unique_lock<std::mutex> &dsGuard, uint32_t partindex, bool mld, uint32_t rowid) { - (void) dsMutex; + (void) dsGuard; FastS_EngineBase* ret = NULL; unsigned int oldCount = 1; @@ -464,11 +462,12 @@ FastS_PlainDataSet::getPartEngines(uint32_t partition) typedef FastS_EngineBase EB; typedef std::vector<EB *> EBV; EBV partEngines; - LockDataset(); - for (FastS_EngineBase *iter = _partMap._partitions[partition]._engines; iter != NULL; iter = iter->_nextpart) { - partEngines.push_back(iter); + { + auto dsGuard(getDsGuard()); + for (FastS_EngineBase *iter = _partMap._partitions[partition]._engines; iter != NULL; iter = iter->_nextpart) { + partEngines.push_back(iter); + } } - UnlockDataset(); return partEngines; } diff --git a/searchcore/src/vespa/searchcore/fdispatch/search/plain_dataset.h b/searchcore/src/vespa/searchcore/fdispatch/search/plain_dataset.h index 827abc13ce7..7c04834bd38 100644 --- a/searchcore/src/vespa/searchcore/fdispatch/search/plain_dataset.h +++ b/searchcore/src/vespa/searchcore/fdispatch/search/plain_dataset.h @@ -174,13 +174,13 @@ public: PossCount getActiveDocs() const { return _stateOfRows.getActiveDocs(); } uint32_t getRandomWeightedRow() const; - FastS_EngineBase * getPartition(const FastOS_Mutex & lock, uint32_t partid); - FastS_EngineBase * getPartition(const FastOS_Mutex & lock, uint32_t partid, uint32_t rowid); + FastS_EngineBase * getPartition(const std::unique_lock<std::mutex> &dsGuard, uint32_t partid); + FastS_EngineBase * getPartition(const std::unique_lock<std::mutex> &dsGuard, uint32_t partid, uint32_t rowid); size_t countNodesUpInRow_HasLock(uint32_t rowid); - FastS_EngineBase * getPartitionMLD(const FastOS_Mutex & lock, uint32_t partid, bool mld); - FastS_EngineBase * getPartitionMLD(const FastOS_Mutex & lock, uint32_t partid, bool mld, uint32_t rowid); + FastS_EngineBase * getPartitionMLD(const std::unique_lock<std::mutex> &dsGuard, uint32_t partid, bool mld); + FastS_EngineBase * getPartitionMLD(const std::unique_lock<std::mutex> &dsGuard, uint32_t partid, bool mld, uint32_t rowid); std::vector<FastS_EngineBase *> getPartEngines(uint32_t partition); diff --git a/searchlib/src/tests/features/prod_features.cpp b/searchlib/src/tests/features/prod_features.cpp index b1524cea904..345c66ec672 100644 --- a/searchlib/src/tests/features/prod_features.cpp +++ b/searchlib/src/tests/features/prod_features.cpp @@ -1837,7 +1837,7 @@ Test::testRankingExpression() } { // test interpreted expression - vespalib::string my_expr("3.0 + value(4.0) + sum(tensorFromWeightedSet(query(my_tensor)))"); + vespalib::string my_expr("3.0 + value(4.0) + reduce(tensorFromWeightedSet(query(my_tensor)),sum)"); FtFeatureTest ft(_factory, getExpression(my_expr)); ft.getQueryEnv().getProperties().add("my_tensor", "{a:1,b:2,c:3}"); ASSERT_TRUE(ft.setup()); diff --git a/searchlib/src/tests/postinglistbm/andstress.cpp b/searchlib/src/tests/postinglistbm/andstress.cpp index d28efb9c0e2..736d53508b4 100644 --- a/searchlib/src/tests/postinglistbm/andstress.cpp +++ b/searchlib/src/tests/postinglistbm/andstress.cpp @@ -13,6 +13,8 @@ #include <vespa/searchlib/test/fakedata/fakezcbfilterocc.h> #include <vespa/searchlib/test/fakedata/fpfactory.h> #include <vespa/fastos/thread.h> +#include <mutex> +#include <condition_variable> #include <vespa/log/log.h> LOG_SETUP(".andstress"); @@ -51,7 +53,8 @@ private: std::vector<std::vector<FakePosting::SP> > _postings; - FastOS_Cond _taskCond; + std::mutex _taskLock; + std::condition_variable _taskCond; unsigned int _taskIdx; uint32_t _numTasks; @@ -135,6 +138,7 @@ AndStressMaster::AndStressMaster(search::Rand48 &rnd, _workersDone(0), _wordSet(wordSet), _postings(FakeWordSet::NUM_WORDCLASSES), + _taskLock(), _taskCond(), _taskIdx(0), _numTasks(numTasks), @@ -276,16 +280,15 @@ AndStressMaster::Task * AndStressMaster::getTask() { Task *result = NULL; - _taskCond.Lock(); + std::unique_lock<std::mutex> taskGuard(_taskLock); if (_taskIdx < _tasks.size()) { result = &_tasks[_taskIdx]; ++_taskIdx; } else { _workersDone++; if (_workersDone == _workers.size()) - _taskCond.Broadcast(); + _taskCond.notify_all(); } - _taskCond.Unlock(); return result; } @@ -329,10 +332,11 @@ AndStressMaster::runWorkers(const std::string &postingFormat) for (unsigned int i = 0; i < _workers.size(); ++i) _threadPool->NewThread(_workers[i]); - _taskCond.Lock(); - while (_workersDone < _workers.size()) - _taskCond.Wait(); - _taskCond.Unlock(); + { + std::unique_lock<std::mutex> taskGuard(_taskLock); + while (_workersDone < _workers.size()) + _taskCond.wait(taskGuard); + } tv.SetNow(); after = tv.Secs(); LOG(info, diff --git a/searchlib/src/vespa/searchlib/attribute/attributemanager.cpp b/searchlib/src/vespa/searchlib/attribute/attributemanager.cpp index ff8c055ff45..0122516b767 100644 --- a/searchlib/src/vespa/searchlib/attribute/attributemanager.cpp +++ b/searchlib/src/vespa/searchlib/attribute/attributemanager.cpp @@ -19,7 +19,7 @@ using search::attribute::IAttributeContext; namespace { -vespalib::Monitor baseDirMonitor("attributemanagerbasedir", false); +vespalib::Monitor baseDirMonitor; typedef std::set<string> BaseDirSet; BaseDirSet baseDirSet; diff --git a/searchlib/src/vespa/searchlib/docstore/compacter.cpp b/searchlib/src/vespa/searchlib/docstore/compacter.cpp index fa6a34db4ac..4d154da9907 100644 --- a/searchlib/src/vespa/searchlib/docstore/compacter.cpp +++ b/searchlib/src/vespa/searchlib/docstore/compacter.cpp @@ -16,7 +16,7 @@ void Compacter::write(LockGuard guard, uint32_t chunkId, uint32_t lid, const void *buffer, size_t sz) { (void) chunkId; FileChunk::FileId fileId= _ds.getActiveFileId(guard); - _ds.write(guard, fileId, lid, buffer, sz); + _ds.write(std::move(guard), fileId, lid, buffer, sz); } BucketCompacter::BucketCompacter(size_t maxSignificantBucketBits, const CompressionConfig & compression, LogDataStore & ds, ThreadExecutor & executor, const IBucketizer & bucketizer, FileId source, FileId destination) : @@ -92,7 +92,7 @@ BucketCompacter::write(BucketId bucketId, uint32_t chunkId, uint32_t lid, const LidInfo lidInfo(_sourceFileId.getId(), chunkId, sz); if (_ds.getLid(_lidGuard, lid) == lidInfo) { FileId fileId = getDestinationId(guard); - _ds.write(guard, fileId, lid, buffer, sz); + _ds.write(std::move(guard), fileId, lid, buffer, sz); } } diff --git a/searchlib/src/vespa/searchlib/docstore/filechunk.cpp b/searchlib/src/vespa/searchlib/docstore/filechunk.cpp index 93a85eda0aa..e9a1ffcda20 100644 --- a/searchlib/src/vespa/searchlib/docstore/filechunk.cpp +++ b/searchlib/src/vespa/searchlib/docstore/filechunk.cpp @@ -322,7 +322,7 @@ appendChunks(FixedParams * args, Chunk::UP chunk) if (args->db.getLid(args->lidReadGuard, e.getLid()) == lidInfo) { // I am still in use so I need to taken care of. vespalib::ConstBufferRef data(chunk->getLid(e.getLid())); - args->dest.write(guard, chunk->getId(), e.getLid(), data.c_str(), data.size()); + args->dest.write(std::move(guard), chunk->getId(), e.getLid(), data.c_str(), data.size()); } } } diff --git a/searchlib/src/vespa/searchlib/docstore/logdatastore.cpp b/searchlib/src/vespa/searchlib/docstore/logdatastore.cpp index 1850db7b02b..6674952d202 100644 --- a/searchlib/src/vespa/searchlib/docstore/logdatastore.cpp +++ b/searchlib/src/vespa/searchlib/docstore/logdatastore.cpp @@ -179,14 +179,14 @@ LogDataStore::write(uint64_t serialNum, uint32_t lid, const void * buffer, size_ { LockGuard guard(_updateLock); WriteableFileChunk & active = getActive(guard); - write(guard, active, serialNum, lid, buffer, len); + write(std::move(guard), active, serialNum, lid, buffer, len); } void LogDataStore::write(LockGuard guard, FileId destinationFileId, uint32_t lid, const void * buffer, size_t len) { WriteableFileChunk & destination = static_cast<WriteableFileChunk &>(*_fileChunks[destinationFileId.getId()]); - write(guard, destination, destination.getSerialNum(), lid, buffer, len); + write(std::move(guard), destination, destination.getSerialNum(), lid, buffer, len); } void @@ -196,7 +196,7 @@ LogDataStore::write(LockGuard guard, WriteableFileChunk & destination, LidInfo lm = destination.append(serialNum, lid, buffer, len); setLid(guard, lid, lm); if (destination.getFileId() == getActiveFileId(guard)) { - requireSpace(guard, destination); + requireSpace(std::move(guard), destination); } } @@ -428,7 +428,7 @@ SerialNum LogDataStore::flushFile(LockGuard guard, WriteableFileChunk & file, Se } void LogDataStore::flushFileAndWait(LockGuard guard, WriteableFileChunk & file, SerialNum syncToken) { - syncToken = flushFile(guard, file, syncToken); + syncToken = flushFile(std::move(guard), file, syncToken); file.waitForDiskToCatchUpToNow(); _tlSyncer.sync(syncToken); file.flushPendingChunks(syncToken); @@ -437,13 +437,13 @@ void LogDataStore::flushFileAndWait(LockGuard guard, WriteableFileChunk & file, SerialNum LogDataStore::flushActive(SerialNum syncToken) { LockGuard guard(_updateLock); WriteableFileChunk &active = getActive(guard); - return flushFile(guard, active, syncToken); + return flushFile(std::move(guard), active, syncToken); } void LogDataStore::flushActiveAndWait(SerialNum syncToken) { LockGuard guard(_updateLock); WriteableFileChunk &active = getActive(guard); - return flushFileAndWait(guard, active, syncToken); + return flushFileAndWait(std::move(guard), active, syncToken); } bool LogDataStore::shouldCompactToActiveFile(size_t compactedSize) const { @@ -488,7 +488,7 @@ void LogDataStore::compactFile(FileId fileId) } else { LockGuard guard(_updateLock); WriteableFileChunk & compactTo = dynamic_cast<WriteableFileChunk &>(*_fileChunks[destinationFileId.getId()]); - flushFileAndWait(guard, compactTo, 0); + flushFileAndWait(std::move(guard), compactTo, 0); compactTo.freeze(); } compacter.reset(); diff --git a/staging_vespalib/src/vespa/vespalib/util/clock.cpp b/staging_vespalib/src/vespa/vespalib/util/clock.cpp index 403e1408747..b19b067afa9 100644 --- a/staging_vespalib/src/vespa/vespalib/util/clock.cpp +++ b/staging_vespalib/src/vespa/vespalib/util/clock.cpp @@ -2,6 +2,7 @@ #include "clock.h" #include <cassert> +#include <chrono> using namespace fastos; @@ -11,6 +12,7 @@ namespace vespalib { Clock::Clock(double timePeriod) : _timeNS(0u), _timePeriodMS(static_cast<uint32_t>(timePeriod*1000)), + _lock(), _cond(), _stop(false), _running(false) @@ -32,22 +34,20 @@ void Clock::Run(FastOS_ThreadInterface *thread, void *arguments) { (void) arguments; _running = true; - _cond.Lock(); + std::unique_lock<std::mutex> guard(_lock); while ( ! thread->GetBreakFlag() && !_stop) { setTime(); - _cond.TimedWait(_timePeriodMS); + _cond.wait_for(guard, std::chrono::milliseconds(_timePeriodMS)); } - _cond.Unlock(); _running = false; } void Clock::stop(void) { - _cond.Lock(); + std::unique_lock<std::mutex> guard(_lock); _stop = true; - _cond.Broadcast(); - _cond.Unlock(); + _cond.notify_all(); } } diff --git a/staging_vespalib/src/vespa/vespalib/util/clock.h b/staging_vespalib/src/vespa/vespalib/util/clock.h index 253eba25556..75f407ec95d 100644 --- a/staging_vespalib/src/vespa/vespalib/util/clock.h +++ b/staging_vespalib/src/vespa/vespalib/util/clock.h @@ -2,8 +2,9 @@ #pragma once #include <vespa/fastos/thread.h> -#include <vespa/fastos/cond.h> #include <vespa/fastos/timestamp.h> +#include <mutex> +#include <condition_variable> namespace vespalib { @@ -21,7 +22,8 @@ private: mutable fastos::TimeStamp _timeNS; int _timePeriodMS; - FastOS_Cond _cond; + std::mutex _lock; + std::condition_variable _cond; bool _stop; bool _running; diff --git a/vespalib/src/tests/sync/sync_test.cpp b/vespalib/src/tests/sync/sync_test.cpp index ba6be74be22..a17f9089877 100644 --- a/vespalib/src/tests/sync/sync_test.cpp +++ b/vespalib/src/tests/sync/sync_test.cpp @@ -156,7 +156,7 @@ Test::Main() TryLock a(lock); CHECK_LOCKED(lock); if (a.hasLock()) { - LockGuard guard(a); + LockGuard guard(std::move(a)); CHECK_LOCKED(lock); } CHECK_UNLOCKED(lock); @@ -167,7 +167,7 @@ Test::Main() TryLock a(mon); CHECK_LOCKED(mon); if (a.hasLock()) { - LockGuard guard(a); + LockGuard guard(std::move(a)); CHECK_LOCKED(mon); } CHECK_UNLOCKED(mon); @@ -178,7 +178,7 @@ Test::Main() TryLock a(mon); CHECK_LOCKED(mon); if (a.hasLock()) { - MonitorGuard guard(a); + MonitorGuard guard(std::move(a)); CHECK_LOCKED(mon); } CHECK_UNLOCKED(mon); @@ -197,7 +197,7 @@ Test::Main() { CHECK_LOCKED(lock); EXPECT_TRUE(a.hasLock()); - LockGuard guard(a); + LockGuard guard(std::move(a)); EXPECT_TRUE(!a.hasLock()); CHECK_LOCKED(lock); } @@ -240,7 +240,7 @@ Test::Main() tl.unlock(); EXPECT_FALSE(tl.hasLock()); } - // LockGuard/MonitorGuard have destructive copy + // LockGuard/MonitorGuard have destructive move { Lock lock; CHECK_UNLOCKED(lock); @@ -248,7 +248,7 @@ Test::Main() CHECK_LOCKED(lock); { CHECK_LOCKED(lock); - LockGuard b(a); + LockGuard b(std::move(a)); CHECK_LOCKED(lock); } CHECK_UNLOCKED(lock); @@ -260,7 +260,7 @@ Test::Main() CHECK_LOCKED(mon); { CHECK_LOCKED(mon); - MonitorGuard b(a); + MonitorGuard b(std::move(a)); CHECK_LOCKED(mon); } CHECK_UNLOCKED(mon); diff --git a/vespalib/src/vespa/vespalib/util/ptrholder.h b/vespalib/src/vespa/vespalib/util/ptrholder.h index 3e807a8a93d..1c06e5c53e6 100644 --- a/vespalib/src/vespa/vespalib/util/ptrholder.h +++ b/vespalib/src/vespa/vespalib/util/ptrholder.h @@ -38,7 +38,7 @@ public: * @brief Create an empty PtrHolder with both current and new * pointers set to 0 **/ - PtrHolder() : _current(), _next(), _lock("PtrHolder", false) {} + PtrHolder() : _current(), _next(), _lock() {} /** * @brief Empty destructor diff --git a/vespalib/src/vespa/vespalib/util/sync.h b/vespalib/src/vespa/vespalib/util/sync.h index 86e0a227c72..c3d8ea86aec 100644 --- a/vespalib/src/vespa/vespalib/util/sync.h +++ b/vespalib/src/vespa/vespalib/util/sync.h @@ -2,39 +2,15 @@ #pragma once -#include <vespa/fastos/mutex.h> -#include <vespa/fastos/cond.h> #include <vespa/fastos/time.h> #include <cassert> +#include <mutex> +#include <condition_variable> +#include <chrono> namespace vespalib { -#ifndef IAM_DOXYGEN -class LockGuardHandover -{ -private: - friend class LockGuard; - FastOS_MutexInterface *_mutex; - LockGuardHandover(const LockGuardHandover &); - LockGuardHandover &operator=(const LockGuardHandover &); - LockGuardHandover(FastOS_MutexInterface *m) : _mutex(m) {} -public: -}; - - -class MonitorGuardHandover -{ -private: - friend class MonitorGuard; - FastOS_CondInterface *_cond; - MonitorGuardHandover(const MonitorGuardHandover &); - MonitorGuardHandover &operator=(const MonitorGuardHandover &); - MonitorGuardHandover(FastOS_CondInterface *c) : _cond(c) {} -public: -}; -#endif - /** * @brief A Lock is a synchronization primitive used to ensure mutual * exclusion. @@ -47,11 +23,11 @@ public: **/ class Lock { -private: +protected: friend class LockGuard; friend class TryLock; - mutable FastOS_Mutex _mutex; + mutable std::mutex _mutex; public: /** * @brief Create a new Lock. @@ -59,42 +35,10 @@ public: * Creates a Lock that has mutex instrumentation disabled. **/ Lock() : _mutex() {} - /** - * @brief Create a new Lock. - * - * Creates a Lock with support for mutex instrumentation. - * - * @param name mutex category (for instrumentation) - * @param leaf false if you will lock other locks while holding this one, - * true if you only lock this as the last (leaf) lock - * (for instrumentation). - **/ - Lock(const char *name, bool leaf) : _mutex() {(void) name; (void) leaf; } - /** - * @brief Copy a Lock. - * - * Create a new Lock with mutex instrumentation settings obtained - * from the given lock. No other information is copied. Normally - * only used when copy-constructing a bigger object containing a - * Lock. - * - * @param rhs other Lock - **/ - Lock(const Lock &rhs) : _mutex() { (void) rhs; } - /** - * @brief No-op assignment operator. - * - * Assignment operator ignoring the right hand side. It makes no - * sense to assign the state of one Lock to another, but we want - * to allow assignment of bigger objects that contain Lock - * objects. - * - * @param rhs other Lock (right hand side) - **/ - Lock &operator=(const Lock &rhs) { - (void) rhs; - return *this; - } + Lock(const Lock &) : Lock() { } + Lock(Lock &&) : Lock() { } + Lock &operator=(const Lock &) { return *this; } + Lock &operator=(Lock &&) { return *this; } }; @@ -110,56 +54,25 @@ public: * * @see TryLock **/ -class Monitor +class Monitor : public Lock { private: friend class LockGuard; friend class MonitorGuard; friend class TryLock; - mutable FastOS_Cond _cond; + mutable std::condition_variable _cond; public: /** * @brief Create a new Monitor. * * Creates a Monitor that has mutex instrumentation disabled. **/ - Monitor() : _cond() {} - /** - * @brief Create a new Monitor. - * - * Creates a Monitor with support for mutex instrumentation. - * - * @param name mutex category (for instrumentation) - * @param leaf false if you will lock other locks while holding this one, - * true if you only lock this as the last (leaf) lock - * (for instrumentation). - **/ - Monitor(const char *name, bool leaf) : _cond() { (void) name; (void) leaf; } - /** - * @brief Copy a Monitor. - * - * Creates a new Monitor with mutex instrumentation settings - * obtained from the given monitor. No other information is - * copied. Normally only used when copy-constructing a bigger - * object containing a Lock. - * - * @param rhs other Monitor - **/ - Monitor(const Monitor &rhs) : _cond() { (void) rhs; } - /** - * @brief No-op assignment operator. - * - * Assignment operator ignoring the right hand side. It makes no - * sense to assign the state of one Monitor to another, but we want - * to allow assigning objects that contain Monitor objects. - * - * @param rhs other Monitor (right hand side) - **/ - Monitor &operator=(const Monitor &rhs) { - (void) rhs; - return *this; - } + Monitor() : Lock(), _cond() {} + Monitor(const Monitor &) : Monitor() { } + Monitor(Monitor &&) : Monitor() { } + Monitor &operator=(const Monitor &) { return *this; } + Monitor &operator=(Monitor &&) { return *this; } }; @@ -198,29 +111,11 @@ private: friend class LockGuard; friend class MonitorGuard; - FastOS_MutexInterface *_mutex; - FastOS_CondInterface *_cond; - - TryLock(const TryLock &); - TryLock &operator=(const TryLock &); + std::unique_lock<std::mutex> _guard; + std::condition_variable *_cond; - FastOS_MutexInterface *stealMutex() { - FastOS_MutexInterface *ret = _mutex; - if (ret != NULL) { - _mutex = NULL; - return ret; - } - ret = _cond; - assert(ret != NULL); - _cond = NULL; - return ret; - } - FastOS_CondInterface *stealCond() { - FastOS_CondInterface *ret = _cond; - assert(ret != NULL); - _cond = NULL; - return ret; - } + TryLock(const TryLock &) = delete; + TryLock &operator=(const TryLock &) = delete; public: /** @@ -228,38 +123,50 @@ public: * * @param lock the lock to obtain **/ - TryLock(const Lock &lock) : _mutex(&lock._mutex), _cond(NULL) { - if (!_mutex->TryLock()) { - _mutex = NULL; - } + TryLock(const Lock &lock) + : _guard(lock._mutex, std::try_to_lock), + _cond(nullptr) + { } + /** * @brief Try to lock the given Monitor * * @param mon the monitor to lock **/ - TryLock(const Monitor &mon) : _mutex(NULL), _cond(&mon._cond) { - if (!_cond->TryLock()) { - _cond = NULL; - } + TryLock(const Monitor &mon) + : _guard(mon._mutex, std::try_to_lock), + _cond(_guard ? &mon._cond : nullptr) + { + } + + TryLock(TryLock &&rhs) + : _guard(std::move(rhs._guard)), + _cond(rhs._cond) + { + rhs._cond = nullptr; } + /** * @brief Release the lock held by this object, if any **/ - ~TryLock() { - if (_mutex != NULL) { - _mutex->Unlock(); - } - if (_cond != NULL) { - _cond->Unlock(); + ~TryLock() = default; + + TryLock &operator=(TryLock &&rhs) { + if (this != &rhs) { + _guard = std::move(rhs._guard); + _cond = rhs._cond; + rhs._cond = nullptr; } + return *this; } + /** * @brief Check whether this object holds a lock * * @return true if this object holds a lock **/ - bool hasLock() { return (_mutex != NULL || _cond != NULL); } + bool hasLock() { return static_cast<bool>(_guard); } /** * @brief Release the lock held by this object. * @@ -269,13 +176,9 @@ public: * the destructor will release the lock. **/ void unlock() { - if (_mutex != NULL) { - _mutex->Unlock(); - _mutex = NULL; - } - if (_cond != NULL) { - _cond->Unlock(); - _cond = NULL; + if (_guard) { + _guard.unlock(); + _cond = nullptr; } } }; @@ -296,25 +199,20 @@ public: class LockGuard { private: - FastOS_MutexInterface *_mutex; - LockGuard &operator=(const LockGuard &); - - FastOS_MutexInterface *stealMutex() { - FastOS_MutexInterface *ret = _mutex; - _mutex = NULL; - return ret; - } + std::unique_lock<std::mutex> _guard; + LockGuard &operator=(const LockGuard &) = delete; public: /** * @brief A noop guard without any mutex. **/ - LockGuard() : _mutex(NULL) {} + LockGuard() : _guard() {} + LockGuard(const LockGuard &rhs) = delete; /** * @brief Steal the lock from the given LockGuard * * @param rhs steal the lock from this one **/ - LockGuard(LockGuard &rhs) : _mutex(rhs.stealMutex()) {} + LockGuard(LockGuard &&rhs) : _guard(std::move(rhs._guard)) { } /** * @brief Obtain the lock represented by the given Lock object. * @@ -322,19 +220,8 @@ public: * * @param lock take it **/ - LockGuard(const Lock &lock) : _mutex(&lock._mutex) { - _mutex->Lock(); - } - /** - * @brief Obtain the lock on the given Monitor object. - * - * The method will block until the lock can be obtained. - * - * @param monitor take the lock on it - **/ - LockGuard(const Monitor &monitor) : _mutex(&monitor._cond) { - _mutex->Lock(); - } + LockGuard(const Lock &lock) : _guard(lock._mutex) { } + /** * @brief Create a LockGuard from a TryLock. * @@ -344,11 +231,18 @@ public: * * @param tlock take the lock from this one **/ - LockGuard(TryLock &tlock) : _mutex(tlock.stealMutex()) {} -#ifndef IAM_DOXYGEN - LockGuard(const LockGuardHandover &rhs) : _mutex(rhs._mutex) {} - operator LockGuardHandover() { return LockGuardHandover(stealMutex()); } -#endif + LockGuard(TryLock &&tlock) : _guard(std::move(tlock._guard)) + { + tlock._cond = nullptr; + } + + LockGuard &operator=(LockGuard &&rhs) { + if (this != &rhs) { + _guard = std::move(rhs._guard); + } + return *this; + } + /** * @brief Release the lock held by this object. * @@ -358,27 +252,22 @@ public: * the destructor will release the lock. **/ void unlock() { - if (_mutex != NULL) { - _mutex->Unlock(); - _mutex = NULL; + if (_guard) { + _guard.unlock(); } } /** * @brief Release the lock held by this object if unlock has not * been called. **/ - ~LockGuard() { - if (_mutex != NULL) { - _mutex->Unlock(); - } - } + ~LockGuard() = default; /** * Allow code to match guard with lock. This allows functions to take a * guard ref as input, ensuring that the caller have grabbed a lock. */ bool locks(const Lock& lock) const { - return (_mutex != NULL && _mutex == &lock._mutex); + return (_guard && _guard.mutex() == &lock._mutex); } }; @@ -399,25 +288,27 @@ public: class MonitorGuard { private: - FastOS_CondInterface *_cond; - MonitorGuard &operator=(const MonitorGuard &); + std::unique_lock<std::mutex> _guard; + std::condition_variable *_cond; + MonitorGuard &operator=(const MonitorGuard &) = delete; - FastOS_CondInterface *stealCond() { - FastOS_CondInterface *ret = _cond; - _cond = NULL; - return ret; - } public: /** * @brief A noop guard without any condition. **/ - MonitorGuard() : _cond(NULL) {} + MonitorGuard() : _guard(), _cond(nullptr) {} + MonitorGuard(const MonitorGuard &rhs) = delete; /** * @brief Steal the lock from the given MonitorGuard * * @param rhs steal the lock from this one **/ - MonitorGuard(MonitorGuard &rhs) : _cond(rhs.stealCond()) {} + MonitorGuard(MonitorGuard &&rhs) + : _guard(std::move(rhs._guard)), + _cond(rhs._cond) + { + rhs._cond = nullptr; + } /** * @brief Obtain the lock on the given Monitor object. * @@ -425,8 +316,10 @@ public: * * @param monitor take the lock on it **/ - MonitorGuard(const Monitor &monitor) : _cond(&monitor._cond) { - _cond->Lock(); + MonitorGuard(const Monitor &monitor) + : _guard(monitor._mutex), + _cond(&monitor._cond) + { } /** * @brief Create a MonitorGuard from a TryLock. @@ -437,13 +330,27 @@ public: * * @param tlock take the lock from this one **/ - MonitorGuard(TryLock &tlock) : _cond(tlock.stealCond()) {} -#ifndef IAM_DOXYGEN - MonitorGuard(const MonitorGuardHandover &rhs) : _cond(rhs._cond) {} - operator MonitorGuardHandover() { - return MonitorGuardHandover(stealCond()); + MonitorGuard(TryLock &&tlock) + : _guard(), + _cond(nullptr) + { + if (tlock._guard && tlock._cond != nullptr) { + _guard = std::move(tlock._guard); + _cond = tlock._cond; + tlock._cond = nullptr; + } + } + + MonitorGuard &operator=(MonitorGuard &&rhs) { + if (this != &rhs) { + _guard = std::move(rhs._guard); + _cond = rhs._cond; + rhs._cond = nullptr; + } + return *this; } -#endif + + /** * @brief Release the lock held by this object. * @@ -453,8 +360,8 @@ public: * the destructor will release the lock. **/ void unlock() { - assert(_cond != NULL); - _cond->Unlock(); + assert(_guard); + _guard.unlock(); _cond = NULL; } /** @@ -462,7 +369,7 @@ public: **/ void wait() { assert(_cond != NULL); - _cond->Wait(); + _cond->wait(_guard); } /** * @brief Wait for a signal on the underlying Monitor with the @@ -473,7 +380,7 @@ public: **/ bool wait(int msTimeout) { assert(_cond != NULL); - return _cond->TimedWait(msTimeout); + return _cond->wait_for(_guard, std::chrono::milliseconds(msTimeout)) == std::cv_status::no_timeout; } /** * @brief Send a signal to a single waiter on the underlying @@ -481,14 +388,14 @@ public: **/ void signal() { assert(_cond != NULL); - _cond->Signal(); + _cond->notify_one(); } /** * @brief Send a signal to all waiters on the underlying Monitor. **/ void broadcast() { assert(_cond != NULL); - _cond->Broadcast(); + _cond->notify_all(); } /** * @brief Send a signal to a single waiter on the underlying @@ -500,8 +407,8 @@ public: **/ void unsafeSignalUnlock() { assert(_cond != NULL); - _cond->Unlock(); - _cond->Signal(); + _guard.unlock(); + _cond->notify_one(); _cond = NULL; } /** @@ -514,19 +421,15 @@ public: **/ void unsafeBroadcastUnlock() { assert(_cond != NULL); - _cond->Unlock(); - _cond->Broadcast(); + _guard.unlock(); + _cond->notify_all(); _cond = NULL; } /** * @brief Release the lock held by this object if unlock has not * been called. **/ - ~MonitorGuard() { - if (_cond != NULL) { - _cond->Unlock(); - } - } + ~MonitorGuard() = default; /** * Allow code to match guard with lock. This allows functions to take a @@ -642,8 +545,8 @@ private: Monitor _monitor; uint32_t _count; - CountDownLatch(const CountDownLatch &rhs); - CountDownLatch &operator=(const CountDownLatch &rhs); + CountDownLatch(const CountDownLatch &rhs) = delete; + CountDownLatch &operator=(const CountDownLatch &rhs) = delete; public: /** |