From c5e350eeaf72810f0b22d28d9ef87834213eec1e Mon Sep 17 00:00:00 2001 From: Tor Egge Date: Fri, 4 Jun 2021 18:41:18 +0200 Subject: Use gcc 10 on CentOS 7, CentOS 8, RHEL 7 and RHEL 8. --- dist/vespa.spec | 37 ++++++++++++++----------------------- 1 file changed, 14 insertions(+), 23 deletions(-) diff --git a/dist/vespa.spec b/dist/vespa.spec index 34afc46c80c..ddaf598079f 100644 --- a/dist/vespa.spec +++ b/dist/vespa.spec @@ -42,11 +42,11 @@ BuildRequires: maven %define _java_home /usr/lib/jvm/java-11-amazon-corretto.%{?_arch} BuildRequires: python3-pytest %else -BuildRequires: devtoolset-9-gcc-c++ -BuildRequires: devtoolset-9-libatomic-devel -BuildRequires: devtoolset-9-binutils +BuildRequires: devtoolset-10-gcc-c++ +BuildRequires: devtoolset-10-libatomic-devel +BuildRequires: devtoolset-10-binutils BuildRequires: rh-maven35 -%define _devtoolset_enable /opt/rh/devtoolset-9/enable +%define _devtoolset_enable /opt/rh/devtoolset-10/enable %define _rhmaven35_enable /opt/rh/rh-maven35/enable BuildRequires: python36-pytest %endif @@ -54,19 +54,10 @@ BuildRequires: vespa-pybind11-devel BuildRequires: python3-devel %endif %if 0%{?el8} -%if 0%{?centos} -%global _centos_stream %(grep -qs '^NAME="CentOS Stream"' /etc/os-release && echo 1 || echo 0) -%endif -%if 0%{?_centos_stream} BuildRequires: gcc-toolset-10-gcc-c++ BuildRequires: gcc-toolset-10-binutils %define _devtoolset_enable /opt/rh/gcc-toolset-10/enable -BuildRequires: vespa-boost-devel >= 1.75.0-1 -%else -BuildRequires: gcc-toolset-9-gcc-c++ -BuildRequires: gcc-toolset-9-binutils -%define _devtoolset_enable /opt/rh/gcc-toolset-9/enable -%endif +BuildRequires: vespa-boost-devel >= 1.76.0-1 BuildRequires: maven BuildRequires: pybind11-devel BuildRequires: python3-pytest @@ -82,7 +73,7 @@ BuildRequires: python3-devel %if 0%{?el7} BuildRequires: cmake3 BuildRequires: llvm7.0-devel -BuildRequires: vespa-boost-devel >= 1.59.0-6 +BuildRequires: vespa-boost-devel >= 1.76.0-1 BuildRequires: vespa-gtest >= 1.8.1-1 BuildRequires: vespa-icu-devel >= 65.1.0-1 BuildRequires: vespa-lz4-devel >= 1.9.2-2 @@ -101,12 +92,12 @@ BuildRequires: vespa-libzstd-devel >= 1.4.5-2 %endif %if 0%{?el8} BuildRequires: cmake >= 3.11.4-3 -%if 0%{?_centos_stream} -BuildRequires: llvm-devel >= 11.0.0 +%if 0%{?centos} +BuildRequires: (llvm-devel >= 11.0.0 and llvm-devel < 12) %else -BuildRequires: llvm-devel >= 10.0.1 +BuildRequires: (llvm-devel >= 10.0.1 and llvm-devel < 11) %endif -BuildRequires: boost-devel >= 1.66 +BuildRequires: vespa-boost-devel >= 1.76.0-1 BuildRequires: openssl-devel BuildRequires: vespa-gtest >= 1.8.1-1 BuildRequires: vespa-lz4-devel >= 1.9.2-2 @@ -225,7 +216,7 @@ Requires: vespa-valgrind >= 3.17.0-1 %endif %endif %if 0%{?el8} -%if 0%{?_centos_stream} +%if 0%{?centos} %define _vespa_llvm_version 11 %else %define _vespa_llvm_version 10 @@ -353,10 +344,10 @@ Requires: libicu Requires: openssl-libs %endif %if 0%{?el8} -%if 0%{?_centos_stream} -Requires: llvm-libs >= 11.0.0 +%if 0%{?centos} +Requires: (llvm-libs >= 11.0.0 and llvm-libs < 12) %else -Requires: llvm-libs >= 10.0.1 +Requires: (llvm-libs >= 10.0.1 and llvm-libs < 11) %endif Requires: vespa-protobuf = 3.7.0-5.el8 %endif -- cgit v1.2.3 From 1eeb73f0e709044a6d8c6d68c5d85da24c90007f Mon Sep 17 00:00:00 2001 From: Tor Egge Date: Fri, 4 Jun 2021 20:51:32 +0200 Subject: CentOS 8 has llvm 11. --- default_build_settings.cmake | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/default_build_settings.cmake b/default_build_settings.cmake index 326481e25c7..a9c3f55dc61 100644 --- a/default_build_settings.cmake +++ b/default_build_settings.cmake @@ -31,11 +31,7 @@ endfunction() function(setup_vespa_default_build_settings_centos_8) message("-- Setting up default build settings for centos 8") set(DEFAULT_EXTRA_INCLUDE_DIRECTORY "${VESPA_DEPS}/include" "/usr/include/openblas" PARENT_SCOPE) - if (VESPA_OS_DISTRO_NAME STREQUAL "CentOS Stream") - set(DEFAULT_VESPA_LLVM_VERSION "11" PARENT_SCOPE) - else() - set(DEFAULT_VESPA_LLVM_VERSION "10" PARENT_SCOPE) - endif() + set(DEFAULT_VESPA_LLVM_VERSION "11" PARENT_SCOPE) endfunction() function(setup_vespa_default_build_settings_darwin) -- cgit v1.2.3 From 0f21830a9e41d0e917ec84c8499f43af209cdd7e Mon Sep 17 00:00:00 2001 From: Ola Aunrønning Date: Tue, 15 Jun 2021 17:31:50 +0200 Subject: Deprovision nodes if not parked by operator --- .../vespa/hosted/provision/maintenance/ParkedExpirer.java | 10 +++++++++- .../vespa/hosted/provision/maintenance/ParkedExpirerTest.java | 8 ++++---- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ParkedExpirer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ParkedExpirer.java index ec7826658e3..dec1f49145b 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ParkedExpirer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ParkedExpirer.java @@ -41,13 +41,14 @@ public class ParkedExpirer extends NodeRepositoryMaintainer { NodeList parkedHosts = nodeRepository.nodes() .list(Node.State.parked) .nodeType(NodeType.host) + .not().matching(this::parkedByOperator) .not().deprovisioning(); int hostsToExpire = Math.max(0, parkedHosts.size() - MAX_ALLOWED_PARKED_HOSTS); parkedHosts.sortedBy(Comparator.comparing(this::parkedAt)) .first(hostsToExpire) .forEach(host -> { log.info("Allowed number of parked nodes exceeded. Recycling " + host.hostname()); - nodeRepository.nodes().deallocate(host, Agent.ParkedExpirer, "Expired by ParkedExpirer"); + nodeRepository.nodes().deprovision(host.hostname(), Agent.ParkedExpirer, Instant.now()); }); return 1.0; @@ -59,4 +60,11 @@ public class ParkedExpirer extends NodeRepositoryMaintainer { .orElse(Instant.EPOCH); // Should not happen } + private boolean parkedByOperator(Node node) { + return node.history().event(History.Event.Type.parked) + .map(History.Event::agent) + .map(Agent.operator::equals) + .orElse(false); + } + } diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ParkedExpirerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ParkedExpirerTest.java index bc60801c1d6..c85a38cc6aa 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ParkedExpirerTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ParkedExpirerTest.java @@ -36,8 +36,8 @@ public class ParkedExpirerTest { var expirer = new ParkedExpirer(tester.nodeRepository(), Duration.ofMinutes(4), new TestMetric()); expirer.maintain(); - assertEquals(0, tester.nodeRepository().nodes().list(Node.State.dirty).size()); - assertEquals(25, tester.nodeRepository().nodes().list(Node.State.parked).size()); + assertEquals(1, tester.nodeRepository().nodes().list().deprovisioning().size()); + assertEquals(24, tester.nodeRepository().nodes().list().not().deprovisioning().size()); } @Test @@ -48,8 +48,8 @@ public class ParkedExpirerTest { var expirer = new ParkedExpirer(tester.nodeRepository(), Duration.ofMinutes(4), new TestMetric()); expirer.maintain(); - assertEquals(4, tester.nodeRepository().nodes().list(Node.State.dirty).size()); - assertEquals(21, tester.nodeRepository().nodes().list(Node.State.parked).size()); + assertEquals(5, tester.nodeRepository().nodes().list().deprovisioning().size()); + assertEquals(20, tester.nodeRepository().nodes().list().not().deprovisioning().size()); } -- cgit v1.2.3 From 0c3afd47b0e2f43bc9aa9e4e60d33c104c37b81b Mon Sep 17 00:00:00 2001 From: Jon Bratseth Date: Tue, 15 Jun 2021 19:43:59 +0200 Subject: Require replacements to be applied during tokenization --- .../linguistics/LinguisticsAnnotator.java | 14 ++--------- .../linguistics/LinguisticsAnnotatorTestCase.java | 27 +++++++++------------- .../java/com/yahoo/language/process/Token.java | 8 +++---- .../java/com/yahoo/language/process/Tokenizer.java | 11 +++------ .../com/yahoo/language/simple/SimpleToken.java | 4 ++++ 5 files changed, 24 insertions(+), 40 deletions(-) diff --git a/indexinglanguage/src/main/java/com/yahoo/vespa/indexinglanguage/linguistics/LinguisticsAnnotator.java b/indexinglanguage/src/main/java/com/yahoo/vespa/indexinglanguage/linguistics/LinguisticsAnnotator.java index 8b6ef83f05e..81a5305a778 100644 --- a/indexinglanguage/src/main/java/com/yahoo/vespa/indexinglanguage/linguistics/LinguisticsAnnotator.java +++ b/indexinglanguage/src/main/java/com/yahoo/vespa/indexinglanguage/linguistics/LinguisticsAnnotator.java @@ -115,9 +115,7 @@ public class LinguisticsAnnotator { } return; } - if ( ! token.isIndexable()) { - return; - } + if ( ! token.isIndexable()) return; } String orig = token.getOrig(); int pos = (int)token.getOffset(); @@ -137,9 +135,6 @@ public class LinguisticsAnnotator { String lowercasedTerm = lowercasedOrig; String term = token.getTokenString(); - if (term != null) { - term = tokenizer.getReplacementTerm(term); - } if (term != null) { lowercasedTerm = toLowerCase(term); } @@ -155,12 +150,7 @@ public class LinguisticsAnnotator { } } else { String term = token.getTokenString(); - if (term != null) { - term = tokenizer.getReplacementTerm(term); - } - if (term == null || term.trim().isEmpty()) { - return; - } + if (term == null || term.trim().isEmpty()) return; if (termOccurrences.termCountBelowLimit(term)) { parent.span(pos, len).annotate(lowerCaseTermAnnotation(term, token.getOrig())); } diff --git a/indexinglanguage/src/test/java/com/yahoo/vespa/indexinglanguage/linguistics/LinguisticsAnnotatorTestCase.java b/indexinglanguage/src/test/java/com/yahoo/vespa/indexinglanguage/linguistics/LinguisticsAnnotatorTestCase.java index afbcf597a46..5f436720990 100644 --- a/indexinglanguage/src/test/java/com/yahoo/vespa/indexinglanguage/linguistics/LinguisticsAnnotatorTestCase.java +++ b/indexinglanguage/src/test/java/com/yahoo/vespa/indexinglanguage/linguistics/LinguisticsAnnotatorTestCase.java @@ -19,6 +19,7 @@ import org.junit.Test; import org.mockito.Mockito; import java.util.*; +import java.util.stream.Collectors; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; @@ -30,12 +31,6 @@ public class LinguisticsAnnotatorTestCase { private static final AnnotatorConfig CONFIG = new AnnotatorConfig(); - // -------------------------------------------------------------------------------- - // - // Tests - // - // -------------------------------------------------------------------------------- - @Test public void requireThatAnnotateFailsWithZeroTokens() { assertAnnotations(null, "foo"); @@ -145,7 +140,7 @@ public class LinguisticsAnnotatorTestCase { continue; } assertAnnotations(expected, "foo", - newLinguistics(Arrays.asList(newToken("foo", "foo", type, specialToken)), + newLinguistics(List.of(newToken("foo", "foo", type, specialToken)), Collections.singletonMap("foo", "bar"))); } } @@ -159,7 +154,7 @@ public class LinguisticsAnnotatorTestCase { StringFieldValue val = new StringFieldValue("foo"); val.setSpanTree(spanTree); - Linguistics linguistics = newLinguistics(Arrays.asList(newToken("foo", "bar", TokenType.ALPHABETIC, false)), + Linguistics linguistics = newLinguistics(List.of(newToken("foo", "bar", TokenType.ALPHABETIC, false)), Collections.emptyMap()); new LinguisticsAnnotator(linguistics, CONFIG).annotate(val); @@ -253,11 +248,15 @@ public class LinguisticsAnnotatorTestCase { private static class MyTokenizer implements Tokenizer { final List tokens; - final Map replacementTerms; public MyTokenizer(List tokens, Map replacementTerms) { - this.tokens = new ArrayList<>(tokens); - this.replacementTerms = replacementTerms; + this.tokens = tokens.stream().map(token -> replace(token, replacementTerms)).collect(Collectors.toList()); + } + + private Token replace(Token token, Map replacementTerms) { + var simpleToken = (SimpleToken)token; + simpleToken.setTokenString(replacementTerms.getOrDefault(token.getTokenString(), token.getTokenString())); + return simpleToken; } @Override @@ -265,10 +264,6 @@ public class LinguisticsAnnotatorTestCase { return tokens; } - @Override - public String getReplacementTerm(String term) { - String replacement = replacementTerms.get(term); - return replacement != null ? replacement : term; - } } + } diff --git a/linguistics/src/main/java/com/yahoo/language/process/Token.java b/linguistics/src/main/java/com/yahoo/language/process/Token.java index 73c0ac857ab..70b78ef1a92 100644 --- a/linguistics/src/main/java/com/yahoo/language/process/Token.java +++ b/linguistics/src/main/java/com/yahoo/language/process/Token.java @@ -38,12 +38,12 @@ public interface Token { TokenScript getScript(); /** - * Returns token string in a form suitable for indexing: The - * most lowercased variant of the most processed token form available. + * Returns the token string in a form suitable for indexing: The + * most lowercased variant of the most processed token form available, * If called on a compound token this returns a lowercased form of the * entire word. - * - * @return token string value + * If this is a special token with a configured replacement, + * this will return the replacement token. */ String getTokenString(); diff --git a/linguistics/src/main/java/com/yahoo/language/process/Tokenizer.java b/linguistics/src/main/java/com/yahoo/language/process/Tokenizer.java index 7e61cd885a8..5be0a6fa635 100644 --- a/linguistics/src/main/java/com/yahoo/language/process/Tokenizer.java +++ b/linguistics/src/main/java/com/yahoo/language/process/Tokenizer.java @@ -23,16 +23,11 @@ public interface Tokenizer { Iterable tokenize(String input, Language language, StemMode stemMode, boolean removeAccents); /** - * Return a replacement for an input token string. - * This accepts strings returned by Token.getTokenString - * and returns a replacement which will be used as the index token. - * The input token string is returned if there is no replacement. - *

- * This default implementation always returns the input token string. + * Not used. * - * @param tokenString the token string of the term to lookup a replacement for - * @return the replacement, if any, or the argument token string if not + * @deprecated replacements are already applied in tokens returned by tokenize */ + @Deprecated // Remove on Vespa 8 default String getReplacementTerm(String tokenString) { return tokenString; } } diff --git a/linguistics/src/main/java/com/yahoo/language/simple/SimpleToken.java b/linguistics/src/main/java/com/yahoo/language/simple/SimpleToken.java index 122b9b6dff6..7b63650fa94 100644 --- a/linguistics/src/main/java/com/yahoo/language/simple/SimpleToken.java +++ b/linguistics/src/main/java/com/yahoo/language/simple/SimpleToken.java @@ -25,6 +25,10 @@ public class SimpleToken implements Token { this.orig = orig; } + public SimpleToken(String orig, String tokenString) { + this.orig = orig; + } + @Override public String getOrig() { return orig; -- cgit v1.2.3 From ad099ae536d1563fcfd8fa079b7972f596763959 Mon Sep 17 00:00:00 2001 From: Arne Juul Date: Wed, 16 Jun 2021 12:42:20 +0000 Subject: tighten limits slightly when feature flag is on --- config-model/src/main/java/com/yahoo/vespa/model/ConfigSentinel.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config-model/src/main/java/com/yahoo/vespa/model/ConfigSentinel.java b/config-model/src/main/java/com/yahoo/vespa/model/ConfigSentinel.java index d05913143e4..6203f78fc0c 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/ConfigSentinel.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/ConfigSentinel.java @@ -86,8 +86,8 @@ public class ConfigSentinel extends AbstractService implements SentinelConfig.Pr private SentinelConfig.Connectivity.Builder getConnectivityConfig(boolean enable) { var builder = new SentinelConfig.Connectivity.Builder(); if (enable) { - builder.minOkPercent(40); - builder.maxBadCount(3); + builder.minOkPercent(50); + builder.maxBadCount(2); } else { builder.minOkPercent(0); builder.maxBadCount(Integer.MAX_VALUE); -- cgit v1.2.3 From d20ccaa2c948c6bb507f36cd69db801d99c87fd9 Mon Sep 17 00:00:00 2001 From: Henning Baldersheim Date: Wed, 16 Jun 2021 15:38:18 +0200 Subject: Various cleanup when reading grouping code. --- .../search/grouping/request/GroupingOperation.java | 7 +++- .../com/yahoo/searchlib/aggregation/Group.java | 40 ++++++++++------------ .../searchlib/aggregation/GroupingTestCase.java | 2 ++ 3 files changed, 27 insertions(+), 22 deletions(-) diff --git a/container-search/src/main/java/com/yahoo/search/grouping/request/GroupingOperation.java b/container-search/src/main/java/com/yahoo/search/grouping/request/GroupingOperation.java index 8b73fa01128..499ed610d34 100644 --- a/container-search/src/main/java/com/yahoo/search/grouping/request/GroupingOperation.java +++ b/container-search/src/main/java/com/yahoo/search/grouping/request/GroupingOperation.java @@ -8,7 +8,12 @@ import com.yahoo.search.grouping.request.parser.GroupingParserInput; import com.yahoo.search.grouping.request.parser.ParseException; import com.yahoo.search.grouping.request.parser.TokenMgrException; -import java.util.*; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Set; + /** * This class represents a single node in a grouping operation tree. You may manually construct this tree, or you may diff --git a/searchlib/src/main/java/com/yahoo/searchlib/aggregation/Group.java b/searchlib/src/main/java/com/yahoo/searchlib/aggregation/Group.java index c508296d739..c7080ec28d8 100644 --- a/searchlib/src/main/java/com/yahoo/searchlib/aggregation/Group.java +++ b/searchlib/src/main/java/com/yahoo/searchlib/aggregation/Group.java @@ -4,9 +4,17 @@ package com.yahoo.searchlib.aggregation; import com.yahoo.searchlib.expression.AggregationRefNode; import com.yahoo.searchlib.expression.ExpressionNode; import com.yahoo.searchlib.expression.ResultNode; -import com.yahoo.vespa.objects.*; - -import java.util.*; +import com.yahoo.vespa.objects.Deserializer; +import com.yahoo.vespa.objects.Identifiable; +import com.yahoo.vespa.objects.ObjectOperation; +import com.yahoo.vespa.objects.ObjectPredicate; +import com.yahoo.vespa.objects.ObjectVisitor; +import com.yahoo.vespa.objects.Serializer; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.Iterator; +import java.util.List; public class Group extends Identifiable { @@ -132,11 +140,7 @@ public class Group extends Identifiable { if (sortType == SortType.BYID) { return; } - Collections.sort(children, new Comparator() { - public int compare(Group lhs, Group rhs) { - return lhs.compareId(rhs); - } - }); + Collections.sort(children, (Group lhs, Group rhs) -> lhs.compareId(rhs)); sortType = SortType.BYID; } @@ -145,11 +149,8 @@ public class Group extends Identifiable { if (sortType == SortType.BYRANK) { return; } - Collections.sort(children, new Comparator() { - public int compare(Group lhs, Group rhs) { - return lhs.compareRank(rhs); - } - }); + Collections.sort(children, (Group lhs, Group rhs) -> lhs.compareRank(rhs) ); + sortType = SortType.BYRANK; } @@ -403,22 +404,19 @@ public class Group extends Identifiable { if (id != null) { obj.id = (ResultNode)id.clone(); } - obj.aggregationResults = new ArrayList(); + obj.aggregationResults = new ArrayList<>(); for (AggregationResult result : aggregationResults) { obj.aggregationResults.add(result.clone()); } - obj.orderByIdx = new ArrayList(); - for (Integer idx : orderByIdx) { - obj.orderByIdx.add(idx); - } - obj.orderByExp = new ArrayList(); + obj.orderByIdx = new ArrayList<>(orderByIdx); + obj.orderByExp = new ArrayList<>(); RefResolver resolver = new RefResolver(obj); for (ExpressionNode exp : orderByExp) { exp = exp.clone(); exp.select(REF_LOCATOR, resolver); obj.orderByExp.add(exp); } - obj.children = new ArrayList(); + obj.children = new ArrayList<>(); for (Group child : children) { obj.children.add(child.clone()); } @@ -447,7 +445,7 @@ public class Group extends Identifiable { } } - private static enum SortType { + private enum SortType { UNSORTED, BYRANK, BYID diff --git a/searchlib/src/test/java/com/yahoo/searchlib/aggregation/GroupingTestCase.java b/searchlib/src/test/java/com/yahoo/searchlib/aggregation/GroupingTestCase.java index ec379e5f8af..e6143a17523 100644 --- a/searchlib/src/test/java/com/yahoo/searchlib/aggregation/GroupingTestCase.java +++ b/searchlib/src/test/java/com/yahoo/searchlib/aggregation/GroupingTestCase.java @@ -1,6 +1,7 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.searchlib.aggregation; +import com.yahoo.searchlib.expression.FloatResultNode; import com.yahoo.searchlib.expression.NullResultNode; import com.yahoo.searchlib.expression.StringBucketResultNode; import com.yahoo.vespa.objects.BufferSerializer; @@ -186,6 +187,7 @@ public class GroupingTestCase { public void requireThatNeedDeepResultCollectionWorks() { assertFalse(new Grouping().addLevel(new GroupingLevel().setGroupPrototype(new Group())).needDeepResultCollection()); assertTrue(new Grouping().addLevel(new GroupingLevel().setGroupPrototype(new Group().addOrderBy(new CountAggregationResult(9), true))).needDeepResultCollection()); + assertTrue(new Grouping().addLevel(new GroupingLevel().setGroupPrototype(new Group().addOrderBy(new AverageAggregationResult(), true))).needDeepResultCollection()); } @Test -- cgit v1.2.3 From e7cf792d47d8888d968a152edc85093d725f0c82 Mon Sep 17 00:00:00 2001 From: Ola Aunrønning Date: Wed, 16 Jun 2021 15:38:12 +0200 Subject: Remove ParkedExpirer, ProvisionedExpirer deprovisions if excessive expiry --- .../maintenance/NodeRepositoryMaintenance.java | 3 - .../provision/maintenance/ParkedExpirer.java | 70 --------------------- .../provision/maintenance/ProvisionedExpirer.java | 30 ++++++++- .../provision/maintenance/ParkedExpirerTest.java | 71 ---------------------- .../maintenance/ProvisionedExpirerTest.java | 50 +++++++++++++++ .../provision/restapi/responses/maintenance.json | 3 - 6 files changed, 78 insertions(+), 149 deletions(-) delete mode 100644 node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ParkedExpirer.java delete mode 100644 node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ParkedExpirerTest.java create mode 100644 node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ProvisionedExpirerTest.java diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRepositoryMaintenance.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRepositoryMaintenance.java index cdb5202603a..96373bd764f 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRepositoryMaintenance.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRepositoryMaintenance.java @@ -67,7 +67,6 @@ public class NodeRepositoryMaintenance extends AbstractComponent { maintainers.add(new ScalingSuggestionsMaintainer(nodeRepository, defaults.scalingSuggestionsInterval, metric)); maintainers.add(new SwitchRebalancer(nodeRepository, defaults.switchRebalancerInterval, metric, deployer)); maintainers.add(new HostEncrypter(nodeRepository, defaults.hostEncrypterInterval, metric)); - maintainers.add(new ParkedExpirer(nodeRepository, defaults.parkedExpirerInterval, metric)); provisionServiceProvider.getLoadBalancerService(nodeRepository) .map(lbService -> new LoadBalancerExpirer(nodeRepository, defaults.loadBalancerExpirerInterval, lbService, metric)) @@ -120,7 +119,6 @@ public class NodeRepositoryMaintenance extends AbstractComponent { private final Duration scalingSuggestionsInterval; private final Duration switchRebalancerInterval; private final Duration hostEncrypterInterval; - private final Duration parkedExpirerInterval; private final NodeFailer.ThrottlePolicy throttlePolicy; @@ -151,7 +149,6 @@ public class NodeRepositoryMaintenance extends AbstractComponent { throttlePolicy = NodeFailer.ThrottlePolicy.hosted; inactiveConfigServerExpiry = Duration.ofMinutes(5); inactiveControllerExpiry = Duration.ofMinutes(5); - parkedExpirerInterval = Duration.ofMinutes(30); if (zone.environment().isProduction() && ! zone.system().isCd()) { inactiveExpiry = Duration.ofHours(4); // enough time for the application owner to discover and redeploy diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ParkedExpirer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ParkedExpirer.java deleted file mode 100644 index dec1f49145b..00000000000 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ParkedExpirer.java +++ /dev/null @@ -1,70 +0,0 @@ -// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -package com.yahoo.vespa.hosted.provision.maintenance; - -import com.yahoo.config.provision.NodeType; -import com.yahoo.jdisc.Metric; -import com.yahoo.vespa.hosted.provision.Node; -import com.yahoo.vespa.hosted.provision.NodeList; -import com.yahoo.vespa.hosted.provision.NodeRepository; -import com.yahoo.vespa.hosted.provision.node.Agent; -import com.yahoo.vespa.hosted.provision.node.History; - -import java.time.Duration; -import java.time.Instant; -import java.util.Comparator; -import java.util.logging.Logger; - -/** - * - * Expires parked nodes in dynamically provisioned zones. - * If number of parked hosts exceed MAX_ALLOWED_PARKED_HOSTS, recycle in a queue order - * - * @author olaa - */ -public class ParkedExpirer extends NodeRepositoryMaintainer { - - private static final int MAX_ALLOWED_PARKED_HOSTS = 20; - private static final Logger log = Logger.getLogger(ParkedExpirer.class.getName()); - - private final NodeRepository nodeRepository; - - ParkedExpirer(NodeRepository nodeRepository, Duration interval, Metric metric) { - super(nodeRepository, interval, metric); - this.nodeRepository = nodeRepository; - } - - @Override - protected double maintain() { - if (!nodeRepository.zone().getCloud().dynamicProvisioning()) - return 1.0; - - NodeList parkedHosts = nodeRepository.nodes() - .list(Node.State.parked) - .nodeType(NodeType.host) - .not().matching(this::parkedByOperator) - .not().deprovisioning(); - int hostsToExpire = Math.max(0, parkedHosts.size() - MAX_ALLOWED_PARKED_HOSTS); - parkedHosts.sortedBy(Comparator.comparing(this::parkedAt)) - .first(hostsToExpire) - .forEach(host -> { - log.info("Allowed number of parked nodes exceeded. Recycling " + host.hostname()); - nodeRepository.nodes().deprovision(host.hostname(), Agent.ParkedExpirer, Instant.now()); - }); - - return 1.0; - } - - private Instant parkedAt(Node node) { - return node.history().event(History.Event.Type.parked) - .map(History.Event::at) - .orElse(Instant.EPOCH); // Should not happen - } - - private boolean parkedByOperator(Node node) { - return node.history().event(History.Event.Type.parked) - .map(History.Event::agent) - .map(Agent.operator::equals) - .orElse(false); - } - -} diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ProvisionedExpirer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ProvisionedExpirer.java index 856d534bbd2..c71eef0abea 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ProvisionedExpirer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ProvisionedExpirer.java @@ -4,15 +4,18 @@ package com.yahoo.vespa.hosted.provision.maintenance; import com.yahoo.config.provision.NodeType; import com.yahoo.jdisc.Metric; import com.yahoo.vespa.hosted.provision.Node; +import com.yahoo.vespa.hosted.provision.NodeList; import com.yahoo.vespa.hosted.provision.NodeRepository; import com.yahoo.vespa.hosted.provision.node.Agent; import com.yahoo.vespa.hosted.provision.node.History; import java.time.Duration; +import java.time.Instant; import java.util.List; /** * This moves nodes of type {@link NodeType#host} from provisioned to parked if they have been in provisioned too long. + * Parked hosts are deprovisioned as well, if too many hosts are being expired. * * Only {@link NodeType#host} is moved because any number of nodes of that type can exist. Other node types such as * {@link NodeType#confighost} have a fixed number and thus cannot be replaced while the fixed number of nodes exist in @@ -22,17 +25,40 @@ import java.util.List; */ public class ProvisionedExpirer extends Expirer { + private final NodeRepository nodeRepository; + private static final int MAXIMUM_ALLOWED_EXPIRED_HOSTS = 20; + ProvisionedExpirer(NodeRepository nodeRepository, Duration dirtyTimeout, Metric metric) { super(Node.State.provisioned, History.Event.Type.provisioned, nodeRepository, dirtyTimeout, metric); + this.nodeRepository = nodeRepository; } @Override protected void expire(List expired) { + int previouslyExpired = numberOfPreviouslyExpired(); for (Node expiredNode : expired) { - if (expiredNode.type() == NodeType.host) { - nodeRepository().nodes().parkRecursively(expiredNode.hostname(), Agent.ProvisionedExpirer, "Node is stuck in provisioned"); + if (expiredNode.type() != NodeType.host) + continue; + nodeRepository().nodes().parkRecursively(expiredNode.hostname(), Agent.ProvisionedExpirer, "Node is stuck in provisioned"); + if (MAXIMUM_ALLOWED_EXPIRED_HOSTS < ++previouslyExpired) { + nodeRepository.nodes().deprovision(expiredNode.hostname(), Agent.ProvisionedExpirer, Instant.now()); } } } + private int numberOfPreviouslyExpired() { + return nodeRepository.nodes() + .list(Node.State.parked) + .nodeType(NodeType.host) + .matching(this::parkedByProvisionedExpirer) + .not().deprovisioning() + .size(); + } + + private boolean parkedByProvisionedExpirer(Node node) { + return node.history().event(History.Event.Type.parked) + .map(History.Event::agent) + .map(Agent.ProvisionedExpirer::equals) + .orElse(false); + } } diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ParkedExpirerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ParkedExpirerTest.java deleted file mode 100644 index c85a38cc6aa..00000000000 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ParkedExpirerTest.java +++ /dev/null @@ -1,71 +0,0 @@ -package com.yahoo.vespa.hosted.provision.maintenance; - -import com.yahoo.config.provision.Cloud; -import com.yahoo.config.provision.Environment; -import com.yahoo.config.provision.Flavor; -import com.yahoo.config.provision.NodeResources; -import com.yahoo.config.provision.NodeType; -import com.yahoo.config.provision.RegionName; -import com.yahoo.config.provision.SystemName; -import com.yahoo.config.provision.Zone; -import com.yahoo.vespa.hosted.provision.Node; -import com.yahoo.vespa.hosted.provision.node.Agent; -import com.yahoo.vespa.hosted.provision.provisioning.ProvisioningTester; -import com.yahoo.vespa.hosted.provision.testutils.MockHostProvisioner; -import org.junit.Test; - -import java.time.Duration; -import java.util.List; -import java.util.stream.Collectors; -import java.util.stream.IntStream; - -import static org.junit.Assert.*; - -/** - * @author olaa - */ -public class ParkedExpirerTest { - - private ProvisioningTester tester; - - @Test - public void noop_if_not_dynamic_provisioning() { - tester = getTester(false); - populateNodeRepo(); - - var expirer = new ParkedExpirer(tester.nodeRepository(), Duration.ofMinutes(4), new TestMetric()); - expirer.maintain(); - - assertEquals(1, tester.nodeRepository().nodes().list().deprovisioning().size()); - assertEquals(24, tester.nodeRepository().nodes().list().not().deprovisioning().size()); - } - - @Test - public void recycles_correct_subset_of_parked_hosts() { - tester = getTester(true); - populateNodeRepo(); - - var expirer = new ParkedExpirer(tester.nodeRepository(), Duration.ofMinutes(4), new TestMetric()); - expirer.maintain(); - - assertEquals(5, tester.nodeRepository().nodes().list().deprovisioning().size()); - assertEquals(20, tester.nodeRepository().nodes().list().not().deprovisioning().size()); - - } - - private ProvisioningTester getTester(boolean dynamicProvisioning) { - var zone = new Zone(Cloud.builder().dynamicProvisioning(dynamicProvisioning).build(), SystemName.main, Environment.prod, RegionName.from("us-east")); - return new ProvisioningTester.Builder().zone(zone) - .hostProvisioner(dynamicProvisioning ? new MockHostProvisioner(List.of()) : null) - .build(); - } - - private void populateNodeRepo() { - var nodes = IntStream.range(0, 25) - .mapToObj(i -> Node.create("id-" + i, "host-" + i, new Flavor(NodeResources.unspecified()), Node.State.parked, NodeType.host).build()) - .collect(Collectors.toList()); - tester.nodeRepository().database().addNodesInState(nodes, Node.State.parked, Agent.system); - tester.nodeRepository().nodes().deprovision(nodes.get(0).hostname(), Agent.system, tester.clock().instant()); // Deprovisioning host is not recycled - } - -} diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ProvisionedExpirerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ProvisionedExpirerTest.java new file mode 100644 index 00000000000..786faae24b4 --- /dev/null +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ProvisionedExpirerTest.java @@ -0,0 +1,50 @@ +package com.yahoo.vespa.hosted.provision.maintenance; + +import com.yahoo.config.provision.Cloud; +import com.yahoo.config.provision.Environment; +import com.yahoo.config.provision.Flavor; +import com.yahoo.config.provision.NodeResources; +import com.yahoo.config.provision.NodeType; +import com.yahoo.config.provision.RegionName; +import com.yahoo.config.provision.SystemName; +import com.yahoo.config.provision.Zone; +import com.yahoo.vespa.hosted.provision.Node; +import com.yahoo.vespa.hosted.provision.node.Agent; +import com.yahoo.vespa.hosted.provision.provisioning.ProvisioningTester; +import com.yahoo.vespa.hosted.provision.testutils.MockHostProvisioner; +import org.junit.Test; + +import java.time.Duration; +import java.util.List; +import java.util.stream.Collectors; +import java.util.stream.IntStream; + +import static org.junit.Assert.*; + +/** + * @author olaa + */ +public class ProvisionedExpirerTest { + + private ProvisioningTester tester; + + @Test + public void deprovisions_hosts_if_excessive_expiry() { + tester = new ProvisioningTester.Builder().build(); + populateNodeRepo(); + + tester.clock().advance(Duration.ofMinutes(5)); + new ProvisionedExpirer(tester.nodeRepository(), Duration.ofMinutes(4), new TestMetric()).maintain(); + + assertEquals(5, tester.nodeRepository().nodes().list().deprovisioning().size()); + assertEquals(20, tester.nodeRepository().nodes().list().not().deprovisioning().size()); + } + + private void populateNodeRepo() { + var nodes = IntStream.range(0, 25) + .mapToObj(i -> Node.create("id-" + i, "host-" + i, new Flavor(NodeResources.unspecified()), Node.State.provisioned, NodeType.host).build()) + .collect(Collectors.toList()); + tester.nodeRepository().database().addNodesInState(nodes, Node.State.provisioned, Agent.system); + } + +} diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/maintenance.json b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/maintenance.json index 26d711945c6..72224ef3cba 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/maintenance.json +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/maintenance.json @@ -42,9 +42,6 @@ { "name": "OsUpgradeActivator" }, - { - "name": "ParkedExpirer" - }, { "name": "PeriodicApplicationMaintainer" }, -- cgit v1.2.3 From 8bc54077a7848ea4bb092973254b4c56da4fbb45 Mon Sep 17 00:00:00 2001 From: Tor Egge Date: Wed, 16 Jun 2021 15:50:31 +0200 Subject: Track change to devtoolset-10. --- .../hosted/node/admin/maintenance/coredump/CoreCollector.java | 2 +- .../hosted/node/admin/maintenance/coredump/CoreCollectorTest.java | 4 ++-- screwdriver/build-vespa.sh | 8 ++++---- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoreCollector.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoreCollector.java index 4c384b09fad..c746b5f3894 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoreCollector.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoreCollector.java @@ -29,7 +29,7 @@ public class CoreCollector { private static final Pattern CORE_GENERATOR_PATH_PATTERN = Pattern.compile("^Core was generated by `(?.*?)'.$"); private static final Pattern EXECFN_PATH_PATTERN = Pattern.compile("^.* execfn: '(?.*?)'"); private static final Pattern FROM_PATH_PATTERN = Pattern.compile("^.* from '(?.*?)'"); - static final String GDB_PATH = "/opt/rh/devtoolset-9/root/bin/gdb"; + static final String GDB_PATH = "/opt/rh/devtoolset-10/root/bin/gdb"; static final Map JAVA_HEAP_DUMP_METADATA = Map.of("bin_path", "java", "backtrace", List.of("Heap dump, no backtrace available")); diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoreCollectorTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoreCollectorTest.java index 2827e99c697..e8679714c6f 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoreCollectorTest.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoreCollectorTest.java @@ -86,7 +86,7 @@ public class CoreCollectorTest { fail("Expected not to be able to get bin path"); } catch (RuntimeException e) { assertEquals("Failed to extract binary path from GDB, result: ProcessResult { exitStatus=1 output= errors=Error 123 }, command: " + - "[/bin/sh, -c, /opt/rh/devtoolset-9/root/bin/gdb -n -batch -core /tmp/core.1234 | grep '^Core was generated by']", e.getMessage()); + "[/bin/sh, -c, /opt/rh/devtoolset-10/root/bin/gdb -n -batch -core /tmp/core.1234 | grep '^Core was generated by']", e.getMessage()); } } @@ -103,7 +103,7 @@ public class CoreCollectorTest { fail("Expected not to be able to read backtrace"); } catch (RuntimeException e) { assertEquals("Failed to read backtrace ProcessResult { exitStatus=1 output= errors=Failure }, Command: " + - "[/opt/rh/devtoolset-9/root/bin/gdb, -n, -ex, bt, -batch, /usr/bin/program, /tmp/core.1234]", e.getMessage()); + "[/opt/rh/devtoolset-10/root/bin/gdb, -n, -ex, bt, -batch, /usr/bin/program, /tmp/core.1234]", e.getMessage()); } } diff --git a/screwdriver/build-vespa.sh b/screwdriver/build-vespa.sh index 4480b33e6f9..91375728ca9 100755 --- a/screwdriver/build-vespa.sh +++ b/screwdriver/build-vespa.sh @@ -6,7 +6,7 @@ set -e readonly SOURCE_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd )" readonly NUM_THREADS=$(( $(nproc) + 2 )) -source /etc/profile.d/enable-devtoolset-9.sh +source /etc/profile.d/enable-devtoolset-10.sh source /etc/profile.d/enable-rh-maven35.sh export MALLOC_ARENA_MAX=1 @@ -52,9 +52,9 @@ esac if [[ $SHOULD_BUILD == systemtest ]]; then yum -y --setopt=skip_missing_names_on_install=False install \ zstd \ - devtoolset-9-gcc-c++ \ - devtoolset-9-libatomic-devel \ - devtoolset-9-binutils \ + devtoolset-10-gcc-c++ \ + devtoolset-10-libatomic-devel \ + devtoolset-10-binutils \ libxml2-devel \ rh-ruby27-rubygems-devel \ rh-ruby27-ruby-devel \ -- cgit v1.2.3 From 46393cdff3a66b88f0eef65753e862b81e94b320 Mon Sep 17 00:00:00 2001 From: Ola Aunrønning Date: Wed, 16 Jun 2021 15:53:52 +0200 Subject: Use noderepository clock --- .../yahoo/vespa/hosted/provision/maintenance/ProvisionedExpirer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ProvisionedExpirer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ProvisionedExpirer.java index c71eef0abea..76c8210338e 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ProvisionedExpirer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ProvisionedExpirer.java @@ -41,7 +41,7 @@ public class ProvisionedExpirer extends Expirer { continue; nodeRepository().nodes().parkRecursively(expiredNode.hostname(), Agent.ProvisionedExpirer, "Node is stuck in provisioned"); if (MAXIMUM_ALLOWED_EXPIRED_HOSTS < ++previouslyExpired) { - nodeRepository.nodes().deprovision(expiredNode.hostname(), Agent.ProvisionedExpirer, Instant.now()); + nodeRepository.nodes().deprovision(expiredNode.hostname(), Agent.ProvisionedExpirer, nodeRepository.clock().instant()); } } } -- cgit v1.2.3 From 8b28e3f8ef0569e26a40b9a8f71208610b38ed12 Mon Sep 17 00:00:00 2001 From: Tor Egge Date: Wed, 16 Jun 2021 15:54:02 +0200 Subject: Remove duplicate BuildRequires. --- dist/vespa.spec | 1 - 1 file changed, 1 deletion(-) diff --git a/dist/vespa.spec b/dist/vespa.spec index ddaf598079f..f4a4759860a 100644 --- a/dist/vespa.spec +++ b/dist/vespa.spec @@ -57,7 +57,6 @@ BuildRequires: python3-devel BuildRequires: gcc-toolset-10-gcc-c++ BuildRequires: gcc-toolset-10-binutils %define _devtoolset_enable /opt/rh/gcc-toolset-10/enable -BuildRequires: vespa-boost-devel >= 1.76.0-1 BuildRequires: maven BuildRequires: pybind11-devel BuildRequires: python3-pytest -- cgit v1.2.3 From a5d947fca330c7765b39051202eba11cf21d8c53 Mon Sep 17 00:00:00 2001 From: Harald Musum Date: Wed, 16 Jun 2021 19:32:50 +0200 Subject: Revert "Cleanup, no functional changes" --- .../yahoo/vespa/config/benchmark/LoadTester.java | 104 ++++++++++----------- 1 file changed, 50 insertions(+), 54 deletions(-) diff --git a/config/src/main/java/com/yahoo/vespa/config/benchmark/LoadTester.java b/config/src/main/java/com/yahoo/vespa/config/benchmark/LoadTester.java index f57da6921ab..3a8d80e5ffe 100644 --- a/config/src/main/java/com/yahoo/vespa/config/benchmark/LoadTester.java +++ b/config/src/main/java/com/yahoo/vespa/config/benchmark/LoadTester.java @@ -1,4 +1,4 @@ -// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +// Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.config.benchmark; import com.yahoo.collections.Tuple2; @@ -33,8 +33,6 @@ import java.util.Map; import java.util.Optional; import java.util.concurrent.ThreadLocalRandom; -import static com.yahoo.vespa.config.ConfigKey.createFull; - /** * A config client for generating load against a config server or config proxy. *

@@ -71,17 +69,18 @@ public class LoadTester { String configsList = parser.getBinarySwitches().get("-l"); String defPath = parser.getBinarySwitches().get("-dd"); debug = parser.getUnarySwitches().contains("-d"); - new LoadTester().runLoad(host, port, iterations, threads, configsList, defPath); + LoadTester loadTester = new LoadTester(); + loadTester.runLoad(host, port, iterations, threads, configsList, defPath); } private void runLoad(String host, int port, int iterations, int threads, String configsList, String defPath) throws IOException, InterruptedException { configs = readConfigs(configsList); defs = readDefs(defPath); - Metrics m = new Metrics(); List threadList = new ArrayList<>(); - long start = System.currentTimeMillis(); + Metrics m = new Metrics(); + for (int i = 0; i < threads; i++) { LoadThread lt = new LoadThread(iterations, host, port); threadList.add(lt); @@ -100,14 +99,12 @@ public class LoadTester { if (defPath == null) return ret; File defDir = new File(defPath); if (!defDir.isDirectory()) { - System.out.println("# Given def file dir is not a directory: " + defDir.getPath() + - " , will not send def contents in requests."); + System.out.println("# Given def file dir is not a directory: " + defDir.getPath() + " , will not send def contents in requests."); return ret; } final File[] files = defDir.listFiles(); if (files == null) { - System.out.println("# Given def file dir has no files: " + defDir.getPath() + - " , will not send def contents in requests."); + System.out.println("# Given def file dir has no files: " + defDir.getPath() + " , will not send def contents in requests."); return ret; } for (File f : files) { @@ -115,8 +112,7 @@ public class LoadTester { if (!name.endsWith(".def")) continue; String contents = IOUtils.readFile(f); ConfigDefinitionKey key = ConfigUtils.createConfigDefinitionKeyFromDefFile(f); - ret.put(key, new Tuple2<>(ConfigUtils.getDefMd5(Arrays.asList(contents.split("\n"))), - contents.split("\n"))); + ret.put(key, new Tuple2<>(ConfigUtils.getDefMd5(Arrays.asList(contents.split("\n"))), contents.split("\n"))); } System.out.println("# Read " + ret.size() + " def files from " + defDir.getPath()); return ret; @@ -135,7 +131,7 @@ public class LoadTester { sb.append((metrics.failedRequests)); sb.append("\n"); sb.append('#').append(TransportMetrics.getInstance().snapshot().toString()).append('\n'); - System.out.println(sb); + System.out.println(sb.toString()); } private List> readConfigs(String configsList) throws IOException { @@ -193,10 +189,10 @@ public class LoadTester { private class LoadThread extends Thread { - private final int iterations; - private final String host; - private final int port; - private final Metrics metrics = new Metrics(); + int iterations = 0; + String host = ""; + int port = 0; + Metrics metrics = new Metrics(); LoadThread(int iterations, String host, int port) { this.iterations = iterations; @@ -208,20 +204,44 @@ public class LoadTester { public void run() { Spec spec = new Spec(host, port); Target target = connect(spec); - + ConfigKey reqKey; + JRTClientConfigRequest request; + int totConfs = configs.size(); + boolean reconnCycle = false; // to log reconn message only once, for instance at restart for (int i = 0; i < iterations; i++) { - ConfigKey reqKey = configs.get(ThreadLocalRandom.current().nextInt(configs.size())); - JRTClientConfigRequest request = getRequest(reqKey); + reqKey = configs.get(ThreadLocalRandom.current().nextInt(totConfs)); + ConfigDefinitionKey dKey = new ConfigDefinitionKey(reqKey); + Tuple2 defContent = defs.get(dKey); + if (defContent == null && defs.size() > 0) { // Only complain if we actually did run with a def dir + System.out.println("# No def found for " + dKey + ", not sending in request."); + } + request = getRequest(ConfigKey.createFull(reqKey.getName(), reqKey.getConfigId(), reqKey.getNamespace(), defContent.first), defContent.second); if (debug) System.out.println("# Requesting: " + reqKey); - long start = System.currentTimeMillis(); target.invokeSync(request.getRequest(), 10.0); long end = System.currentTimeMillis(); - if (request.isError()) { - target = handleError(request, spec, target); + if ("Connection lost".equals(request.errorMessage()) || "Connection down".equals(request.errorMessage())) { + try { + Thread.sleep(100); + } catch (InterruptedException e) { + throw new RuntimeException(e); + } + if (!reconnCycle) { + System.out.println("# Connection lost, reconnecting..."); + reconnCycle = true; + } + target.close(); + target = connect(spec); + } else { + System.err.println(request.errorMessage()); + } + metrics.incFailedRequests(); } else { - System.out.println("# Connection OK"); + if (reconnCycle) { + reconnCycle = false; + System.out.println("# Connection OK"); + } long duration = end - start; if (debug) { @@ -235,36 +255,12 @@ public class LoadTester { } } - private Target handleError(JRTClientConfigRequest request, Spec spec, Target target) { - if (List.of("Connection lost", "Connection down").contains(request.errorMessage())) { - try { - Thread.sleep(100); - } catch (InterruptedException e) { - throw new RuntimeException(e); - } - System.out.println("# Connection lost, reconnecting..."); - target.close(); - target = connect(spec); - } else { - System.err.println(request.errorMessage()); - } - metrics.incFailedRequests(); - return target; - } - - private JRTClientConfigRequest getRequest(ConfigKey reqKey) { - long serverTimeout = 1000; - - ConfigDefinitionKey dKey = new ConfigDefinitionKey(reqKey); - Tuple2 defPair = defs.get(dKey); - - String defMd5 = defPair.first; - DefContent defContent = DefContent.fromList(List.of(defPair.second)); - - ConfigKey fullKey = createFull(reqKey.getName(), reqKey.getConfigId(), reqKey.getNamespace(), defMd5); - return JRTClientConfigRequestV3.createWithParams(fullKey, defContent, ConfigUtils.getCanonicalHostName(), - "", 0, serverTimeout, - Trace.createDummy(), compressionType, Optional.empty()); + private JRTClientConfigRequest getRequest(ConfigKey reqKey, String[] defContent) { + if (defContent == null) defContent = new String[0]; + final long serverTimeout = 1000; + return JRTClientConfigRequestV3.createWithParams(reqKey, DefContent.fromList(Arrays.asList(defContent)), + ConfigUtils.getCanonicalHostName(), "", 0, serverTimeout, Trace.createDummy(), + compressionType, Optional.empty()); } private Target connect(Spec spec) { -- cgit v1.2.3 From ccb22a673bb8c5bbb30c1933be26f82ccbc99868 Mon Sep 17 00:00:00 2001 From: Jon Bratseth Date: Wed, 16 Jun 2021 21:16:40 +0200 Subject: Reuse some application packages for performance --- .../controller/maintenance/UpgraderTest.java | 30 ++++++++++++++++++---- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/UpgraderTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/UpgraderTest.java index 484b471cbaa..326f4bf311e 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/UpgraderTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/UpgraderTest.java @@ -164,7 +164,6 @@ public class UpgraderTest { tester.triggerJobs(); assertEquals("Upgrade with error should retry", 1, tester.jobs().active().size()); - // --- Failing application is repaired by changing the application, causing confidence to move above 'high' threshold // Deploy application change default0.submit(applicationPackage("default")); @@ -1114,11 +1113,32 @@ public class UpgraderTest { assertEquals("Upgrade orders are distinct", versions.size(), upgradeOrders.size()); } + private static final ApplicationPackage canaryApplicationPackage = + new ApplicationPackageBuilder().upgradePolicy("canary") + .region("us-west-1") + .region("us-east-3") + .build(); + + private static final ApplicationPackage defaultApplicationPackage = + new ApplicationPackageBuilder().upgradePolicy("default") + .region("us-west-1") + .region("us-east-3") + .build(); + + private static final ApplicationPackage conservativeApplicationPackage = + new ApplicationPackageBuilder().upgradePolicy("conservative") + .region("us-west-1") + .region("us-east-3") + .build(); + + /** Returns empty prebuilt applications for efficiency */ private ApplicationPackage applicationPackage(String upgradePolicy) { - return new ApplicationPackageBuilder().upgradePolicy(upgradePolicy) - .region("us-west-1") - .region("us-east-3") - .build(); + switch (upgradePolicy) { + case "canary" : return canaryApplicationPackage; + case "default" : return defaultApplicationPackage; + case "conservative" : return conservativeApplicationPackage; + default : throw new IllegalArgumentException("No upgrade policy '" + upgradePolicy + "'"); + } } private DeploymentContext createAndDeploy(String applicationName, String upgradePolicy) { -- cgit v1.2.3 From 95a82805f83443304c8137b13eabb6030cd9e9fc Mon Sep 17 00:00:00 2001 From: Henning Baldersheim Date: Thu, 17 Jun 2021 06:28:46 +0000 Subject: - NULL -> nullptr - Remove virtual on override. - minor style changes. --- .../vespa/searchcore/grouping/groupingcontext.cpp | 10 ++-- .../searchcore/proton/server/fileconfigmanager.cpp | 37 ++++++--------- .../searchcore/proton/server/fileconfigmanager.h | 44 +++++++----------- searchlib/src/vespa/searchlib/aggregation/group.h | 2 +- .../src/vespa/searchlib/common/indexmetainfo.cpp | 54 ++++++++-------------- 5 files changed, 55 insertions(+), 92 deletions(-) diff --git a/searchcore/src/vespa/searchcore/grouping/groupingcontext.cpp b/searchcore/src/vespa/searchcore/grouping/groupingcontext.cpp index 55e9ce16f70..01dd069b03c 100644 --- a/searchcore/src/vespa/searchcore/grouping/groupingcontext.cpp +++ b/searchcore/src/vespa/searchcore/grouping/groupingcontext.cpp @@ -4,17 +4,15 @@ #include #include -namespace search { +namespace search::grouping { using aggregation::CountFS4Hits; using aggregation::FS4HitSetDistributionKey; -namespace grouping { - void GroupingContext::deserialize(const char *groupSpec, uint32_t groupSpecLen) { - if ((groupSpec != NULL) && (groupSpecLen > 4)) { + if ((groupSpec != nullptr) && (groupSpecLen > 4)) { vespalib::nbostream is(groupSpec, groupSpecLen); vespalib::NBOSerializer nis(is); uint32_t numGroupings = 0; @@ -102,6 +100,4 @@ GroupingContext::needRanking() const return true; } - -} // namespace search::grouping -} // namespace search +} diff --git a/searchcore/src/vespa/searchcore/proton/server/fileconfigmanager.cpp b/searchcore/src/vespa/searchcore/proton/server/fileconfigmanager.cpp index 4b862b40896..04aea64fbd4 100644 --- a/searchcore/src/vespa/searchcore/proton/server/fileconfigmanager.cpp +++ b/searchcore/src/vespa/searchcore/proton/server/fileconfigmanager.cpp @@ -4,8 +4,6 @@ #include "bootstrapconfig.h" #include #include -#include -#include #include #include #include @@ -42,7 +40,8 @@ using vespa::config::search::summary::JuniperrcConfig; using vespa::config::content::core::BucketspacesConfig; using vespalib::nbostream; -typedef IndexMetaInfo::SnapshotList SnapshotList; +using SnapshotList = IndexMetaInfo::SnapshotList; +using Snapshot = IndexMetaInfo::Snapshot; using namespace std::chrono_literals; namespace proton { @@ -74,9 +73,7 @@ fsyncFile(const vespalib::string &fileName) template void -saveHelper(const vespalib::string &snapDir, - const vespalib::string &name, - const Config &config) +saveHelper(const vespalib::string &snapDir, const vespalib::string &name, const Config &config) { vespalib::string fileName(snapDir + "/" + name + ".cfg"); config::FileConfigWriter writer(fileName); @@ -105,8 +102,7 @@ public: ConfigFile(); ~ConfigFile(); - ConfigFile(const vespalib::string &name, - const vespalib::string &fullName); + ConfigFile(const vespalib::string &name, const vespalib::string &fullName); nbostream &serialize(nbostream &stream) const; nbostream &deserialize(nbostream &stream); @@ -122,8 +118,7 @@ ConfigFile::ConfigFile() ConfigFile::~ConfigFile() = default; -ConfigFile::ConfigFile(const vespalib::string &name, - const vespalib::string &fullName) +ConfigFile::ConfigFile(const vespalib::string &name, const vespalib::string &fullName) : _name(name), _modTime(0), _content() @@ -142,7 +137,7 @@ ConfigFile::ConfigFile(const vespalib::string &name, nbostream & ConfigFile::serialize(nbostream &stream) const { - assert(strchr(_name.c_str(), '/') == NULL); + assert(strchr(_name.c_str(), '/') == nullptr); stream << _name; stream << static_cast(_modTime);; uint32_t sz = _content.size(); @@ -155,7 +150,7 @@ nbostream & ConfigFile::deserialize(nbostream &stream) { stream >> _name; - assert(strchr(_name.c_str(), '/') == NULL); + assert(strchr(_name.c_str(), '/') == nullptr); int64_t modTime; stream >> modTime; _modTime = modTime; @@ -255,8 +250,7 @@ FileConfigManager::getOldestSerialNum() const } void -FileConfigManager::saveConfig(const DocumentDBConfig &snapshot, - SerialNum serialNum) +FileConfigManager::saveConfig(const DocumentDBConfig &snapshot, SerialNum serialNum) { if (getBestSerialNum() >= serialNum) { LOG(warning, "Config for serial >= %" PRIu64 " already saved", @@ -318,8 +312,7 @@ void addEmptyFile(vespalib::string snapDir, vespalib::string fileName) } void -FileConfigManager::loadConfig(const DocumentDBConfig ¤tSnapshot, - search::SerialNum serialNum, +FileConfigManager::loadConfig(const DocumentDBConfig ¤tSnapshot, search::SerialNum serialNum, DocumentDBConfig::SP &loadedSnapshot) { vespalib::string snapDirBaseName(makeSnapDirBaseName(serialNum)); @@ -333,13 +326,14 @@ FileConfigManager::loadConfig(const DocumentDBConfig ¤tSnapshot, DocumentDBConfigHelper dbc(spec, _docTypeName); - typedef DocumenttypesConfig DTC; - typedef DocumentDBConfig::DocumenttypesConfigSP DTCSP; - DTCSP docTypesCfg(config::ConfigGetter::getConfig("", spec).release()); + using DTC = DocumenttypesConfig; + using DTCSP = DocumentDBConfig::DocumenttypesConfigSP; + DTCSP docTypesCfg = config::ConfigGetter::getConfig("", spec); std::shared_ptr repo; if (currentSnapshot.getDocumenttypesConfigSP() && currentSnapshot.getDocumentTypeRepoSP() && - currentSnapshot.getDocumenttypesConfig() == *docTypesCfg) { + (currentSnapshot.getDocumenttypesConfig() == *docTypesCfg)) + { docTypesCfg = currentSnapshot.getDocumenttypesConfigSP(); repo = currentSnapshot.getDocumentTypeRepoSP(); } else { @@ -462,8 +456,7 @@ FileConfigManager::serializeConfig(SerialNum serialNum, nbostream &stream) uint32_t numConfigs = configs.size(); stream << numConfigs; for (const auto &config : configs) { - ConfigFile file(config, - snapDir + "/" + config); + ConfigFile file(config, snapDir + "/" + config); stream << file; } } diff --git a/searchcore/src/vespa/searchcore/proton/server/fileconfigmanager.h b/searchcore/src/vespa/searchcore/proton/server/fileconfigmanager.h index 1c477ffd3c8..d58d7920c67 100644 --- a/searchcore/src/vespa/searchcore/proton/server/fileconfigmanager.h +++ b/searchcore/src/vespa/searchcore/proton/server/fileconfigmanager.h @@ -10,17 +10,12 @@ namespace proton { class FileConfigManager : public ConfigStore { -public: - typedef std::unique_ptr UP; - typedef std::shared_ptr SP; - typedef search::IndexMetaInfo::Snapshot Snapshot; - private: - vespalib::string _baseDir; - vespalib::string _configId; - vespalib::string _docTypeName; + vespalib::string _baseDir; + vespalib::string _configId; + vespalib::string _docTypeName; search::IndexMetaInfo _info; - ProtonConfigSP _protonConfig; + ProtonConfigSP _protonConfig; public: /** @@ -33,14 +28,12 @@ public: const vespalib::string &configId, const vespalib::string &docTypeName); - virtual - ~FileConfigManager(); + ~FileConfigManager() override; - virtual SerialNum getBestSerialNum() const override; - virtual SerialNum getOldestSerialNum() const override; + SerialNum getBestSerialNum() const override; + SerialNum getOldestSerialNum() const override; - virtual void saveConfig(const DocumentDBConfig &snapshot, - SerialNum serialNum) override; + void saveConfig(const DocumentDBConfig &snapshot, SerialNum serialNum) override; /** * Load a config snapshot from disk corresponding to the given @@ -53,23 +46,21 @@ public: * @param loadedSnapshot the shared pointer in which to store the * resulting config snapshot. */ - virtual void loadConfig(const DocumentDBConfig ¤tSnapshot, - SerialNum serialNum, - DocumentDBConfig::SP &loadedSnapshot) override; + void loadConfig(const DocumentDBConfig ¤tSnapshot, SerialNum serialNum, + DocumentDBConfig::SP &loadedSnapshot) override; - virtual void removeInvalid() override; - virtual void prune(SerialNum serialNum) override; - virtual bool hasValidSerial(SerialNum serialNum) const override; + void removeInvalid() override; + void prune(SerialNum serialNum) override; + bool hasValidSerial(SerialNum serialNum) const override; - virtual SerialNum getPrevValidSerial(SerialNum serialNum) const override; + SerialNum getPrevValidSerial(SerialNum serialNum) const override; /** * Serialize config files. * * Used for serializing config into transaction log. */ - virtual void - serializeConfig(SerialNum serialNum, vespalib::nbostream &stream) override; + void serializeConfig(SerialNum serialNum, vespalib::nbostream &stream) override; /** @@ -80,10 +71,9 @@ public: * takes precedence over the serialized config files in the * transaction log. */ - virtual void - deserializeConfig(SerialNum serialNum, vespalib::nbostream &stream) override; + void deserializeConfig(SerialNum serialNum, vespalib::nbostream &stream) override; - virtual void setProtonConfig(const ProtonConfigSP &protonConfig) override; + void setProtonConfig(const ProtonConfigSP &protonConfig) override; }; } // namespace proton diff --git a/searchlib/src/vespa/searchlib/aggregation/group.h b/searchlib/src/vespa/searchlib/aggregation/group.h index 5b425de24e6..681cda43afa 100644 --- a/searchlib/src/vespa/searchlib/aggregation/group.h +++ b/searchlib/src/vespa/searchlib/aggregation/group.h @@ -232,7 +232,7 @@ public: /** * Recursively checks if any itself or any children needs a full resort. - * Then all hits must be processed and should be doen before any hit sorting. + * Then all hits must be processed and should be done before any hit sorting. */ bool needResort() const { return _aggr.needResort(); } diff --git a/searchlib/src/vespa/searchlib/common/indexmetainfo.cpp b/searchlib/src/vespa/searchlib/common/indexmetainfo.cpp index 837c38eb340..25bc754a86f 100644 --- a/searchlib/src/vespa/searchlib/common/indexmetainfo.cpp +++ b/searchlib/src/vespa/searchlib/common/indexmetainfo.cpp @@ -5,7 +5,6 @@ #include #include #include -#include #include LOG_SETUP(".indexmetainfo"); @@ -14,13 +13,13 @@ namespace { class Parser { private: - vespalib::string _name; + vespalib::string _name; vespalib::FilePointer _file; uint32_t _line; char _buf[2048]; bool _error; - vespalib::string _lastKey; - vespalib::string _lastValue; + vespalib::string _lastKey; + vespalib::string _lastValue; uint32_t _lastIdx; bool _matched; @@ -44,8 +43,7 @@ public: return false; } bool illegalLine() { - LOG(warning, "%s:%d: illegal line: %s", - _name.c_str(), _line, _buf); + LOG(warning, "%s:%d: illegal line: %s", _name.c_str(), _line, _buf); _error = true; return false; } @@ -57,8 +55,7 @@ public: } bool illegalValue() { LOG(warning, "%s:%d: illegal value for '%s': %s", - _name.c_str(), _line, _lastKey.c_str(), - _lastValue.c_str()); + _name.c_str(), _line, _lastKey.c_str(), _lastValue.c_str()); _error = true; return false; } @@ -79,7 +76,7 @@ public: if (!_file.valid()) { return openFailed(); } - if (fgets(_buf, sizeof(_buf), _file) == NULL) { + if (fgets(_buf, sizeof(_buf), _file) == nullptr) { return false; // EOF } ++_line; @@ -88,7 +85,7 @@ public: _buf[--len] = '\0'; } char *split = strchr(_buf, '='); - if (split == NULL || (split - _buf) == 0) { + if (split == nullptr || (split - _buf) == 0) { return illegalLine(); } _lastKey = vespalib::string(_buf, split - _buf); @@ -119,9 +116,9 @@ public: void parseInt64(const vespalib::string &k, uint64_t &v) { if (!_matched && !_error && _lastKey == k) { _matched = true; - char *end = NULL; + char *end = nullptr; uint64_t val = strtoull(_lastValue.c_str(), &end, 10); - if (end == NULL || *end != '\0' || + if (end == nullptr || *end != '\0' || val == static_cast(-1)) { illegalValue(); return; @@ -141,10 +138,10 @@ public: if (dot2 == vespalib::string::npos) { return illegalArrayKey(); } - char *end = NULL; + char *end = nullptr; const char *pt = _lastKey.c_str() + name.length() + 1; uint32_t val = strtoul(pt, &end, 10); - if (end == NULL || end == pt || *end != '.' + if (end == nullptr || end == pt || *end != '.' || val > size || size > val + 1) { return illegalArrayKey(); @@ -200,7 +197,7 @@ IndexMetaInfo::IndexMetaInfo(const vespalib::string &path) { } -IndexMetaInfo::~IndexMetaInfo() {} +IndexMetaInfo::~IndexMetaInfo() = default; IndexMetaInfo::Snapshot IndexMetaInfo::getBestSnapshot() const @@ -209,11 +206,7 @@ IndexMetaInfo::getBestSnapshot() const while (idx >= 0 && !_snapshots[idx].valid) { --idx; } - if (idx >= 0) { - return _snapshots[idx]; - } else { - return Snapshot(); - } + return (idx >= 0) ? _snapshots[idx] : Snapshot(); } @@ -233,7 +226,7 @@ bool IndexMetaInfo::addSnapshot(const Snapshot &snap) { if (snap.dirName.empty() - || findSnapshot(snap.syncToken) != _snapshots.end()) + || (findSnapshot(snap.syncToken) != _snapshots.end())) { return false; } @@ -324,32 +317,23 @@ IndexMetaInfo::save(const vespalib::string &baseName) fprintf(f, "snapshot.%d.dirName=%s\n", i, snap.dirName.c_str()); } if (ferror(f) != 0) { - LOG(error, - "Could not write to file %s", - newName.c_str()); + LOG(error, "Could not write to file %s", newName.c_str()); return false; } if (fflush(f) != 0) { - LOG(error, - "Could not flush file %s", - newName.c_str()); + LOG(error, "Could not flush file %s", newName.c_str()); return false; } if (fsync(fileno(f)) != 0) { - LOG(error, - "Could not fsync file %s", - newName.c_str()); + LOG(error, "Could not fsync file %s", newName.c_str()); return false; } if (fclose(f.release()) != 0) { - LOG(error, - "Could not close file %s", - newName.c_str()); + LOG(error, "Could not close file %s", newName.c_str()); return false; } if (rename(newName.c_str(), fileName.c_str()) != 0) { - LOG(warning, "could not rename: %s->%s", - newName.c_str(), fileName.c_str()); + LOG(warning, "could not rename: %s->%s", newName.c_str(), fileName.c_str()); return false; } vespalib::File::sync(vespalib::dirname(fileName)); -- cgit v1.2.3 From ea140a25c5fd16052c1ab5d69cbe0a68a3d9c71c Mon Sep 17 00:00:00 2001 From: Harald Musum Date: Thu, 17 Jun 2021 08:39:30 +0200 Subject: Cleanup, no functional changes, take 2 Trying again, with fewer changes --- .../yahoo/vespa/config/benchmark/LoadTester.java | 78 +++++++++++----------- 1 file changed, 40 insertions(+), 38 deletions(-) diff --git a/config/src/main/java/com/yahoo/vespa/config/benchmark/LoadTester.java b/config/src/main/java/com/yahoo/vespa/config/benchmark/LoadTester.java index 3a8d80e5ffe..43caf2d52fe 100644 --- a/config/src/main/java/com/yahoo/vespa/config/benchmark/LoadTester.java +++ b/config/src/main/java/com/yahoo/vespa/config/benchmark/LoadTester.java @@ -1,4 +1,4 @@ -// Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.config.benchmark; import com.yahoo.collections.Tuple2; @@ -33,6 +33,8 @@ import java.util.Map; import java.util.Optional; import java.util.concurrent.ThreadLocalRandom; +import static com.yahoo.vespa.config.ConfigKey.createFull; + /** * A config client for generating load against a config server or config proxy. *

@@ -69,8 +71,7 @@ public class LoadTester { String configsList = parser.getBinarySwitches().get("-l"); String defPath = parser.getBinarySwitches().get("-dd"); debug = parser.getUnarySwitches().contains("-d"); - LoadTester loadTester = new LoadTester(); - loadTester.runLoad(host, port, iterations, threads, configsList, defPath); + new LoadTester().runLoad(host, port, iterations, threads, configsList, defPath); } private void runLoad(String host, int port, int iterations, int threads, @@ -97,14 +98,17 @@ public class LoadTester { private Map> readDefs(String defPath) throws IOException { Map> ret = new HashMap<>(); if (defPath == null) return ret; + File defDir = new File(defPath); if (!defDir.isDirectory()) { - System.out.println("# Given def file dir is not a directory: " + defDir.getPath() + " , will not send def contents in requests."); + System.out.println("# Given def file dir is not a directory: " + + defDir.getPath() + " , will not send def contents in requests."); return ret; } - final File[] files = defDir.listFiles(); + File[] files = defDir.listFiles(); if (files == null) { - System.out.println("# Given def file dir has no files: " + defDir.getPath() + " , will not send def contents in requests."); + System.out.println("# Given def file dir has no files: " + + defDir.getPath() + " , will not send def contents in requests."); return ret; } for (File f : files) { @@ -131,7 +135,7 @@ public class LoadTester { sb.append((metrics.failedRequests)); sb.append("\n"); sb.append('#').append(TransportMetrics.getInstance().snapshot().toString()).append('\n'); - System.out.println(sb.toString()); + System.out.println(sb); } private List> readConfigs(String configsList) throws IOException { @@ -189,10 +193,10 @@ public class LoadTester { private class LoadThread extends Thread { - int iterations = 0; - String host = ""; - int port = 0; - Metrics metrics = new Metrics(); + private final int iterations; + private final String host; + private final int port; + private final Metrics metrics = new Metrics(); LoadThread(int iterations, String host, int port) { this.iterations = iterations; @@ -204,44 +208,24 @@ public class LoadTester { public void run() { Spec spec = new Spec(host, port); Target target = connect(spec); - ConfigKey reqKey; - JRTClientConfigRequest request; - int totConfs = configs.size(); - boolean reconnCycle = false; // to log reconn message only once, for instance at restart + for (int i = 0; i < iterations; i++) { - reqKey = configs.get(ThreadLocalRandom.current().nextInt(totConfs)); + ConfigKey reqKey = configs.get(ThreadLocalRandom.current().nextInt(configs.size())); ConfigDefinitionKey dKey = new ConfigDefinitionKey(reqKey); Tuple2 defContent = defs.get(dKey); if (defContent == null && defs.size() > 0) { // Only complain if we actually did run with a def dir System.out.println("# No def found for " + dKey + ", not sending in request."); } - request = getRequest(ConfigKey.createFull(reqKey.getName(), reqKey.getConfigId(), reqKey.getNamespace(), defContent.first), defContent.second); + ConfigKey configKey = createFull(reqKey.getName(), reqKey.getConfigId(), reqKey.getNamespace(), defContent.first); + JRTClientConfigRequest request = createRequest(configKey, defContent.second); if (debug) System.out.println("# Requesting: " + reqKey); long start = System.currentTimeMillis(); target.invokeSync(request.getRequest(), 10.0); long end = System.currentTimeMillis(); if (request.isError()) { - if ("Connection lost".equals(request.errorMessage()) || "Connection down".equals(request.errorMessage())) { - try { - Thread.sleep(100); - } catch (InterruptedException e) { - throw new RuntimeException(e); - } - if (!reconnCycle) { - System.out.println("# Connection lost, reconnecting..."); - reconnCycle = true; - } - target.close(); - target = connect(spec); - } else { - System.err.println(request.errorMessage()); - } - metrics.incFailedRequests(); + target = handleError(request, spec, target); } else { - if (reconnCycle) { - reconnCycle = false; - System.out.println("# Connection OK"); - } + System.out.println("# Connection OK"); long duration = end - start; if (debug) { @@ -255,7 +239,7 @@ public class LoadTester { } } - private JRTClientConfigRequest getRequest(ConfigKey reqKey, String[] defContent) { + private JRTClientConfigRequest createRequest(ConfigKey reqKey, String[] defContent) { if (defContent == null) defContent = new String[0]; final long serverTimeout = 1000; return JRTClientConfigRequestV3.createWithParams(reqKey, DefContent.fromList(Arrays.asList(defContent)), @@ -266,6 +250,24 @@ public class LoadTester { private Target connect(Spec spec) { return supervisor.connect(spec); } + + private Target handleError(JRTClientConfigRequest request, Spec spec, Target target) { + if (List.of("Connection lost", "Connection down").contains(request.errorMessage())) { + try { + Thread.sleep(100); + } catch (InterruptedException e) { + throw new RuntimeException(e); + } + System.out.println("# Connection lost, reconnecting..."); + target.close(); + target = connect(spec); + } else { + System.err.println(request.errorMessage()); + } + metrics.incFailedRequests(); + return target; + } + } } -- cgit v1.2.3 From 7f86c88a95391631ab6023569f007ab68d03e30d Mon Sep 17 00:00:00 2001 From: Ola Aunrønning Date: Thu, 17 Jun 2021 11:50:07 +0200 Subject: Check feed metric value separately --- .../config/server/metrics/ClusterDeploymentMetricsRetriever.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/metrics/ClusterDeploymentMetricsRetriever.java b/configserver/src/main/java/com/yahoo/vespa/config/server/metrics/ClusterDeploymentMetricsRetriever.java index e1135063f97..d406cafc3b8 100644 --- a/configserver/src/main/java/com/yahoo/vespa/config/server/metrics/ClusterDeploymentMetricsRetriever.java +++ b/configserver/src/main/java/com/yahoo/vespa/config/server/metrics/ClusterDeploymentMetricsRetriever.java @@ -127,8 +127,10 @@ public class ClusterDeploymentMetricsRetriever { case VESPA_CONTAINER: optionalDouble(values.field("query_latency.sum")).ifPresent(qlSum -> aggregator.get() - .addContainerLatency(qlSum, values.field("query_latency.count").asDouble()) - .addFeedLatency(values.field("feed.latency.sum").asDouble(), values.field("feed.latency.count").asDouble())); + .addContainerLatency(qlSum, values.field("query_latency.count").asDouble())); + optionalDouble(values.field("feed.latency.sum")).ifPresent(flSum -> + aggregator.get() + .addFeedLatency(flSum, values.field("feed.latency.count").asDouble())); break; case VESPA_QRSERVER: optionalDouble(values.field("query_latency.sum")).ifPresent(qlSum -> -- cgit v1.2.3 From 97ab9a801bda0002bfaf331161cd57514e6e9718 Mon Sep 17 00:00:00 2001 From: Tor Egge Date: Thu, 17 Jun 2021 13:10:43 +0200 Subject: Use correct serial number for resumed save config. --- .../tests/proton/documentdb/documentdb_test.cpp | 124 ++++++++++++++++++++- .../vespa/searchcore/proton/server/documentdb.cpp | 14 ++- 2 files changed, 126 insertions(+), 12 deletions(-) diff --git a/searchcore/src/tests/proton/documentdb/documentdb_test.cpp b/searchcore/src/tests/proton/documentdb/documentdb_test.cpp index 463a7b164e1..3013e8f38d1 100644 --- a/searchcore/src/tests/proton/documentdb/documentdb_test.cpp +++ b/searchcore/src/tests/proton/documentdb/documentdb_test.cpp @@ -19,6 +19,8 @@ #include #include #include +#include +#include #include #include #include @@ -28,7 +30,10 @@ #include #include #include +#include +#include #include +#include using namespace cloud::config::filedistribution; using namespace proton; @@ -39,6 +44,7 @@ using document::DocumentType; using document::DocumentTypeRepo; using document::DocumenttypesConfig; using document::test::makeBucketSpace; +using search::SerialNum; using search::TuneFileDocumentDB; using search::index::DummyFileHeaderContext; using search::index::Schema; @@ -51,6 +57,24 @@ using vespalib::Slime; namespace { +void +cleanup_dirs(bool file_config) +{ + vespalib::rmdir("typea", true); + vespalib::rmdir("tmp", true); + if (file_config) { + vespalib::rmdir("config", true); + } +} + +vespalib::string +config_subdir(SerialNum serialNum) +{ + vespalib::asciistream os; + os << "config/config-" << serialNum; + return os.str(); +} + struct MyDBOwner : public DummyDBOwner { std::shared_ptr _registry; @@ -67,7 +91,30 @@ MyDBOwner::MyDBOwner() {} MyDBOwner::~MyDBOwner() = default; -struct Fixture { +struct FixtureBase { + bool _cleanup; + bool _file_config; + FixtureBase(bool file_config); + ~FixtureBase(); + void disable_cleanup() { _cleanup = false; } +}; + +FixtureBase::FixtureBase(bool file_config) + : _cleanup(true), + _file_config(file_config) +{ + vespalib::mkdir("typea"); +} + + +FixtureBase::~FixtureBase() +{ + if (_cleanup) { + cleanup_dirs(_file_config); + } +} + +struct Fixture : public FixtureBase { DummyWireService _dummy; MyDBOwner _myDBOwner; vespalib::ThreadStackExecutor _summaryExecutor; @@ -79,12 +126,20 @@ struct Fixture { matching::QueryLimiter _queryLimiter; vespalib::Clock _clock; + std::unique_ptr make_config_store(); Fixture(); + Fixture(bool file_config); ~Fixture(); }; Fixture::Fixture() - : _dummy(), + : Fixture(false) +{ +} + +Fixture::Fixture(bool file_config) + : FixtureBase(file_config), + _dummy(), _myDBOwner(), _summaryExecutor(8, 128_Ki), _hwInfo(), @@ -111,13 +166,25 @@ Fixture::Fixture() _db = DocumentDB::create(".", mgr.getConfig(), "tcp/localhost:9014", _queryLimiter, _clock, DocTypeName("typea"), makeBucketSpace(), *b->getProtonConfigSP(), _myDBOwner, _summaryExecutor, _summaryExecutor, _bucketExecutor, _tls, _dummy, - _fileHeaderContext, std::make_unique(), + _fileHeaderContext, make_config_store(), std::make_shared(16, 128_Ki), _hwInfo); _db->start(); _db->waitForOnlineState(); } -Fixture::~Fixture() = default; +Fixture::~Fixture() +{ +} + +std::unique_ptr +Fixture::make_config_store() +{ + if (_file_config) { + return std::make_unique("config", "", "typea"); + } else { + return std::make_unique(); + } +} const IFlushTarget * extractRealFlushTarget(const IFlushTarget *target) @@ -249,11 +316,56 @@ TEST_F("require that document db registers reference", Fixture) EXPECT_EQUAL(search::attribute::BasicType::INT32, attrReadGuard->attribute()->getBasicType()); } +TEST("require that normal restart works") +{ + { + Fixture f(true); + f.disable_cleanup(); + } + { + Fixture f(true); + } +} + +TEST("require that resume after interrupted save config works") +{ + SerialNum serialNum = 0; + { + Fixture f(true); + f.disable_cleanup(); + serialNum = f._db->getFeedHandler().getSerialNum(); + } + { + /* + * Simulate interrupted save config by copying best config to + * serial number after end of transaction log + */ + std::cout << "Replay end serial num is " << serialNum << std::endl; + search::IndexMetaInfo info("config"); + ASSERT_TRUE(info.load()); + auto best_config_snapshot = info.getBestSnapshot(); + ASSERT_TRUE(best_config_snapshot.valid); + std::cout << "Best config serial is " << best_config_snapshot.syncToken << std::endl; + auto old_config_subdir = config_subdir(best_config_snapshot.syncToken); + auto new_config_subdir = config_subdir(serialNum + 1); + vespalib::mkdir(new_config_subdir); + auto config_files = vespalib::listDirectory(old_config_subdir); + for (auto &config_file : config_files) { + vespalib::copy(old_config_subdir + "/" + config_file, new_config_subdir + "/" + config_file, false, false); + } + info.addSnapshot({true, serialNum + 1, new_config_subdir.substr(new_config_subdir.rfind('/') + 1)}); + info.save(); + } + { + Fixture f(true); + } +} + } // namespace TEST_MAIN() { + cleanup_dirs(true); DummyFileHeaderContext::setCreator("documentdb_test"); - FastOS_File::MakeDirectory("typea"); TEST_RUN_ALL(); - FastOS_FileInterface::EmptyAndRemoveDirectory("typea"); + cleanup_dirs(true); } diff --git a/searchcore/src/vespa/searchcore/proton/server/documentdb.cpp b/searchcore/src/vespa/searchcore/proton/server/documentdb.cpp index aa633536419..e53e817af8d 100644 --- a/searchcore/src/vespa/searchcore/proton/server/documentdb.cpp +++ b/searchcore/src/vespa/searchcore/proton/server/documentdb.cpp @@ -632,8 +632,9 @@ DocumentDB::saveInitialConfig(const DocumentDBConfig &configSnapshot) // Only called from ctor lock_guard guard(_configMutex); - if (_config_store->getBestSerialNum() != 0) + if (_config_store->getBestSerialNum() != 0) { return; // Initial config already present + } SerialNum confSerial = _feedHandler->inc_replay_end_serial_num(); _feedHandler->setSerialNum(confSerial); @@ -658,16 +659,17 @@ void DocumentDB::resumeSaveConfig() { SerialNum bestSerial = _config_store->getBestSerialNum(); - if (bestSerial == 0) - return; - if (bestSerial != _feedHandler->get_replay_end_serial_num() + 1) + assert(bestSerial != 0); + if (bestSerial != _feedHandler->get_replay_end_serial_num() + 1) { return; + } + LOG(warning, "DocumentDB(%s): resumeSaveConfig() resuming save config for serial %" PRIu64, + _docTypeName.toString().c_str(), bestSerial); // proton was interrupted when saving later config. SerialNum confSerial = _feedHandler->inc_replay_end_serial_num(); - _feedHandler->setSerialNum(confSerial); + assert(confSerial == bestSerial); // resume operation, i.e. save config entry in transaction log NewConfigOperation op(confSerial, *_config_store); - op.setSerialNum(_feedHandler->inc_replay_end_serial_num()); (void) _feedHandler->storeOperationSync(op); sync(op.getSerialNum()); } -- cgit v1.2.3 From 0fd074975bd6bf45bfe886331c9c5af17af6e5bf Mon Sep 17 00:00:00 2001 From: Jon Bratseth Date: Thu, 17 Jun 2021 13:26:30 +0200 Subject: Internal structs take precedence over external documents --- .../com/yahoo/documentmodel/NewDocumentType.java | 2 +- .../searchdefinition/DocumentModelBuilder.java | 27 +++++------ .../java/com/yahoo/searchdefinition/Search.java | 2 +- .../yahoo/searchdefinition/derived/VsmFields.java | 1 + .../src/test/derived/namecollision/collision.sd | 8 ++++ .../test/derived/namecollision/collisionstruct.sd | 15 ++++++ .../test/derived/namecollision/documentmanager.cfg | 55 ++++++++++++++++++++++ .../derived/NameCollisionTestCase.java | 20 ++++++++ .../java/com/yahoo/document/StructDataType.java | 2 +- .../document/TemporaryStructuredDataType.java | 3 +- 10 files changed, 117 insertions(+), 18 deletions(-) create mode 100644 config-model/src/test/derived/namecollision/collision.sd create mode 100644 config-model/src/test/derived/namecollision/collisionstruct.sd create mode 100644 config-model/src/test/derived/namecollision/documentmanager.cfg create mode 100644 config-model/src/test/java/com/yahoo/searchdefinition/derived/NameCollisionTestCase.java diff --git a/config-model/src/main/java/com/yahoo/documentmodel/NewDocumentType.java b/config-model/src/main/java/com/yahoo/documentmodel/NewDocumentType.java index 38d831a0b28..9fda53d52ce 100644 --- a/config-model/src/main/java/com/yahoo/documentmodel/NewDocumentType.java +++ b/config-model/src/main/java/com/yahoo/documentmodel/NewDocumentType.java @@ -243,7 +243,7 @@ public final class NewDocumentType extends StructuredDataType implements DataTyp @Override public Document createFieldValue() { - return new Document(null, (DocumentId)null); + return new Document(null, (DocumentId)null); // XXX: Always causes NPE } @Override diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/DocumentModelBuilder.java b/config-model/src/main/java/com/yahoo/searchdefinition/DocumentModelBuilder.java index fed35382b21..9b752c4179f 100644 --- a/config-model/src/main/java/com/yahoo/searchdefinition/DocumentModelBuilder.java +++ b/config-model/src/main/java/com/yahoo/searchdefinition/DocumentModelBuilder.java @@ -209,17 +209,13 @@ public class DocumentModelBuilder { private static DataType resolveTemporariesRecurse(DataType type, DataTypeCollection repo, Collection docs) { if (type instanceof TemporaryStructuredDataType) { - NewDocumentType docType = getDocumentType(docs, type.getId()); - if (docType != null) { - type = docType; - return type; - } - DataType real = repo.getDataType(type.getId()); - if (real == null) { - throw new NullPointerException("Can not find type '" + type.toString() + "', impossible."); - } - type = real; - } else if (type instanceof StructDataType) { + DataType struct = repo.getDataType(type.getId()); + if (struct != null) + type = struct; + else + type = getDocumentType(docs, type.getId()); + } + else if (type instanceof StructDataType) { StructDataType dt = (StructDataType) type; for (com.yahoo.document.Field field : dt.getFields()) { if (field.getDataType() != type) { @@ -227,14 +223,17 @@ public class DocumentModelBuilder { field.setDataType(resolveTemporariesRecurse(field.getDataType(), repo, docs)); } } - } else if (type instanceof MapDataType) { + } + else if (type instanceof MapDataType) { MapDataType t = (MapDataType) type; t.setKeyType(resolveTemporariesRecurse(t.getKeyType(), repo, docs)); t.setValueType(resolveTemporariesRecurse(t.getValueType(), repo, docs)); - } else if (type instanceof CollectionDataType) { + } + else if (type instanceof CollectionDataType) { CollectionDataType t = (CollectionDataType) type; t.setNestedType(resolveTemporariesRecurse(t.getNestedType(), repo, docs)); - } else if (type instanceof ReferenceDataType) { + } + else if (type instanceof ReferenceDataType) { ReferenceDataType t = (ReferenceDataType) type; if (t.getTargetType() instanceof TemporaryStructuredDataType) { DataType targetType = resolveTemporariesRecurse(t.getTargetType(), repo, docs); diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/Search.java b/config-model/src/main/java/com/yahoo/searchdefinition/Search.java index 4b7b1625a01..9ce1b8bb330 100644 --- a/config-model/src/main/java/com/yahoo/searchdefinition/Search.java +++ b/config-model/src/main/java/com/yahoo/searchdefinition/Search.java @@ -98,7 +98,7 @@ public class Search implements ImmutableSearch { private final DeployLogger deployLogger; private final ModelContext.Properties properties; - /** Testin only */ + /** Testing only */ public Search(String name) { this(name, null, new BaseDeployLogger(), new TestProperties()); } diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/derived/VsmFields.java b/config-model/src/main/java/com/yahoo/searchdefinition/derived/VsmFields.java index 3f1474704fe..64b94b65006 100644 --- a/config-model/src/main/java/com/yahoo/searchdefinition/derived/VsmFields.java +++ b/config-model/src/main/java/com/yahoo/searchdefinition/derived/VsmFields.java @@ -178,6 +178,7 @@ public class VsmFields extends Derived implements VsmfieldsConfig.Producer { /** Converts to the right index type from a field datatype */ private static Type convertType(DataType fieldType) { + System.out.println("Converting field type " + fieldType + " which is a " + fieldType.getClass()); FieldValue fval = fieldType.createFieldValue(); if (fieldType.equals(DataType.FLOAT16)) { return Type.FLOAT16; diff --git a/config-model/src/test/derived/namecollision/collision.sd b/config-model/src/test/derived/namecollision/collision.sd new file mode 100644 index 00000000000..43dd4830204 --- /dev/null +++ b/config-model/src/test/derived/namecollision/collision.sd @@ -0,0 +1,8 @@ +# Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +search collision { + + document collision { + + } + +} diff --git a/config-model/src/test/derived/namecollision/collisionstruct.sd b/config-model/src/test/derived/namecollision/collisionstruct.sd new file mode 100644 index 00000000000..c98efb0b582 --- /dev/null +++ b/config-model/src/test/derived/namecollision/collisionstruct.sd @@ -0,0 +1,15 @@ +# Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +search collisionstruct { + + document collisionstruct { + + struct collision { + } + + field structarray type array { + indexing: summary + } + + } + +} diff --git a/config-model/src/test/derived/namecollision/documentmanager.cfg b/config-model/src/test/derived/namecollision/documentmanager.cfg new file mode 100644 index 00000000000..8d0d89dde35 --- /dev/null +++ b/config-model/src/test/derived/namecollision/documentmanager.cfg @@ -0,0 +1,55 @@ +enablecompression false +datatype[].id 1381038251 +datatype[].structtype[].name "position" +datatype[].structtype[].version 0 +datatype[].structtype[].compresstype NONE +datatype[].structtype[].compresslevel 0 +datatype[].structtype[].compressthreshold 95 +datatype[].structtype[].compressminsize 800 +datatype[].structtype[].field[].name "x" +datatype[].structtype[].field[].datatype 0 +datatype[].structtype[].field[].detailedtype "" +datatype[].structtype[].field[].name "y" +datatype[].structtype[].field[].datatype 0 +datatype[].structtype[].field[].detailedtype "" +datatype[].id -379118517 +datatype[].structtype[].name "collision.header" +datatype[].structtype[].version 0 +datatype[].structtype[].compresstype NONE +datatype[].structtype[].compresslevel 0 +datatype[].structtype[].compressthreshold 95 +datatype[].structtype[].compressminsize 800 +datatype[].id 1557022836 +datatype[].documenttype[].name "collision" +datatype[].documenttype[].version 0 +datatype[].documenttype[].inherits[].name "document" +datatype[].documenttype[].inherits[].version 0 +datatype[].documenttype[].headerstruct -379118517 +datatype[].documenttype[].bodystruct 0 +datatype[].id 1557022836 +datatype[].structtype[].name "collision" +datatype[].structtype[].version 0 +datatype[].structtype[].compresstype NONE +datatype[].structtype[].compresslevel 0 +datatype[].structtype[].compressthreshold 95 +datatype[].structtype[].compressminsize 800 +datatype[].id -1730522993 +datatype[].arraytype[].datatype 1557022836 +datatype[].id -1270379114 +datatype[].structtype[].name "collisionstruct.header" +datatype[].structtype[].version 0 +datatype[].structtype[].compresstype NONE +datatype[].structtype[].compresslevel 0 +datatype[].structtype[].compressthreshold 95 +datatype[].structtype[].compressminsize 800 +datatype[].structtype[].field[].name "structarray" +datatype[].structtype[].field[].datatype -1730522993 +datatype[].structtype[].field[].detailedtype "" +datatype[].id -1723079287 +datatype[].documenttype[].name "collisionstruct" +datatype[].documenttype[].version 0 +datatype[].documenttype[].inherits[].name "document" +datatype[].documenttype[].inherits[].version 0 +datatype[].documenttype[].headerstruct -1270379114 +datatype[].documenttype[].bodystruct 0 +datatype[].documenttype[].fieldsets{[]}.fields[] "structarray" diff --git a/config-model/src/test/java/com/yahoo/searchdefinition/derived/NameCollisionTestCase.java b/config-model/src/test/java/com/yahoo/searchdefinition/derived/NameCollisionTestCase.java new file mode 100644 index 00000000000..fda9e6327ce --- /dev/null +++ b/config-model/src/test/java/com/yahoo/searchdefinition/derived/NameCollisionTestCase.java @@ -0,0 +1,20 @@ +// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. + +package com.yahoo.searchdefinition.derived; + +import org.junit.Test; + +/** + * Verifies that a struct in a document type is preferred over another dopcument type + * of the same name. + * + * @author bratseth + */ +public class NameCollisionTestCase extends AbstractExportingTestCase { + + @Test + public void testNameCollision() throws Exception { + assertCorrectDeriving("namecollision", "collisionstruct", new TestableDeployLogger()); + } + +} diff --git a/document/src/main/java/com/yahoo/document/StructDataType.java b/document/src/main/java/com/yahoo/document/StructDataType.java index 73fe580308e..8a153856eff 100644 --- a/document/src/main/java/com/yahoo/document/StructDataType.java +++ b/document/src/main/java/com/yahoo/document/StructDataType.java @@ -22,7 +22,7 @@ public class StructDataType extends BaseStructDataType { super(name); } - public StructDataType(int id,String name) { + public StructDataType(int id, String name) { super(id, name); } diff --git a/document/src/main/java/com/yahoo/document/TemporaryStructuredDataType.java b/document/src/main/java/com/yahoo/document/TemporaryStructuredDataType.java index 0449612da6f..f4139a597d2 100644 --- a/document/src/main/java/com/yahoo/document/TemporaryStructuredDataType.java +++ b/document/src/main/java/com/yahoo/document/TemporaryStructuredDataType.java @@ -2,7 +2,8 @@ package com.yahoo.document; /** - * Internal class, DO NOT USE!! Only public because it must be used from com.yahoo.searchdefinition.parser. + * Internal class, DO NOT USE!! + * Only public because it must be used from com.yahoo.searchdefinition.parser. * * @author Einar M R Rosenvinge */ -- cgit v1.2.3 From f006c1c153d491a9afdf61e07fb5a90ade0eeae0 Mon Sep 17 00:00:00 2001 From: Jon Bratseth Date: Thu, 17 Jun 2021 13:36:01 +0200 Subject: Cleanup, no functional changes --- .../com/yahoo/documentmodel/NewDocumentType.java | 77 ++++++++++++---------- .../yahoo/searchdefinition/derived/VsmFields.java | 1 - .../com/yahoo/document/StructuredDataType.java | 2 - 3 files changed, 42 insertions(+), 38 deletions(-) diff --git a/config-model/src/main/java/com/yahoo/documentmodel/NewDocumentType.java b/config-model/src/main/java/com/yahoo/documentmodel/NewDocumentType.java index 9fda53d52ce..d896a7f7d22 100644 --- a/config-model/src/main/java/com/yahoo/documentmodel/NewDocumentType.java +++ b/config-model/src/main/java/com/yahoo/documentmodel/NewDocumentType.java @@ -32,34 +32,6 @@ import static java.util.Collections.emptySet; */ public final class NewDocumentType extends StructuredDataType implements DataTypeCollection { - public static final class Name { - - private final String name; - private final int id; - - public Name(String name) { - this(name.hashCode(), name); - } - - public Name(int id, String name) { - this.id = id; - this.name = name; - } - - public String toString() { return name; } - - public final String getName() { return name; } - - public final int getId() { return id; } - - public int hashCode() { return name.hashCode(); } - - public boolean equals(Object other) { - if ( ! (other instanceof Name)) return false; - return name.equals(((Name)other).getName()); - } - } - private final Name name; private final DataTypeRepo dataTypes = new DataTypeRepo(); private final Map inherits = new LinkedHashMap<>(); @@ -139,7 +111,7 @@ public final class NewDocumentType extends StructuredDataType implements DataTyp } @Override - public Class getValueClass() { + public Class getValueClass() { return Document.class; } @@ -148,7 +120,8 @@ public final class NewDocumentType extends StructuredDataType implements DataTyp if (!(value instanceof Document)) { return false; } - /** Temporary disabled due to clash with document and covariant return type + /* + Temporary disabled due to clash with document and covariant return type Document doc = (Document) value; if (((NewDocumentType) doc.getDataType()).inherits(this)) { //the value is of this type; or the supertype of the value is of this type, etc.... @@ -162,28 +135,31 @@ public final class NewDocumentType extends StructuredDataType implements DataTyp for (Field f : getFields()) { Field inhF = inherited.getField(f.getName()); if (inhF != null && !inhF.equals(f)) { - throw new IllegalArgumentException("Inherited document '" + inherited.toString() + "' already contains field '" + - inhF.getName() + "'. Can not override with '" + f.getName() + "'."); + throw new IllegalArgumentException("Inherited document '" + inherited + "' already contains field '" + + inhF.getName() + "'. Can not override with '" + f.getName() + "'."); } } for (Field f : inherited.getAllFields()) { for (NewDocumentType side : inherits.values()) { Field sideF = side.getField(f.getName()); if (sideF != null && !sideF.equals(f)) { - throw new IllegalArgumentException("Inherited document '" + side.toString() + "' already contains field '" + - sideF.getName() + "'. Document '" + inherited.toString() + "' also defines field '" + f.getName() + - "'.Multiple inheritance must be disjunctive."); + throw new IllegalArgumentException("Inherited document '" + side + "' already contains field '" + + sideF.getName() + "'. Document '" + inherited + + "' also defines field '" + f.getName() + + "'.Multiple inheritance must be disjunctive."); } } } return true; } + public void inherit(NewDocumentType inherited) { if ( ! inherits.containsKey(inherited.getId())) { verifyInheritance(inherited); inherits.put(inherited.getId(), inherited); } } + public boolean inherits(NewDocumentType superType) { if (getId() == superType.getId()) return true; for (NewDocumentType type : inherits.values()) { @@ -375,4 +351,35 @@ public final class NewDocumentType extends StructuredDataType implements DataTyp return importedFieldNames; } + public static final class Name { + + private final String name; + private final int id; + + public Name(String name) { + this(name.hashCode(), name); + } + + public Name(int id, String name) { + this.id = id; + this.name = name; + } + + public String toString() { return name; } + + public final String getName() { return name; } + + public final int getId() { return id; } + + @Override + public int hashCode() { return name.hashCode(); } + + @Override + public boolean equals(Object other) { + if ( ! (other instanceof Name)) return false; + return name.equals(((Name)other).getName()); + } + + } + } diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/derived/VsmFields.java b/config-model/src/main/java/com/yahoo/searchdefinition/derived/VsmFields.java index 64b94b65006..3f1474704fe 100644 --- a/config-model/src/main/java/com/yahoo/searchdefinition/derived/VsmFields.java +++ b/config-model/src/main/java/com/yahoo/searchdefinition/derived/VsmFields.java @@ -178,7 +178,6 @@ public class VsmFields extends Derived implements VsmfieldsConfig.Producer { /** Converts to the right index type from a field datatype */ private static Type convertType(DataType fieldType) { - System.out.println("Converting field type " + fieldType + " which is a " + fieldType.getClass()); FieldValue fval = fieldType.createFieldValue(); if (fieldType.equals(DataType.FLOAT16)) { return Type.FLOAT16; diff --git a/document/src/main/java/com/yahoo/document/StructuredDataType.java b/document/src/main/java/com/yahoo/document/StructuredDataType.java index e4bb94a5465..8a5f344e79e 100644 --- a/document/src/main/java/com/yahoo/document/StructuredDataType.java +++ b/document/src/main/java/com/yahoo/document/StructuredDataType.java @@ -10,8 +10,6 @@ import java.util.Collection; import java.util.List; /** - * TODO: What is this and why - * * @author Håkon Humberset */ public abstract class StructuredDataType extends DataType { -- cgit v1.2.3 From e0ca1501f65ffe1facf434526183c829eece882f Mon Sep 17 00:00:00 2001 From: Jon Bratseth Date: Thu, 17 Jun 2021 13:40:36 +0200 Subject: Throw explicitly on unsupported operation --- .../src/main/java/com/yahoo/documentmodel/NewDocumentType.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config-model/src/main/java/com/yahoo/documentmodel/NewDocumentType.java b/config-model/src/main/java/com/yahoo/documentmodel/NewDocumentType.java index d896a7f7d22..da338ad3107 100644 --- a/config-model/src/main/java/com/yahoo/documentmodel/NewDocumentType.java +++ b/config-model/src/main/java/com/yahoo/documentmodel/NewDocumentType.java @@ -3,7 +3,6 @@ package com.yahoo.documentmodel; import com.yahoo.document.DataType; import com.yahoo.document.Document; -import com.yahoo.document.DocumentId; import com.yahoo.document.Field; import com.yahoo.document.StructDataType; import com.yahoo.document.StructuredDataType; @@ -219,7 +218,7 @@ public final class NewDocumentType extends StructuredDataType implements DataTyp @Override public Document createFieldValue() { - return new Document(null, (DocumentId)null); // XXX: Always causes NPE + throw new RuntimeException("Cannot create an instance of " + this); } @Override @@ -365,6 +364,7 @@ public final class NewDocumentType extends StructuredDataType implements DataTyp this.name = name; } + @Override public String toString() { return name; } public final String getName() { return name; } -- cgit v1.2.3