diff options
45 files changed, 545 insertions, 325 deletions
diff --git a/config-model/src/main/resources/schema/deployment.rnc b/config-model/src/main/resources/schema/deployment.rnc index 48a5c6bf25a..ede05ad65ef 100644 --- a/config-model/src/main/resources/schema/deployment.rnc +++ b/config-model/src/main/resources/schema/deployment.rnc @@ -174,7 +174,7 @@ Endpoints = element endpoints { Bcp = element bcp { attribute deadline { xsd:string }? & - Group+ + Group* } Group = element group { diff --git a/config-model/src/test/schema-test-files/deployment-with-instances.xml b/config-model/src/test/schema-test-files/deployment-with-instances.xml index 2d319197e79..3b872f4c1cf 100644 --- a/config-model/src/test/schema-test-files/deployment-with-instances.xml +++ b/config-model/src/test/schema-test-files/deployment-with-instances.xml @@ -60,6 +60,7 @@ <endpoints> <endpoint container-id="barz" /> </endpoints> + <bcp deadline="1d"/> </instance> </parallel> diff --git a/searchcore/src/tests/proton/matching/match_phase_limiter/match_phase_limiter_test.cpp b/searchcore/src/tests/proton/matching/match_phase_limiter/match_phase_limiter_test.cpp index 45ac0a8f924..2cb82fabc4c 100644 --- a/searchcore/src/tests/proton/matching/match_phase_limiter/match_phase_limiter_test.cpp +++ b/searchcore/src/tests/proton/matching/match_phase_limiter/match_phase_limiter_test.cpp @@ -71,7 +71,7 @@ struct MockBlueprint : SimpleLeafBlueprint { bool postings_fetched = false; search::queryeval::ExecuteInfo postings_strict = search::queryeval::ExecuteInfo::FALSE; MockBlueprint(const FieldSpec &spec_in, const vespalib::string &term_in) - : SimpleLeafBlueprint(FieldSpecBaseList().add(spec_in)), spec(spec_in), term(term_in) + : SimpleLeafBlueprint(spec_in), spec(spec_in), term(term_in) { setEstimate(HitEstimate(756, false)); } diff --git a/searchcore/src/vespa/searchcore/proton/documentmetastore/lid_allocator.cpp b/searchcore/src/vespa/searchcore/proton/documentmetastore/lid_allocator.cpp index 7b7d590d913..162db5d8760 100644 --- a/searchcore/src/vespa/searchcore/proton/documentmetastore/lid_allocator.cpp +++ b/searchcore/src/vespa/searchcore/proton/documentmetastore/lid_allocator.cpp @@ -213,7 +213,7 @@ private: } public: WhiteListBlueprint(const search::BitVector &activeLids, bool all_lids_active) - : SimpleLeafBlueprint(FieldSpecBaseList()), + : SimpleLeafBlueprint(), _activeLids(activeLids), _all_lids_active(all_lids_active), _lock(), diff --git a/searchcore/src/vespa/searchcore/proton/matching/blueprintbuilder.cpp b/searchcore/src/vespa/searchcore/proton/matching/blueprintbuilder.cpp index 2d3323949d1..eee2b7a7203 100644 --- a/searchcore/src/vespa/searchcore/proton/matching/blueprintbuilder.cpp +++ b/searchcore/src/vespa/searchcore/proton/matching/blueprintbuilder.cpp @@ -2,7 +2,6 @@ #include "blueprintbuilder.h" #include "querynodes.h" -#include "termdatafromnode.h" #include "same_element_builder.h" #include <vespa/searchcorespi/index/indexsearchable.h> #include <vespa/searchlib/query/tree/customtypevisitor.h> @@ -25,7 +24,7 @@ struct Mixer { void addAttribute(Blueprint::UP attr) { if (attributes.get() == 0) { - attributes.reset(new OrBlueprint()); + attributes = std::make_unique<OrBlueprint>(); } attributes->addChild(std::move(attr)); } @@ -92,7 +91,7 @@ private: for (size_t i = 0; i < n.numFields(); ++i) { specs.add(n.field(i).fieldSpec()); } - EquivBlueprint *eq = new EquivBlueprint(specs, n.children_mdl); + EquivBlueprint *eq = new EquivBlueprint(std::move(specs), n.children_mdl); _result.reset(eq); for (size_t i = 0; i < n.getChildren().size(); ++i) { search::query::Node &node = *n.getChildren()[i]; diff --git a/searchlib/src/tests/attribute/searchable/attribute_weighted_set_blueprint_test.cpp b/searchlib/src/tests/attribute/searchable/attribute_weighted_set_blueprint_test.cpp index 26fbed81146..d7a854e0afc 100644 --- a/searchlib/src/tests/attribute/searchable/attribute_weighted_set_blueprint_test.cpp +++ b/searchlib/src/tests/attribute/searchable/attribute_weighted_set_blueprint_test.cpp @@ -101,7 +101,8 @@ struct WS { FakeRequestContext requestContext(&ac); MatchData::UP md = layout.createMatchData(); Node::UP node = createNode(); - FieldSpecList fields = FieldSpecList().add(FieldSpec(field, fieldId, handle)); + FieldSpecList fields; + fields.add(FieldSpec(field, fieldId, handle)); queryeval::Blueprint::UP bp = searchable.createBlueprint(requestContext, fields, *node); bp->fetchPostings(queryeval::ExecuteInfo::create(strict)); SearchIterator::UP sb = bp->createSearch(*md, strict); @@ -113,7 +114,8 @@ struct WS { FakeRequestContext requestContext(&ac); MatchData::UP md = layout.createMatchData(); Node::UP node = createNode(); - FieldSpecList fields = FieldSpecList().add(FieldSpec(field, fieldId, handle)); + FieldSpecList fields; + fields.add(FieldSpec(field, fieldId, handle)); queryeval::Blueprint::UP bp = searchable.createBlueprint(requestContext, fields, *node); bp->fetchPostings(queryeval::ExecuteInfo::create(strict)); SearchIterator::UP sb = bp->createSearch(*md, strict); diff --git a/searchlib/src/tests/queryeval/blueprint/blueprint_test.cpp b/searchlib/src/tests/queryeval/blueprint/blueprint_test.cpp index 6f4ffc31741..b0d3edc984a 100644 --- a/searchlib/src/tests/queryeval/blueprint/blueprint_test.cpp +++ b/searchlib/src/tests/queryeval/blueprint/blueprint_test.cpp @@ -22,19 +22,19 @@ class MyOr : public IntermediateBlueprint { private: public: - virtual HitEstimate combine(const std::vector<HitEstimate> &data) const override { + HitEstimate combine(const std::vector<HitEstimate> &data) const override { return max(data); } - virtual FieldSpecBaseList exposeFields() const override { + FieldSpecBaseList exposeFields() const override { return mixChildrenFields(); } - virtual void sort(Children &children) const override { + void sort(Children &children) const override { std::sort(children.begin(), children.end(), TieredGreaterEstimate()); } - virtual bool inheritStrict(size_t i) const override { + bool inheritStrict(size_t i) const override { (void) i; return true; } @@ -136,10 +136,10 @@ public: //----------------------------------------------------------------------------- struct MyTerm : SimpleLeafBlueprint { - MyTerm(const FieldSpecBaseList &fields, uint32_t hitEstimate) : SimpleLeafBlueprint(fields) { + MyTerm(FieldSpecBase field, uint32_t hitEstimate) : SimpleLeafBlueprint(field) { setEstimate(HitEstimate(hitEstimate, false)); } - virtual SearchIterator::UP createLeafSearch(const search::fef::TermFieldMatchDataArray &, bool) const override { + SearchIterator::UP createLeafSearch(const search::fef::TermFieldMatchDataArray &, bool) const override { return SearchIterator::UP(); } SearchIteratorUP createFilterSearch(bool strict, FilterConstraint constraint) const override { @@ -149,49 +149,29 @@ struct MyTerm : SimpleLeafBlueprint { //----------------------------------------------------------------------------- +Blueprint::UP ap(Blueprint *b) { return Blueprint::UP(b); } +Blueprint::UP ap(Blueprint &b) { return Blueprint::UP(&b); } + } // namespace <unnamed> -class Test : public vespalib::TestApp +class Fixture { private: MatchData::UP _md; - - static Blueprint::UP ap(Blueprint *b) { return Blueprint::UP(b); } - static Blueprint::UP ap(Blueprint &b) { return Blueprint::UP(&b); } - +public: + Fixture() + : _md(MatchData::makeTestInstance(100, 10)) + {} + ~Fixture() = default; SearchIterator::UP create(const Blueprint &blueprint); - bool check_equal(const SearchIterator &a, const SearchIterator &b); bool check_equal(const Blueprint &a, const Blueprint &b); - bool check_not_equal(const SearchIterator &a, const SearchIterator &b); bool check_not_equal(const Blueprint &a, const Blueprint &b); - -public: - Test() - : vespalib::TestApp(), - _md(MatchData::makeTestInstance(100, 10)) - {} - ~Test() {} - Blueprint::UP buildBlueprint1(); - Blueprint::UP buildBlueprint2(); - void testBlueprintBuilding(); - void testHitEstimateCalculation(); - void testHitEstimatePropagation(); - void testMatchDataPropagation(); - void testChildSorting(); - void testChildAndNotCollapsing(); - void testChildAndCollapsing(); - void testChildOrCollapsing(); - void testSearchCreation(); - void testBlueprintMakeNew(); - void requireThatAsStringWorks(); - void requireThatAsSlimeWorks(); - void requireThatVisitMembersWorks(); - void requireThatDocIdLimitInjectionWorks(); - int Main() override; + static bool check_equal(const SearchIterator &a, const SearchIterator &b); + static bool check_not_equal(const SearchIterator &a, const SearchIterator &b); }; SearchIterator::UP -Test::create(const Blueprint &blueprint) +Fixture::create(const Blueprint &blueprint) { const_cast<Blueprint &>(blueprint).fetchPostings(ExecuteInfo::TRUE); SearchIterator::UP search = blueprint.createSearch(*_md, true); @@ -200,39 +180,37 @@ Test::create(const Blueprint &blueprint) } bool -Test::check_equal(const SearchIterator &a, const SearchIterator &b) +Fixture::check_equal(const SearchIterator &a, const SearchIterator &b) { return EXPECT_EQUAL(a.asString(), b.asString()); } bool -Test::check_equal(const Blueprint &a, const Blueprint &b) +Fixture::check_not_equal(const SearchIterator &a, const SearchIterator &b) { - SearchIterator::UP searchA = create(a); - SearchIterator::UP searchB = create(b); - TEST_STATE("check_equal"); - bool ok = check_equal(*searchA, *searchB); - return ok; + return EXPECT_NOT_EQUAL(a.asString(), b.asString()); } bool -Test::check_not_equal(const SearchIterator &a, const SearchIterator &b) +Fixture::check_equal(const Blueprint &a, const Blueprint &b) { - return EXPECT_NOT_EQUAL(a.asString(), b.asString()); + SearchIterator::UP searchA = create(a); + SearchIterator::UP searchB = create(b); + bool ok = check_equal(*searchA, *searchB); + return ok; } bool -Test::check_not_equal(const Blueprint &a, const Blueprint &b) +Fixture::check_not_equal(const Blueprint &a, const Blueprint &b) { SearchIterator::UP searchA = create(a); SearchIterator::UP searchB = create(b); - TEST_STATE("check_not_equal"); bool ok = check_not_equal(*searchA, *searchB); return ok; } Blueprint::UP -Test::buildBlueprint1() +buildBlueprint1() { return ap(MyAnd::create() .add(MyOr::create() @@ -248,7 +226,7 @@ Test::buildBlueprint1() } Blueprint::UP -Test::buildBlueprint2() +buildBlueprint2() { return ap(MyAnd::create() .add(MyOr::create() @@ -263,19 +241,17 @@ Test::buildBlueprint2() ); } -void -Test::testBlueprintBuilding() +TEST_F("testBlueprintBuilding", Fixture) { Blueprint::UP root1 = buildBlueprint1(); Blueprint::UP root2 = buildBlueprint2(); - SearchIterator::UP search1 = create(*root1); - SearchIterator::UP search2 = create(*root2); + SearchIterator::UP search1 = f.create(*root1); + SearchIterator::UP search2 = f.create(*root2); // fprintf(stderr, "%s\n", search1->asString().c_str()); // fprintf(stderr, "%s\n", search2->asString().c_str()); } -void -Test::testHitEstimateCalculation() +TEST("testHitEstimateCalculation") { { Blueprint::UP leaf = ap(MyLeafSpec(37).create()); @@ -333,16 +309,15 @@ Test::testHitEstimateCalculation() } } -void -Test::testHitEstimatePropagation() +TEST("testHitEstimatePropagation") { - MyLeaf *leaf1 = new MyLeaf(FieldSpecBaseList()); + MyLeaf *leaf1 = new MyLeaf(); leaf1->estimate(10); - MyLeaf *leaf2 = new MyLeaf(FieldSpecBaseList()); + MyLeaf *leaf2 = new MyLeaf(); leaf2->estimate(20); - MyLeaf *leaf3 = new MyLeaf(FieldSpecBaseList()); + MyLeaf *leaf3 = new MyLeaf(); leaf3->estimate(30); MyOr *parent = new MyOr(); @@ -375,8 +350,7 @@ Test::testHitEstimatePropagation() EXPECT_EQUAL(25u, root->getState().estimate().estHits); } -void -Test::testMatchDataPropagation() +TEST("testMatchDataPropagation") { { Blueprint::UP leaf = ap(MyLeafSpec(0, true).create()); @@ -430,8 +404,7 @@ Test::testMatchDataPropagation() } } -void -Test::testChildAndNotCollapsing() +TEST_F("testChildAndNotCollapsing", Fixture) { Blueprint::UP unsorted = ap(OtherAndNot::create() .add(OtherAndNot::create() @@ -464,13 +437,12 @@ Test::testChildAndNotCollapsing() .add(MyLeafSpec(3).addField(2, 62).create()) ) ); - TEST_DO(check_not_equal(*sorted, *unsorted)); + TEST_DO(f.check_not_equal(*sorted, *unsorted)); unsorted = Blueprint::optimize(std::move(unsorted)); - TEST_DO(check_equal(*sorted, *unsorted)); + TEST_DO(f.check_equal(*sorted, *unsorted)); } -void -Test::testChildAndCollapsing() +TEST_F("testChildAndCollapsing", Fixture) { Blueprint::UP unsorted = ap(OtherAnd::create() .add(OtherAnd::create() @@ -504,13 +476,12 @@ Test::testChildAndCollapsing() .add(MyLeafSpec(300).addField(1, 31).create()) ); - TEST_DO(check_not_equal(*sorted, *unsorted)); + TEST_DO(f.check_not_equal(*sorted, *unsorted)); unsorted = Blueprint::optimize(std::move(unsorted)); - TEST_DO(check_equal(*sorted, *unsorted)); + TEST_DO(f.check_equal(*sorted, *unsorted)); } -void -Test::testChildOrCollapsing() +TEST_F("testChildOrCollapsing", Fixture) { Blueprint::UP unsorted = ap(OtherOr::create() .add(OtherOr::create() @@ -543,13 +514,12 @@ Test::testChildOrCollapsing() .add(MyLeafSpec(2).addField(2, 52).create()) .add(MyLeafSpec(1).addField(2, 42).create()) ); - TEST_DO(check_not_equal(*sorted, *unsorted)); + TEST_DO(f.check_not_equal(*sorted, *unsorted)); unsorted = Blueprint::optimize(std::move(unsorted)); - TEST_DO(check_equal(*sorted, *unsorted)); + TEST_DO(f.check_equal(*sorted, *unsorted)); } -void -Test::testChildSorting() +TEST_F("testChildSorting", Fixture) { Blueprint::UP unsorted = ap(MyAnd::create() .add(MyOr::create() @@ -587,33 +557,32 @@ Test::testChildSorting() ) ); - TEST_DO(check_not_equal(*sorted, *unsorted)); + TEST_DO(f.check_not_equal(*sorted, *unsorted)); unsorted = Blueprint::optimize(std::move(unsorted)); - TEST_DO(check_equal(*sorted, *unsorted)); + TEST_DO(f.check_equal(*sorted, *unsorted)); } -void -Test::testSearchCreation() +TEST_F("testSearchCreation", Fixture) { { Blueprint::UP l = ap(MyLeafSpec(3) .addField(1, 1) .addField(2, 2) .addField(3, 3).create()); - SearchIterator::UP leafsearch = create(*l); + SearchIterator::UP leafsearch = f.create(*l); MySearch *lw = new MySearch("leaf", true, true); lw->addHandle(1).addHandle(2).addHandle(3); SearchIterator::UP wantleaf(lw); - TEST_DO(check_equal(*wantleaf, *leafsearch)); + TEST_DO(f.check_equal(*wantleaf, *leafsearch)); } { Blueprint::UP a = ap(MyAnd::create() .add(MyLeafSpec(1).addField(1, 1).create()) .add(MyLeafSpec(2).addField(2, 2).create())); - SearchIterator::UP andsearch = create(*a); + SearchIterator::UP andsearch = f.create(*a); MySearch *l1 = new MySearch("leaf", true, true); MySearch *l2 = new MySearch("leaf", true, false); @@ -623,13 +592,13 @@ Test::testSearchCreation() aw->add(l1); aw->add(l2); SearchIterator::UP wanted(aw); - TEST_DO(check_equal(*wanted, *andsearch)); + TEST_DO(f.check_equal(*wanted, *andsearch)); } { Blueprint::UP o = ap(MyOr::create() .add(MyLeafSpec(1).addField(1, 11).create()) .add(MyLeafSpec(2).addField(2, 22).create())); - SearchIterator::UP orsearch = create(*o); + SearchIterator::UP orsearch = f.create(*o); MySearch *l1 = new MySearch("leaf", true, true); MySearch *l2 = new MySearch("leaf", true, true); @@ -639,18 +608,11 @@ Test::testSearchCreation() ow->add(l1); ow->add(l2); SearchIterator::UP wanted(ow); - TEST_DO(check_equal(*wanted, *orsearch)); + TEST_DO(f.check_equal(*wanted, *orsearch)); } } -template<typename T> -Blueprint::UP makeNew(T *orig) -{ - return std::make_unique<T>(*orig); -} - -void -Test::testBlueprintMakeNew() +TEST("testBlueprintMakeNew") { Blueprint::UP orig = ap(MyOr::create() .add(MyLeafSpec(1).addField(1, 11).create()) @@ -765,19 +727,17 @@ struct BlueprintFixture { MyOr _blueprint; BlueprintFixture() : _blueprint() { - _blueprint.add(new MyTerm(FieldSpecBaseList().add(FieldSpecBase(5, 7)), 9)); + _blueprint.add(new MyTerm(FieldSpecBase(5, 7), 9)); } }; -void -Test::requireThatAsStringWorks() +TEST("requireThatAsStringWorks") { BlueprintFixture f; EXPECT_EQUAL(getExpectedBlueprint(), f._blueprint.asString()); } -void -Test::requireThatAsSlimeWorks() +TEST("requireThatAsSlimeWorks") { BlueprintFixture f; vespalib::Slime slime; @@ -788,8 +748,7 @@ Test::requireThatAsSlimeWorks() EXPECT_EQUAL(expectedSlime, slime); } -void -Test::requireThatVisitMembersWorks() +TEST("requireThatVisitMembersWorks") { BlueprintFixture f; vespalib::ObjectDumper dumper; @@ -797,8 +756,7 @@ Test::requireThatVisitMembersWorks() EXPECT_EQUAL(getExpectedBlueprint(), dumper.toString()); } -void -Test::requireThatDocIdLimitInjectionWorks() +TEST("requireThatDocIdLimitInjectionWorks") { BlueprintFixture f; ASSERT_GREATER(f._blueprint.childCnt(), 0u); @@ -808,26 +766,13 @@ Test::requireThatDocIdLimitInjectionWorks() EXPECT_EQUAL(1000u, term.get_docid_limit()); } -int -Test::Main() -{ - TEST_DEBUG("lhs.out", "rhs.out"); - TEST_INIT("blueprint_test"); - testBlueprintBuilding(); - testHitEstimateCalculation(); - testHitEstimatePropagation(); - testMatchDataPropagation(); - testChildSorting(); - testChildAndNotCollapsing(); - testChildAndCollapsing(); - testChildOrCollapsing(); - testSearchCreation(); - testBlueprintMakeNew(); - requireThatAsStringWorks(); - requireThatAsSlimeWorks(); - requireThatVisitMembersWorks(); - requireThatDocIdLimitInjectionWorks(); - TEST_DONE(); +TEST("Control object sizes") { + EXPECT_EQUAL(32u, sizeof(Blueprint::State)); + EXPECT_EQUAL(32u, sizeof(Blueprint)); + EXPECT_EQUAL(64u, sizeof(LeafBlueprint)); } -TEST_APPHOOK(Test); +TEST_MAIN() { + TEST_DEBUG("lhs.out", "rhs.out"); + TEST_RUN_ALL(); +} diff --git a/searchlib/src/tests/queryeval/blueprint/mysearch.h b/searchlib/src/tests/queryeval/blueprint/mysearch.h index 6c91a23e80b..4fa53443123 100644 --- a/searchlib/src/tests/queryeval/blueprint/mysearch.h +++ b/searchlib/src/tests/queryeval/blueprint/mysearch.h @@ -110,11 +110,14 @@ public: SearchIterator::UP createLeafSearch(const TFMDA &tfmda, bool strict) const override { - return SearchIterator::UP(new MySearch("leaf", tfmda, strict)); + return std::make_unique<MySearch>("leaf", tfmda, strict); } - MyLeaf(const FieldSpecBaseList &fields) - : SimpleLeafBlueprint(fields), _got_global_filter(false) + MyLeaf() + : SimpleLeafBlueprint(), _got_global_filter(false) + {} + MyLeaf(FieldSpecBaseList fields) + : SimpleLeafBlueprint(std::move(fields)), _got_global_filter(false) {} MyLeaf &estimate(uint32_t hits, bool empty = false) { diff --git a/searchlib/src/tests/queryeval/dot_product/dot_product_test.cpp b/searchlib/src/tests/queryeval/dot_product/dot_product_test.cpp index 0fef721a6fa..b90f009e4b7 100644 --- a/searchlib/src/tests/queryeval/dot_product/dot_product_test.cpp +++ b/searchlib/src/tests/queryeval/dot_product/dot_product_test.cpp @@ -70,7 +70,8 @@ struct DP { } FakeRequestContext requestContext; Node::UP node = createNode(); - FieldSpecList fields = FieldSpecList().add(FieldSpec(field, fieldId, handle, field_is_filter)); + FieldSpecList fields; + fields.add(FieldSpec(field, fieldId, handle, field_is_filter)); queryeval::Blueprint::UP bp = searchable.createBlueprint(requestContext, fields, *node); bp->fetchPostings(ExecuteInfo::create(strict)); SearchIterator::UP sb = bp->createSearch(*md, strict); diff --git a/searchlib/src/tests/queryeval/equiv/equiv_test.cpp b/searchlib/src/tests/queryeval/equiv/equiv_test.cpp index 887128b8d5e..bc927e233db 100644 --- a/searchlib/src/tests/queryeval/equiv/equiv_test.cpp +++ b/searchlib/src/tests/queryeval/equiv/equiv_test.cpp @@ -45,7 +45,7 @@ EquivTest::test_equiv(bool strict, bool unpack_normal_features, bool unpack_inte FieldSpecBaseList fields; fields.add(FieldSpecBase(1, 1)); fields.add(FieldSpecBase(2, 2)); - auto bp = std::make_unique<EquivBlueprint>(fields, subLayout); + auto bp = std::make_unique<EquivBlueprint>(std::move(fields), subLayout); bp->addTerm(std::make_unique<FakeBlueprint>(FieldSpec("foo", 1, fbh11), a), 1.0); bp->addTerm(std::make_unique<FakeBlueprint>(FieldSpec("bar", 2, fbh21), b), 1.0); diff --git a/searchlib/src/tests/queryeval/filter_search/filter_search_test.cpp b/searchlib/src/tests/queryeval/filter_search/filter_search_test.cpp index fdac3c582b1..ea4753ab847 100644 --- a/searchlib/src/tests/queryeval/filter_search/filter_search_test.cpp +++ b/searchlib/src/tests/queryeval/filter_search/filter_search_test.cpp @@ -66,7 +66,7 @@ struct LeafProxy : SimpleLeafBlueprint { child->setParent(this); } LeafProxy(std::unique_ptr<Blueprint> child_in) - : SimpleLeafBlueprint({}), child(std::move(child_in)) { init(); } + : SimpleLeafBlueprint(), child(std::move(child_in)) { init(); } LeafProxy(const FieldSpec &field, std::unique_ptr<Blueprint> child_in) : SimpleLeafBlueprint(field), child(std::move(child_in)) { init(); } SearchIteratorUP createLeafSearch(const TermFieldMatchDataArray &, bool) const override { abort(); } diff --git a/searchlib/src/tests/queryeval/parallel_weak_and/parallel_weak_and_test.cpp b/searchlib/src/tests/queryeval/parallel_weak_and/parallel_weak_and_test.cpp index 83ffae0524c..3c08b263a7b 100644 --- a/searchlib/src/tests/queryeval/parallel_weak_and/parallel_weak_and_test.cpp +++ b/searchlib/src/tests/queryeval/parallel_weak_and/parallel_weak_and_test.cpp @@ -12,7 +12,6 @@ #include <vespa/searchlib/test/weightedchildrenverifiers.h> #include <vespa/searchlib/test/document_weight_attribute_helper.h> #include <vespa/searchlib/queryeval/document_weight_search_iterator.h> -#include <vespa/searchlib/fef/fef.h> using namespace search::query; using namespace search::queryeval; @@ -167,7 +166,8 @@ struct WandBlueprintSpec } Blueprint::UP blueprint(Searchable &searchable, const std::string &field, const search::query::Node &term) const { - FieldSpecList fields = FieldSpecList().add(FieldSpec(field, fieldId, handle)); + FieldSpecList fields; + fields.add(FieldSpec(field, fieldId, handle)); Blueprint::UP bp = searchable.createBlueprint(requestContext, fields, term); EXPECT_TRUE(dynamic_cast<ParallelWeakAndBlueprint*>(bp.get()) != 0); return bp; diff --git a/searchlib/src/tests/queryeval/queryeval.cpp b/searchlib/src/tests/queryeval/queryeval.cpp index 517e633f328..698bf7c08d5 100644 --- a/searchlib/src/tests/queryeval/queryeval.cpp +++ b/searchlib/src/tests/queryeval/queryeval.cpp @@ -339,7 +339,7 @@ class DummySingleValueBitNumericAttributeBlueprint : public SimpleLeafBlueprint { public: explicit DummySingleValueBitNumericAttributeBlueprint(const SimpleResult & result) : - SimpleLeafBlueprint(FieldSpecBaseList()), + SimpleLeafBlueprint(), _a("a", search::GrowStrategy(), false), _sc(), _tfmd() diff --git a/searchlib/src/tests/queryeval/termwise_eval/termwise_eval_test.cpp b/searchlib/src/tests/queryeval/termwise_eval/termwise_eval_test.cpp index bad7146479c..763701b00a3 100644 --- a/searchlib/src/tests/queryeval/termwise_eval/termwise_eval_test.cpp +++ b/searchlib/src/tests/queryeval/termwise_eval/termwise_eval_test.cpp @@ -66,12 +66,12 @@ struct MyTerm : public SearchIterator { struct MyBlueprint : SimpleLeafBlueprint { std::vector<uint32_t> hits; MyBlueprint(const std::vector<uint32_t> &hits_in) - : SimpleLeafBlueprint(FieldSpecBaseList()), hits(hits_in) + : SimpleLeafBlueprint(), hits(hits_in) { setEstimate(HitEstimate(hits.size(), hits.empty())); } MyBlueprint(const std::vector<uint32_t> &hits_in, bool allow_termwise_eval) - : SimpleLeafBlueprint(FieldSpecBaseList()), hits(hits_in) + : SimpleLeafBlueprint(), hits(hits_in) { setEstimate(HitEstimate(hits.size(), hits.empty())); set_allow_termwise_eval(allow_termwise_eval); diff --git a/searchlib/src/tests/queryeval/weighted_set_term/weighted_set_term_test.cpp b/searchlib/src/tests/queryeval/weighted_set_term/weighted_set_term_test.cpp index 51594c478e5..90e16d4feff 100644 --- a/searchlib/src/tests/queryeval/weighted_set_term/weighted_set_term_test.cpp +++ b/searchlib/src/tests/queryeval/weighted_set_term/weighted_set_term_test.cpp @@ -66,7 +66,8 @@ struct WS { FakeRequestContext requestContext; MatchData::UP md = layout.createMatchData(); Node::UP node = createNode(); - FieldSpecList fields = FieldSpecList().add(FieldSpec(field, fieldId, handle)); + FieldSpecList fields; + fields.add(FieldSpec(field, fieldId, handle)); queryeval::Blueprint::UP bp = searchable.createBlueprint(requestContext, fields, *node); bp->fetchPostings(ExecuteInfo::create(strict)); SearchIterator::UP sb = bp->createSearch(*md, strict); @@ -80,7 +81,8 @@ struct WS { md->resolveTermField(handle)->tagAsNotNeeded(); } Node::UP node = createNode(); - FieldSpecList fields = FieldSpecList().add(FieldSpec(field, fieldId, handle, field_is_filter)); + FieldSpecList fields; + fields.add(FieldSpec(field, fieldId, handle, field_is_filter)); queryeval::Blueprint::UP bp = searchable.createBlueprint(requestContext, fields, *node); bp->fetchPostings(ExecuteInfo::create(strict)); SearchIterator::UP sb = bp->createSearch(*md, strict); diff --git a/searchlib/src/vespa/searchlib/queryeval/blueprint.cpp b/searchlib/src/vespa/searchlib/queryeval/blueprint.cpp index 87f7ab99002..488c58e3119 100644 --- a/searchlib/src/vespa/searchlib/queryeval/blueprint.cpp +++ b/searchlib/src/vespa/searchlib/queryeval/blueprint.cpp @@ -2,7 +2,6 @@ #include "blueprint.h" #include "leaf_blueprints.h" -#include "intermediate_blueprints.h" #include "emptysearch.h" #include "full_search.h" #include "field_spec.hpp" @@ -17,7 +16,6 @@ #include <vespa/vespalib/util/classname.h> #include <vespa/vespalib/util/require.h> #include <vespa/vespalib/data/slime/inserter.h> -#include <type_traits> #include <map> #include <vespa/log/log.h> @@ -89,13 +87,30 @@ Blueprint::sat_sum(const std::vector<HitEstimate> &data, uint32_t docid_limit) return { uint32_t(std::min(sum, uint64_t(limit))), empty }; } -Blueprint::State::State(const FieldSpecBaseList &fields_in) - : _fields(fields_in), - _estimate(), - _cost_tier(COST_TIER_NORMAL), +Blueprint::State::State() + : _fields(), + _estimateHits(0), _tree_size(1), + _estimateEmpty(true), _allow_termwise_eval(true), - _want_global_filter(false) + _want_global_filter(false), + _cost_tier(COST_TIER_NORMAL) +{} + +Blueprint::State::State(FieldSpecBase field) + : State() +{ + _fields.add(field); +} + +Blueprint::State::State(FieldSpecBaseList fields_in) + : _fields(std::move(fields_in)), + _estimateHits(0), + _tree_size(1), + _estimateEmpty(true), + _allow_termwise_eval(true), + _want_global_filter(false), + _cost_tier(COST_TIER_NORMAL) { } @@ -387,10 +402,10 @@ IntermediateBlueprint::calculateEstimate() const return combine(estimates); } -uint32_t +uint8_t IntermediateBlueprint::calculate_cost_tier() const { - uint32_t cost_tier = State::COST_TIER_MAX; + uint8_t cost_tier = State::COST_TIER_MAX; for (const Blueprint::UP &child : _children) { cost_tier = std::min(cost_tier, child->getState().cost_tier()); } @@ -483,6 +498,7 @@ IntermediateBlueprint::mixChildrenFields() const } } } + fieldList.reserve(fieldMap.size()); for (const auto & entry : fieldMap) { fieldList.add(*entry.second); } @@ -684,18 +700,9 @@ IntermediateBlueprint::calculateUnpackInfo(const fef::MatchData & md) const //----------------------------------------------------------------------------- -LeafBlueprint::LeafBlueprint(const FieldSpecBaseList &fields, bool allow_termwise_eval) - : _state(fields) -{ - _state.allow_termwise_eval(allow_termwise_eval); -} - -LeafBlueprint::~LeafBlueprint() = default; - void -LeafBlueprint::fetchPostings(const ExecuteInfo &execInfo) +LeafBlueprint::fetchPostings(const ExecuteInfo &) { - (void) execInfo; } void @@ -739,6 +746,7 @@ LeafBlueprint::setEstimate(HitEstimate est) void LeafBlueprint::set_cost_tier(uint32_t value) { + assert(value < 0x100); _state.cost_tier(value); notifyChange(); } diff --git a/searchlib/src/vespa/searchlib/queryeval/blueprint.h b/searchlib/src/vespa/searchlib/queryeval/blueprint.h index c76f0a22e98..1ea02e41a62 100644 --- a/searchlib/src/vespa/searchlib/queryeval/blueprint.h +++ b/searchlib/src/vespa/searchlib/queryeval/blueprint.h @@ -65,22 +65,25 @@ public: { private: FieldSpecBaseList _fields; - HitEstimate _estimate; - uint32_t _cost_tier; - uint32_t _tree_size; - bool _allow_termwise_eval; - bool _want_global_filter; + uint32_t _estimateHits; + uint32_t _tree_size : 20; + bool _estimateEmpty : 1; + bool _allow_termwise_eval : 1; + bool _want_global_filter : 1; + uint8_t _cost_tier; public: - static constexpr uint32_t COST_TIER_NORMAL = 1; - static constexpr uint32_t COST_TIER_EXPENSIVE = 2; - static constexpr uint32_t COST_TIER_MAX = 999; + static constexpr uint8_t COST_TIER_NORMAL = 1; + static constexpr uint8_t COST_TIER_EXPENSIVE = 2; + static constexpr uint8_t COST_TIER_MAX = 255; - State(const FieldSpecBaseList &fields_in); + State(); + State(FieldSpecBase field); + State(FieldSpecBaseList fields_in); State(const State &rhs) = delete; - State(State &&rhs) = default; + State(State &&rhs) noexcept = default; State &operator=(const State &rhs) = delete; - State &operator=(State &&rhs) = default; + State &operator=(State &&rhs) noexcept = default; ~State(); bool isTermLike() const { return !_fields.empty(); } @@ -97,21 +100,27 @@ public: return nullptr; } - void estimate(HitEstimate est) { _estimate = est; } - HitEstimate estimate() const { return _estimate; } + void estimate(HitEstimate est) { + _estimateHits = est.estHits; + _estimateEmpty = est.empty; + } + HitEstimate estimate() const { return HitEstimate(_estimateHits, _estimateEmpty); } double hit_ratio(uint32_t docid_limit) const { - uint32_t total_hits = _estimate.estHits; + uint32_t total_hits = _estimateHits; uint32_t total_docs = std::max(total_hits, docid_limit); return (total_docs == 0) ? 0.0 : double(total_hits) / double(total_docs); } - void tree_size(uint32_t value) { _tree_size = value; } + void tree_size(uint32_t value) { + assert(value < 0x100000); + _tree_size = value; + } uint32_t tree_size() const { return _tree_size; } void allow_termwise_eval(bool value) { _allow_termwise_eval = value; } bool allow_termwise_eval() const { return _allow_termwise_eval; } void want_global_filter(bool value) { _want_global_filter = value; } bool want_global_filter() const { return _want_global_filter; } - void cost_tier(uint32_t value) { _cost_tier = value; } - uint32_t cost_tier() const { return _cost_tier; } + void cost_tier(uint8_t value) { _cost_tier = value; } + uint8_t cost_tier() const { return _cost_tier; } }; // utility that just takes maximum estimate @@ -269,7 +278,7 @@ protected: virtual State calculateState() const = 0; public: - StateCache() : _stale(true), _state(FieldSpecBaseList()) {} + StateCache() : _stale(true), _state() {} const State &getState() const final { if (_stale) { updateState(); @@ -287,7 +296,7 @@ class IntermediateBlueprint : public blueprint::StateCache private: Children _children; HitEstimate calculateEstimate() const; - uint32_t calculate_cost_tier() const; + uint8_t calculate_cost_tier() const; uint32_t calculate_tree_size() const; bool infer_allow_termwise_eval() const; bool infer_want_global_filter() const; @@ -348,7 +357,6 @@ class LeafBlueprint : public Blueprint { private: State _state; - protected: void optimize(Blueprint* &self) final; void setEstimate(HitEstimate est); @@ -357,9 +365,25 @@ protected: void set_want_global_filter(bool value); void set_tree_size(uint32_t value); - LeafBlueprint(const FieldSpecBaseList &fields, bool allow_termwise_eval); + LeafBlueprint(bool allow_termwise_eval) + : _state() + { + _state.allow_termwise_eval(allow_termwise_eval); + } + + LeafBlueprint(FieldSpecBase field, bool allow_termwise_eval) + : _state(field) + { + _state.allow_termwise_eval(allow_termwise_eval); + } + LeafBlueprint(FieldSpecBaseList fields, bool allow_termwise_eval) + : _state(std::move(fields)) + { + _state.allow_termwise_eval(allow_termwise_eval); + } + public: - ~LeafBlueprint() override; + ~LeafBlueprint() override = default; const State &getState() const final { return _state; } void setDocIdLimit(uint32_t limit) final { Blueprint::setDocIdLimit(limit); } void fetchPostings(const ExecuteInfo &execInfo) override; @@ -372,14 +396,15 @@ public: // for leaf nodes representing a single term struct SimpleLeafBlueprint : LeafBlueprint { - explicit SimpleLeafBlueprint(const FieldSpecBase &field) : LeafBlueprint(FieldSpecBaseList().add(field), true) {} - explicit SimpleLeafBlueprint(const FieldSpecBaseList &fields) : LeafBlueprint(fields, true) {} + explicit SimpleLeafBlueprint() : LeafBlueprint(true) {} + explicit SimpleLeafBlueprint(FieldSpecBase field) : LeafBlueprint(field, true) {} + explicit SimpleLeafBlueprint(FieldSpecBaseList fields) : LeafBlueprint(std::move(fields), true) {} }; // for leaf nodes representing more complex structures like wand/phrase struct ComplexLeafBlueprint : LeafBlueprint { - explicit ComplexLeafBlueprint(const FieldSpecBase &field) : LeafBlueprint(FieldSpecBaseList().add(field), false) {} - explicit ComplexLeafBlueprint(const FieldSpecBaseList &fields) : LeafBlueprint(fields, false) {} + explicit ComplexLeafBlueprint(FieldSpecBase field) : LeafBlueprint(field, false) {} + explicit ComplexLeafBlueprint(FieldSpecBaseList fields) : LeafBlueprint(std::move(fields), false) {} }; //----------------------------------------------------------------------------- diff --git a/searchlib/src/vespa/searchlib/queryeval/equiv_blueprint.cpp b/searchlib/src/vespa/searchlib/queryeval/equiv_blueprint.cpp index c2f4c53224d..af6b59dd6ca 100644 --- a/searchlib/src/vespa/searchlib/queryeval/equiv_blueprint.cpp +++ b/searchlib/src/vespa/searchlib/queryeval/equiv_blueprint.cpp @@ -40,10 +40,9 @@ public: }; -EquivBlueprint::EquivBlueprint(const FieldSpecBaseList &fields, +EquivBlueprint::EquivBlueprint(FieldSpecBaseList fields, fef::MatchDataLayout subtree_mdl) - : ComplexLeafBlueprint(fields), - _fields(fields), + : ComplexLeafBlueprint(std::move(fields)), _estimate(), _layout(subtree_mdl), _terms(), diff --git a/searchlib/src/vespa/searchlib/queryeval/equiv_blueprint.h b/searchlib/src/vespa/searchlib/queryeval/equiv_blueprint.h index 86bd4f09cd4..140da63afcc 100644 --- a/searchlib/src/vespa/searchlib/queryeval/equiv_blueprint.h +++ b/searchlib/src/vespa/searchlib/queryeval/equiv_blueprint.h @@ -10,14 +10,13 @@ namespace search::queryeval { class EquivBlueprint : public ComplexLeafBlueprint { private: - FieldSpecBaseList _fields; HitEstimate _estimate; fef::MatchDataLayout _layout; std::vector<Blueprint::UP> _terms; std::vector<double> _exactness; public: - EquivBlueprint(const FieldSpecBaseList &fields, fef::MatchDataLayout subtree_mdl); + EquivBlueprint(FieldSpecBaseList fields, fef::MatchDataLayout subtree_mdl); ~EquivBlueprint() override; // used by create visitor diff --git a/searchlib/src/vespa/searchlib/queryeval/field_spec.h b/searchlib/src/vespa/searchlib/queryeval/field_spec.h index f37403471ac..a1050209b41 100644 --- a/searchlib/src/vespa/searchlib/queryeval/field_spec.h +++ b/searchlib/src/vespa/searchlib/queryeval/field_spec.h @@ -4,6 +4,7 @@ #include <vespa/searchlib/fef/handle.h> #include <vespa/vespalib/stllike/string.h> +#include <vespa/vespalib/util/small_vector.h> #include <vector> namespace search::fef { @@ -56,12 +57,18 @@ private: class FieldSpecBaseList { private: - using List = std::vector<FieldSpecBase>; + using List = vespalib::SmallVector<FieldSpecBase, 1>; List _list; public: + FieldSpecBaseList() = default; + FieldSpecBaseList(FieldSpecBaseList &&) noexcept = default; + FieldSpecBaseList & operator=(FieldSpecBaseList &&) noexcept = default; + FieldSpecBaseList(const FieldSpecBaseList &) = default; + FieldSpecBaseList & operator=(FieldSpecBaseList &) = delete; ~FieldSpecBaseList(); - using const_iterator = List::const_iterator; + void reserve(size_t sz) { _list.reserve(sz); } + using const_iterator = const FieldSpecBase *; FieldSpecBaseList &add(const FieldSpecBase &spec) { _list.push_back(spec); return *this; @@ -71,8 +78,6 @@ public: const_iterator begin() const { return _list.begin(); } const_iterator end() const { return _list.end(); } const FieldSpecBase &operator[](size_t i) const { return _list[i]; } - void clear() { _list.clear(); } - void swap(FieldSpecBaseList & rhs) { _list.swap(rhs._list); } }; /** @@ -84,6 +89,11 @@ private: std::vector<FieldSpec> _list; public: + FieldSpecList() = default; + FieldSpecList(FieldSpecList &&) noexcept = delete; + FieldSpecList & operator=(FieldSpecList &&) noexcept = delete; + FieldSpecList(const FieldSpecList &) noexcept = delete; + FieldSpecList & operator=(const FieldSpecList &) noexcept = delete; ~FieldSpecList(); FieldSpecList &add(const FieldSpec &spec) { _list.push_back(spec); @@ -93,7 +103,6 @@ public: size_t size() const { return _list.size(); } const FieldSpec &operator[](size_t i) const { return _list[i]; } void clear() { _list.clear(); } - void swap(FieldSpecList & rhs) { _list.swap(rhs._list); } }; } diff --git a/searchlib/src/vespa/searchlib/queryeval/leaf_blueprints.cpp b/searchlib/src/vespa/searchlib/queryeval/leaf_blueprints.cpp index 86f520c8711..1af5f7fa3d4 100644 --- a/searchlib/src/vespa/searchlib/queryeval/leaf_blueprints.cpp +++ b/searchlib/src/vespa/searchlib/queryeval/leaf_blueprints.cpp @@ -11,8 +11,7 @@ namespace search::queryeval { //----------------------------------------------------------------------------- SearchIterator::UP -EmptyBlueprint::createLeafSearch(const search::fef::TermFieldMatchDataArray &, - bool) const +EmptyBlueprint::createLeafSearch(const search::fef::TermFieldMatchDataArray &, bool) const { return std::make_unique<EmptySearch>(); } @@ -23,22 +22,11 @@ EmptyBlueprint::createFilterSearch(bool /*strict*/, FilterConstraint /* constrai return std::make_unique<EmptySearch>(); } -EmptyBlueprint::EmptyBlueprint(const FieldSpecBase &field) - : SimpleLeafBlueprint(field) +EmptyBlueprint::EmptyBlueprint(FieldSpecBaseList fields) + : SimpleLeafBlueprint(std::move(fields)) { } -EmptyBlueprint::EmptyBlueprint(const FieldSpecBaseList &fields) - : SimpleLeafBlueprint(fields) -{ -} - -EmptyBlueprint::EmptyBlueprint() - : SimpleLeafBlueprint(FieldSpecBaseList()) -{ -} - - SearchIterator::UP AlwaysTrueBlueprint::createLeafSearch(const search::fef::TermFieldMatchDataArray &, bool) const { @@ -51,7 +39,7 @@ AlwaysTrueBlueprint::createFilterSearch(bool /*strict*/, FilterConstraint /* con return std::make_unique<FullSearch>(); } -AlwaysTrueBlueprint::AlwaysTrueBlueprint() : SimpleLeafBlueprint(FieldSpecBaseList()) +AlwaysTrueBlueprint::AlwaysTrueBlueprint() : SimpleLeafBlueprint() { setEstimate(HitEstimate(search::endDocId, false)); } @@ -61,25 +49,23 @@ AlwaysTrueBlueprint::AlwaysTrueBlueprint() : SimpleLeafBlueprint(FieldSpecBaseLi SearchIterator::UP SimpleBlueprint::createLeafSearch(const search::fef::TermFieldMatchDataArray &, bool strict) const { - SimpleSearch *ss = new SimpleSearch(_result, strict); - SearchIterator::UP search(ss); - ss->tag(_tag); + auto search = std::make_unique<SimpleSearch>(_result, strict); + search->tag(_tag); return search; } SearchIterator::UP SimpleBlueprint::createFilterSearch(bool strict, FilterConstraint constraint) const { - SimpleSearch *ss = new SimpleSearch(_result, strict); - SearchIterator::UP search(ss); - ss->tag(_tag + - (strict ? "<strict," : "<nostrict,") + - (constraint == FilterConstraint::UPPER_BOUND ? "upper>" : "lower>")); + auto search = std::make_unique<SimpleSearch>(_result, strict); + search->tag(_tag + + (strict ? "<strict," : "<nostrict,") + + (constraint == FilterConstraint::UPPER_BOUND ? "upper>" : "lower>")); return search; } SimpleBlueprint::SimpleBlueprint(const SimpleResult &result) - : SimpleLeafBlueprint(FieldSpecBaseList()), + : SimpleLeafBlueprint(), _tag(), _result(result) { diff --git a/searchlib/src/vespa/searchlib/queryeval/leaf_blueprints.h b/searchlib/src/vespa/searchlib/queryeval/leaf_blueprints.h index b4d7682cf89..57c3e5f3533 100644 --- a/searchlib/src/vespa/searchlib/queryeval/leaf_blueprints.h +++ b/searchlib/src/vespa/searchlib/queryeval/leaf_blueprints.h @@ -16,9 +16,9 @@ class EmptyBlueprint : public SimpleLeafBlueprint protected: SearchIterator::UP createLeafSearch(const search::fef::TermFieldMatchDataArray &tfmda, bool strict) const override; public: - EmptyBlueprint(const FieldSpecBaseList &fields); - EmptyBlueprint(const FieldSpecBase &field); - EmptyBlueprint(); + EmptyBlueprint(FieldSpecBaseList fields); + EmptyBlueprint(FieldSpecBase field) : SimpleLeafBlueprint(field) {} + EmptyBlueprint() = default; SearchIterator::UP createFilterSearch(bool strict, FilterConstraint constraint) const override; }; diff --git a/storage/src/tests/distributor/check_condition_test.cpp b/storage/src/tests/distributor/check_condition_test.cpp index d4b7d7019d7..1b5cede8af6 100644 --- a/storage/src/tests/distributor/check_condition_test.cpp +++ b/storage/src/tests/distributor/check_condition_test.cpp @@ -9,7 +9,8 @@ #include <vespa/storage/distributor/persistence_operation_metric_set.h> #include <vespa/storageapi/message/persistence.h> #include <tests/distributor/distributor_stripe_test_util.h> -#include <vespa/vespalib/gtest/gtest.h> +#include <gtest/gtest.h> +#include <gmock/gmock.h> using namespace ::testing; @@ -27,6 +28,7 @@ public: BucketId _bucket_id{16, 1234}; TestAndSetCondition _tas_cond{"foo or bar"}; PersistenceOperationMetricSet _metrics{"dummy_metrics", nullptr}; + uint32_t _trace_level{5}; CheckConditionTest(); ~CheckConditionTest() override; @@ -52,7 +54,8 @@ public: auto bucket = Bucket(FixedBucketSpaces::default_space(), _bucket_id); assert(_bucket_id.contains(doc_bucket)); return CheckCondition::create_if_inconsistent_replicas(bucket, bucket_space, _doc_id, _tas_cond, - node_context(), operation_context(), _metrics); + node_context(), operation_context(), _metrics, + _trace_level); } std::shared_ptr<api::GetCommand> sent_get_command(size_t idx) { @@ -86,6 +89,12 @@ public: return make_reply(*sent_get_command(cmd_idx), ts, true, false); } + std::shared_ptr<api::GetReply> make_trace_reply(size_t cmd_idx, api::Timestamp ts, std::string trace_message) { + auto reply = make_reply(*sent_get_command(cmd_idx), ts, true, false); + MBUS_TRACE(reply->getTrace(), _trace_level, trace_message); + return reply; + } + std::shared_ptr<api::GetReply> make_failed_reply(size_t cmd_idx) { auto reply = make_reply(*sent_get_command(cmd_idx), 0, false, false); reply->setResult(api::ReturnCode(api::ReturnCode::ABORTED, "did a bork")); @@ -149,6 +158,7 @@ TEST_F(CheckConditionTest, starting_sends_condition_probe_gets) { EXPECT_EQ(cmd->condition(), _tas_cond); EXPECT_EQ(cmd->getFieldSet(), NoFields::NAME); EXPECT_EQ(cmd->internal_read_consistency(), api::InternalReadConsistency::Strong); + EXPECT_EQ(cmd->getTrace().getLevel(), _trace_level); } TEST_F(CheckConditionTest, condition_matching_completes_check_with_match_outcome) { @@ -232,4 +242,15 @@ TEST_F(CheckConditionTest, check_fails_if_replica_set_changed_between_start_and_ }); } +TEST_F(CheckConditionTest, nested_get_traces_are_propagated_to_outcome) { + test_cond_with_2_gets_sent([&](auto& cond) { + cond.handle_reply(_sender, make_trace_reply(0, 100, "hello")); + cond.handle_reply(_sender, make_trace_reply(1, 200, "world")); + }, [&](auto& outcome) { + auto trace_str = outcome.trace().toString(); + EXPECT_THAT(trace_str, HasSubstr("hello")); + EXPECT_THAT(trace_str, HasSubstr("world")); + }); +} + } diff --git a/storage/src/tests/distributor/distributor_stripe_test_util.h b/storage/src/tests/distributor/distributor_stripe_test_util.h index 707418512f9..9963b2c96b4 100644 --- a/storage/src/tests/distributor/distributor_stripe_test_util.h +++ b/storage/src/tests/distributor/distributor_stripe_test_util.h @@ -220,6 +220,15 @@ public: return cmd; } + template <typename ReplyType> + requires std::is_base_of_v<api::StorageReply, ReplyType> + [[nodiscard]] std::shared_ptr<ReplyType> sent_reply(size_t idx) { + assert(idx < _sender.replies().size()); + auto reply = std::dynamic_pointer_cast<ReplyType>(_sender.reply(idx)); + assert(reply != nullptr); + return reply; + } + void config_enable_condition_probing(bool enable); void tag_content_node_supports_condition_probing(uint16_t index, bool supported); diff --git a/storage/src/tests/distributor/getoperationtest.cpp b/storage/src/tests/distributor/getoperationtest.cpp index 6601fcabbeb..600a4de462e 100644 --- a/storage/src/tests/distributor/getoperationtest.cpp +++ b/storage/src/tests/distributor/getoperationtest.cpp @@ -16,8 +16,9 @@ #include <vespa/storage/distributor/externaloperationhandler.h> #include <vespa/storage/distributor/operations/external/getoperation.h> #include <vespa/storageapi/message/persistence.h> -#include <vespa/vespalib/gtest/gtest.h> #include <iomanip> +#include <gtest/gtest.h> +#include <gmock/gmock.h> using config::ConfigGetter; using config::FileSpec; @@ -75,7 +76,8 @@ struct GetOperationTest : Test, DistributorStripeTestUtil { std::string authorVal, uint32_t timestamp, bool is_tombstone = false, - bool condition_matched = false) + bool condition_matched = false, + std::string trace_msg = "") { if (idx == LastCommand) { idx = _sender.commands().size() - 1; @@ -98,6 +100,9 @@ struct GetOperationTest : Test, DistributorStripeTestUtil { auto reply = std::make_shared<api::GetReply>(*tmp, doc, timestamp, false, is_tombstone, condition_matched); reply->setResult(result); + if (!trace_msg.empty()) { + MBUS_TRACE(reply->getTrace(), 1, trace_msg); + } op->receive(_sender, reply); } @@ -110,6 +115,10 @@ struct GetOperationTest : Test, DistributorStripeTestUtil { sendReply(idx, api::ReturnCode::OK, "", timestamp, false, condition_match); } + void reply_with_trace(uint32_t idx, uint32_t timestamp, std::string trace_message) { + sendReply(idx, api::ReturnCode::OK, "", timestamp, false, true, std::move(trace_message)); + } + void replyWithFailure() { sendReply(LastCommand, api::ReturnCode::IO_FAILURE, "", 0); } @@ -682,6 +691,7 @@ void GetOperationTest::set_up_condition_match_get_operation() { TestAndSetCondition my_cond("my_cool_condition"); auto msg = std::make_shared<api::GetCommand>(makeDocumentBucket(BucketId(0)), docId, document::NoFields::NAME); msg->set_condition(my_cond); + msg->getTrace().setLevel(9); // FIXME a very tiny bit dirty to set this here >_> start_operation(std::move(msg), api::InternalReadConsistency::Strong); ASSERT_EQ("Get => 0,Get => 2,Get => 1", _sender.getCommands(true)); @@ -742,4 +752,21 @@ TEST_F(GetOperationTest, condition_match_result_is_aggregated_for_newest_replica EXPECT_EQ(replica_of(api::Timestamp(500), bucketId, 2, true, false), *op->newest_replica()); } +TEST_F(GetOperationTest, trace_is_aggregated_from_all_sub_replies_and_propagated_to_operation_reply) { + ASSERT_NO_FATAL_FAILURE(set_up_condition_match_get_operation()); + + ASSERT_NO_FATAL_FAILURE(reply_with_trace(0, 400, "foo")); + ASSERT_NO_FATAL_FAILURE(reply_with_trace(1, 500, "bar")); + ASSERT_NO_FATAL_FAILURE(reply_with_trace(2, 300, "baz")); + + ASSERT_EQ(_sender.replies().size(), 1); + auto get_reply = sent_reply<api::GetReply>(0); + + auto trace_str = get_reply->getTrace().toString(); + EXPECT_THAT(trace_str, HasSubstr("foo")); + EXPECT_THAT(trace_str, HasSubstr("bar")); + EXPECT_THAT(trace_str, HasSubstr("baz")); +} + + } diff --git a/storage/src/tests/distributor/putoperationtest.cpp b/storage/src/tests/distributor/putoperationtest.cpp index 405f25f9359..ff375e5b902 100644 --- a/storage/src/tests/distributor/putoperationtest.cpp +++ b/storage/src/tests/distributor/putoperationtest.cpp @@ -10,20 +10,18 @@ #include <vespa/storageapi/message/bucket.h> #include <vespa/storageapi/message/persistence.h> #include <vespa/storageapi/message/state.h> -#include <vespa/vespalib/gtest/gtest.h> #include <vespa/vespalib/text/stringtokenizer.h> #include <vespa/config/helper/configgetter.h> +#include <gtest/gtest.h> +#include <gmock/gmock.h> -using std::shared_ptr; using config::ConfigGetter; using config::FileSpec; using vespalib::string; using namespace document; -using namespace storage; -using namespace storage::api; -using namespace storage::lib; using namespace std::literals::string_literals; using document::test::makeDocumentBucket; +using storage::api::TestAndSetCondition; using namespace ::testing; @@ -493,7 +491,8 @@ TEST_F(PutOperationTest, update_correct_bucket_on_remapped_put) { { std::shared_ptr<api::StorageCommand> msg2 = _sender.command(0); std::shared_ptr<api::StorageReply> reply(msg2->makeReply().release()); - PutReply* sreply = (PutReply*)reply.get(); + auto* sreply = dynamic_cast<api::PutReply*>(reply.get()); + ASSERT_TRUE(sreply); sreply->remapBucketId(document::BucketId(17, 13)); sreply->setBucketInfo(api::BucketInfo(1,2,3,4,5)); op->receive(_sender, reply); @@ -690,6 +689,7 @@ void PutOperationTest::set_up_tas_put_with_2_inconsistent_replica_nodes(bool cre auto put = createPut(doc); put->setCondition(TestAndSetCondition("test.foo")); put->set_create_if_non_existent(create); + put->getTrace().setLevel(9); sendPut(std::move(put)); ASSERT_EQ("Get => 1,Get => 0", _sender.getCommands(true)); } @@ -821,4 +821,41 @@ TEST_F(PutOperationTest, conditional_put_no_replicas_case_with_create_set_acts_a EXPECT_TRUE(put_n1->get_create_if_non_existent()); } +TEST_F(PutOperationTest, trace_is_propagated_from_condition_probe_gets_ok_probe_case) { + ASSERT_NO_FATAL_FAILURE(set_up_tas_put_with_2_inconsistent_replica_nodes()); + + ASSERT_EQ(sent_get_command(0)->getTrace().getLevel(), 9); + auto get_reply = make_get_reply(*sent_get_command(0), 50, false, true); + MBUS_TRACE(get_reply->getTrace(), 1, "a foo walks into a bar"); + + op->receive(_sender, get_reply); + op->receive(_sender, make_get_reply(*sent_get_command(1), 50, false, true)); + + ASSERT_EQ("Get => 1,Get => 0,Put => 1,Put => 0", _sender.getCommands(true)); + sendReply(2); + sendReply(3); + ASSERT_EQ(_sender.replies().size(), 1); + auto put_reply = sent_reply<api::PutReply>(0); + + auto trace_str = put_reply->getTrace().toString(); + EXPECT_THAT(trace_str, HasSubstr("a foo walks into a bar")); +} + +TEST_F(PutOperationTest, trace_is_propagated_from_condition_probe_gets_failed_probe_case) { + ASSERT_NO_FATAL_FAILURE(set_up_tas_put_with_2_inconsistent_replica_nodes()); + + auto get_reply = make_get_reply(*sent_get_command(0), 50, false, false); + MBUS_TRACE(get_reply->getTrace(), 1, "a foo walks into a zoo"); + + op->receive(_sender, get_reply); + op->receive(_sender, make_get_reply(*sent_get_command(1), 50, false, false)); + + ASSERT_EQ("Get => 1,Get => 0", _sender.getCommands(true)); + ASSERT_EQ(_sender.replies().size(), 1); + auto put_reply = sent_reply<api::PutReply>(0); + + auto trace_str = put_reply->getTrace().toString(); + EXPECT_THAT(trace_str, HasSubstr("a foo walks into a zoo")); +} + } diff --git a/storage/src/vespa/storage/distributor/operations/external/check_condition.cpp b/storage/src/vespa/storage/distributor/operations/external/check_condition.cpp index 7f7e6b08c1f..9f7dbcaa132 100644 --- a/storage/src/vespa/storage/distributor/operations/external/check_condition.cpp +++ b/storage/src/vespa/storage/distributor/operations/external/check_condition.cpp @@ -16,6 +16,29 @@ LOG_SETUP(".distributor.operations.external.check_condition"); namespace storage::distributor { +CheckCondition::Outcome::Outcome(api::ReturnCode error_code, vespalib::Trace trace) noexcept + : _error_code(std::move(error_code)), + _result(Result::HasError), + _trace(std::move(trace)) +{ +} + +CheckCondition::Outcome::Outcome(Result result, vespalib::Trace trace) noexcept + : _error_code(), + _result(result), + _trace(std::move(trace)) +{ +} + +CheckCondition::Outcome::Outcome(Result result) noexcept + : _error_code(), + _result(result), + _trace() +{ +} + +CheckCondition::Outcome::~Outcome() = default; + CheckCondition::CheckCondition(Outcome known_outcome, const DistributorBucketSpace& bucket_space, const DistributorNodeContext& node_ctx, @@ -36,6 +59,7 @@ CheckCondition::CheckCondition(const document::Bucket& bucket, const DistributorBucketSpace& bucket_space, const DistributorNodeContext& node_ctx, PersistenceOperationMetricSet& metric, + uint32_t trace_level, private_ctor_tag) : _doc_id_bucket(bucket), _bucket_space(bucket_space), @@ -48,6 +72,7 @@ CheckCondition::CheckCondition(const document::Bucket& bucket, // Side note: the BucketId provided to the GetCommand is ignored; GetOperation computes explicitly from the doc ID. auto get_cmd = std::make_shared<api::GetCommand>(_doc_id_bucket, doc_id, document::NoFields::NAME); get_cmd->set_condition(tas_condition); + get_cmd->getTrace().setLevel(trace_level); _cond_get_op = std::make_shared<GetOperation>(_node_ctx, _bucket_space, _bucket_space.getBucketDatabase().acquire_read_guard(), std::move(get_cmd), @@ -129,7 +154,8 @@ void CheckCondition::handle_internal_get_operation_reply(std::shared_ptr<api::St if (reply->getResult().success()) { if (_cond_get_op->any_replicas_failed()) { _outcome.emplace(api::ReturnCode(api::ReturnCode::ABORTED, - "One or more replicas failed during test-and-set condition evaluation")); + "One or more replicas failed during test-and-set condition evaluation"), + reply->steal_trace()); return; } const auto state_version_now = _bucket_space.getClusterState().getVersion(); @@ -143,13 +169,14 @@ void CheckCondition::handle_internal_get_operation_reply(std::shared_ptr<api::St // explicitly edge-handled...! _outcome.emplace(api::ReturnCode(api::ReturnCode::BUCKET_NOT_FOUND, "Bucket ownership or replica set changed between condition " - "read and operation write phases")); + "read and operation write phases"), + reply->steal_trace()); } else { auto maybe_newest = _cond_get_op->newest_replica(); - _outcome.emplace(newest_replica_to_outcome(maybe_newest)); + _outcome.emplace(newest_replica_to_outcome(maybe_newest), reply->steal_trace()); } } else { - _outcome.emplace(reply->getResult()); + _outcome.emplace(reply->getResult(), reply->steal_trace()); } } @@ -193,7 +220,8 @@ CheckCondition::create_if_inconsistent_replicas(const document::Bucket& bucket, const documentapi::TestAndSetCondition& tas_condition, const DistributorNodeContext& node_ctx, const DistributorStripeOperationContext& op_ctx, - PersistenceOperationMetricSet& metric) + PersistenceOperationMetricSet& metric, + uint32_t trace_level) { // TODO move this check to the caller? if (!op_ctx.distributor_config().enable_condition_probing()) { @@ -210,7 +238,7 @@ CheckCondition::create_if_inconsistent_replicas(const document::Bucket& bucket, return {}; // Want write-repair, but one or more nodes are too old to use the feature } return std::make_shared<CheckCondition>(bucket, doc_id, tas_condition, bucket_space, - node_ctx, metric, private_ctor_tag{}); + node_ctx, metric, trace_level, private_ctor_tag{}); } } diff --git a/storage/src/vespa/storage/distributor/operations/external/check_condition.h b/storage/src/vespa/storage/distributor/operations/external/check_condition.h index ea3bfff2e3e..2a659c55081 100644 --- a/storage/src/vespa/storage/distributor/operations/external/check_condition.h +++ b/storage/src/vespa/storage/distributor/operations/external/check_condition.h @@ -60,17 +60,10 @@ public: NotFound, }; - explicit Outcome(api::ReturnCode error_code) noexcept - : _error_code(std::move(error_code)), - _result(Result::HasError) - { - } - - explicit Outcome(Result result) noexcept - : _error_code(), - _result(result) - { - } + Outcome(api::ReturnCode error_code, vespalib::Trace trace) noexcept; + Outcome(Result result, vespalib::Trace trace) noexcept; + explicit Outcome(Result result) noexcept; + ~Outcome(); [[nodiscard]] bool failed() const noexcept { return _error_code.failed(); @@ -88,9 +81,18 @@ public: return _result == Result::NotFound; } + [[nodiscard]] const vespalib::Trace& trace() const noexcept { + return _trace; + } + + [[nodiscard]] vespalib::Trace&& steal_trace() noexcept { + return std::move(_trace); + } + private: api::ReturnCode _error_code; Result _result; + vespalib::Trace _trace; }; private: const document::Bucket _doc_id_bucket; @@ -113,6 +115,7 @@ public: const DistributorBucketSpace& bucket_space, const DistributorNodeContext& node_ctx, PersistenceOperationMetricSet& metric, + uint32_t trace_level, private_ctor_tag); ~CheckCondition(); @@ -121,7 +124,7 @@ public: const std::shared_ptr<api::StorageReply>& reply); void cancel(DistributorStripeMessageSender& sender); - [[nodiscard]] const std::optional<Outcome>& maybe_outcome() const noexcept { + [[nodiscard]] std::optional<Outcome>& maybe_outcome() noexcept { return _outcome; } @@ -132,7 +135,8 @@ public: const documentapi::TestAndSetCondition& tas_condition, const DistributorNodeContext& node_ctx, const DistributorStripeOperationContext& op_ctx, - PersistenceOperationMetricSet& metric); + PersistenceOperationMetricSet& metric, + uint32_t trace_level = 0); // TODO remove default value private: [[nodiscard]] bool replica_set_changed_after_get_operation() const; diff --git a/storage/src/vespa/storage/distributor/operations/external/getoperation.cpp b/storage/src/vespa/storage/distributor/operations/external/getoperation.cpp index 72943eaf713..a261a898283 100644 --- a/storage/src/vespa/storage/distributor/operations/external/getoperation.cpp +++ b/storage/src/vespa/storage/distributor/operations/external/getoperation.cpp @@ -63,6 +63,7 @@ GetOperation::GetOperation(const DistributorNodeContext& node_ctx, _newest_replica(), _metric(metric), _operationTimer(node_ctx.clock()), + _trace(_msg->getTrace().getLevel()), _desired_read_consistency(desired_read_consistency), _has_replica_inconsistency(false), _any_replicas_failed(false) @@ -156,7 +157,7 @@ GetOperation::onReceive(DistributorStripeMessageSender& sender, const std::share LOG(debug, "Received %s", msg->toString(true).c_str()); - _msg->getTrace().addChild(getreply->steal_trace()); + _trace.addChild(getreply->steal_trace()); // TODO sweet, sweet nectar of distributed tracing bool allDone = true; for (auto& response : _responses) { for (uint32_t i = 0; i < response.second.size(); i++) { @@ -247,6 +248,10 @@ GetOperation::sendReply(DistributorStripeMessageSender& sender) const auto timestamp = (newest.is_tombstone ? api::Timestamp(0) : newest.timestamp); auto repl = std::make_shared<api::GetReply>(*_msg, _doc, timestamp, !_has_replica_inconsistency); repl->setResult(_returnCode); + if (!_trace.isEmpty()) { + _trace.setStrict(false); + repl->getTrace().addChild(std::move(_trace)); + } update_internal_metrics(); sender.sendReply(repl); _msg.reset(); diff --git a/storage/src/vespa/storage/distributor/operations/external/getoperation.h b/storage/src/vespa/storage/distributor/operations/external/getoperation.h index 9d6b9f1986d..a6de9370dcb 100644 --- a/storage/src/vespa/storage/distributor/operations/external/getoperation.h +++ b/storage/src/vespa/storage/distributor/operations/external/getoperation.h @@ -88,28 +88,25 @@ private: }; using GroupVector = std::vector<BucketChecksumGroup>; + using DbReplicaState = std::vector<std::pair<document::BucketId, uint16_t>>; // Organize the different copies by bucket/checksum pairs. We should // try to request GETs from each bucket and each different checksum // within that bucket. - std::map<GroupId, GroupVector> _responses; - - const DistributorNodeContext& _node_ctx; - const DistributorBucketSpace &_bucketSpace; - - std::shared_ptr<api::GetCommand> _msg; - - api::ReturnCode _returnCode; + std::map<GroupId, GroupVector> _responses; + const DistributorNodeContext& _node_ctx; + const DistributorBucketSpace& _bucketSpace; + std::shared_ptr<api::GetCommand> _msg; + api::ReturnCode _returnCode; std::shared_ptr<document::Document> _doc; - - std::optional<NewestReplica> _newest_replica; - - PersistenceOperationMetricSet& _metric; - framework::MilliSecTimer _operationTimer; - std::vector<std::pair<document::BucketId, uint16_t>> _replicas_in_db; - api::InternalReadConsistency _desired_read_consistency; - bool _has_replica_inconsistency; - bool _any_replicas_failed; + std::optional<NewestReplica> _newest_replica; + PersistenceOperationMetricSet& _metric; + framework::MilliSecTimer _operationTimer; + DbReplicaState _replicas_in_db; + vespalib::Trace _trace; + api::InternalReadConsistency _desired_read_consistency; + bool _has_replica_inconsistency; + bool _any_replicas_failed; void sendReply(DistributorStripeMessageSender& sender); bool sendForChecksum(DistributorStripeMessageSender& sender, const document::BucketId& id, GroupVector& res); diff --git a/storage/src/vespa/storage/distributor/operations/external/putoperation.cpp b/storage/src/vespa/storage/distributor/operations/external/putoperation.cpp index 96252c37844..952aeff0800 100644 --- a/storage/src/vespa/storage/distributor/operations/external/putoperation.cpp +++ b/storage/src/vespa/storage/distributor/operations/external/putoperation.cpp @@ -156,13 +156,13 @@ void PutOperation::start_conditional_put(DistributorStripeMessageSender& sender) document::Bucket bucket(_msg->getBucket().getBucketSpace(), _doc_id_bucket_id); _check_condition = CheckCondition::create_if_inconsistent_replicas(bucket, _bucket_space, _msg->getDocumentId(), _msg->getCondition(), _node_ctx, _op_ctx, - _temp_metric); + _temp_metric, _msg->getTrace().getLevel()); if (!_check_condition) { start_direct_put_dispatch(sender); } else { // Inconsistent replicas; need write repair _check_condition->start_and_send(sender); - const auto& outcome = _check_condition->maybe_outcome(); // Might be done immediately + auto& outcome = _check_condition->maybe_outcome(); // Might be done immediately if (outcome) { on_completed_check_condition(*outcome, sender); } @@ -265,7 +265,7 @@ PutOperation::onReceive(DistributorStripeMessageSender& sender, const std::share _tracker.receiveReply(sender, dynamic_cast<api::BucketInfoReply&>(*msg)); } else { _check_condition->handle_reply(sender, msg); - const auto& outcome = _check_condition->maybe_outcome(); + auto& outcome = _check_condition->maybe_outcome(); if (!outcome) { return; // Condition check not done yet } @@ -274,9 +274,12 @@ PutOperation::onReceive(DistributorStripeMessageSender& sender, const std::share } void -PutOperation::on_completed_check_condition(const CheckCondition::Outcome& outcome, +PutOperation::on_completed_check_condition(CheckCondition::Outcome& outcome, DistributorStripeMessageSender& sender) { + if (!outcome.trace().isEmpty()) { + _tracker.add_trace_tree_to_reply(outcome.steal_trace()); + } const bool effectively_matched = (outcome.matched_condition() || (outcome.not_found() && _msg->get_create_if_non_existent())); if (effectively_matched) { diff --git a/storage/src/vespa/storage/distributor/operations/external/putoperation.h b/storage/src/vespa/storage/distributor/operations/external/putoperation.h index d80e9cc00d7..6befb8d3e38 100644 --- a/storage/src/vespa/storage/distributor/operations/external/putoperation.h +++ b/storage/src/vespa/storage/distributor/operations/external/putoperation.h @@ -50,7 +50,7 @@ private: void start_direct_put_dispatch(DistributorStripeMessageSender& sender); void start_conditional_put(DistributorStripeMessageSender& sender); - void on_completed_check_condition(const CheckCondition::Outcome& outcome, + void on_completed_check_condition(CheckCondition::Outcome& outcome, DistributorStripeMessageSender& sender); void insertDatabaseEntryAndScheduleCreateBucket(const OperationTargetList& copies, bool setOneActive, const api::StorageCommand& originalCommand, diff --git a/storage/src/vespa/storage/distributor/persistencemessagetracker.cpp b/storage/src/vespa/storage/distributor/persistencemessagetracker.cpp index 08b99cf89a8..a30663bde2f 100644 --- a/storage/src/vespa/storage/distributor/persistencemessagetracker.cpp +++ b/storage/src/vespa/storage/distributor/persistencemessagetracker.cpp @@ -58,6 +58,7 @@ PersistenceMessageTrackerImpl::fail(MessageSender& sender, const api::ReturnCode if (_reply.get()) { _reply->setResult(result); updateMetrics(); + transfer_trace_state_to_reply(); sender.sendReply(_reply); _reply.reset(); } @@ -222,10 +223,7 @@ PersistenceMessageTrackerImpl::sendReply(MessageSender& sender) } updateMetrics(); - if ( ! _trace.isEmpty()) { - _trace.setStrict(false); - _reply->getTrace().addChild(std::move(_trace)); - } + transfer_trace_state_to_reply(); sender.sendReply(_reply); _reply = std::shared_ptr<api::BucketInfoReply>(); @@ -288,6 +286,15 @@ PersistenceMessageTrackerImpl::handlePersistenceReply( } void +PersistenceMessageTrackerImpl::transfer_trace_state_to_reply() +{ + if (!_trace.isEmpty()) { + _trace.setStrict(false); + _reply->getTrace().addChild(std::move(_trace)); + } +} + +void PersistenceMessageTrackerImpl::updateFromReply( MessageSender& sender, api::BucketInfoReply& reply, @@ -318,4 +325,10 @@ PersistenceMessageTrackerImpl::updateFromReply( } } +void +PersistenceMessageTrackerImpl::add_trace_tree_to_reply(vespalib::Trace trace) +{ + _trace.addChild(std::move(trace)); +} + } diff --git a/storage/src/vespa/storage/distributor/persistencemessagetracker.h b/storage/src/vespa/storage/distributor/persistencemessagetracker.h index ca330859259..923ecf45649 100644 --- a/storage/src/vespa/storage/distributor/persistencemessagetracker.h +++ b/storage/src/vespa/storage/distributor/persistencemessagetracker.h @@ -23,10 +23,12 @@ struct PersistenceMessageTracker { virtual void queueCommand(api::BucketCommand::SP, uint16_t target) = 0; virtual void flushQueue(MessageSender&) = 0; virtual uint16_t handleReply(api::BucketReply& reply) = 0; + virtual void add_trace_tree_to_reply(vespalib::Trace trace) = 0; }; -class PersistenceMessageTrackerImpl : public PersistenceMessageTracker, - public MessageTracker +class PersistenceMessageTrackerImpl final + : public PersistenceMessageTracker, + public MessageTracker { private: using BucketInfoMap = std::map<document::Bucket, std::vector<BucketCopy>>; @@ -92,12 +94,14 @@ private: void updateFailureResult(const api::BucketInfoReply& reply); void handleCreateBucketReply(api::BucketInfoReply& reply, uint16_t node); void handlePersistenceReply(api::BucketInfoReply& reply, uint16_t node); + void transfer_trace_state_to_reply(); void queueCommand(std::shared_ptr<api::BucketCommand> msg, uint16_t target) override { MessageTracker::queueCommand(std::move(msg), target); } void flushQueue(MessageSender& s) override { MessageTracker::flushQueue(s); } uint16_t handleReply(api::BucketReply& r) override { return MessageTracker::handleReply(r); } + void add_trace_tree_to_reply(vespalib::Trace trace) override; }; } diff --git a/streamingvisitors/src/vespa/searchvisitor/CMakeLists.txt b/streamingvisitors/src/vespa/searchvisitor/CMakeLists.txt index 06caf080923..4911b0693e4 100644 --- a/streamingvisitors/src/vespa/searchvisitor/CMakeLists.txt +++ b/streamingvisitors/src/vespa/searchvisitor/CMakeLists.txt @@ -1,6 +1,7 @@ # Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. vespa_add_library(streamingvisitors SOURCES + attribute_access_recorder.cpp hitcollector.cpp indexenvironment.cpp matching_elements_filler.cpp diff --git a/streamingvisitors/src/vespa/searchvisitor/attribute_access_recorder.cpp b/streamingvisitors/src/vespa/searchvisitor/attribute_access_recorder.cpp new file mode 100644 index 00000000000..9d520cde187 --- /dev/null +++ b/streamingvisitors/src/vespa/searchvisitor/attribute_access_recorder.cpp @@ -0,0 +1,67 @@ +// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. + +#include "attribute_access_recorder.h" +#include <vespa/vespalib/stllike/hash_set.hpp> + +using search::attribute::IAttributeVector; + +namespace streaming { + +AttributeAccessRecorder::AttributeAccessRecorder(std::unique_ptr<IAttributeContext> ctx) + : _ctx(std::move(ctx)), + _accessed_attributes() +{ +} + +AttributeAccessRecorder::~AttributeAccessRecorder() = default; + +void +AttributeAccessRecorder::asyncForAttribute(const vespalib::string& name, std::unique_ptr<search::attribute::IAttributeFunctor> func) const +{ + _ctx->asyncForAttribute(name, std::move(func)); +} + +const IAttributeVector* +AttributeAccessRecorder::getAttribute(const string& name) const +{ + auto ret = _ctx->getAttribute(name); + if (ret != nullptr) { + _accessed_attributes.insert(name); + } + return ret; +} + +const IAttributeVector* +AttributeAccessRecorder::getAttributeStableEnum(const string& name) const +{ + auto ret = _ctx->getAttributeStableEnum(name); + if (ret != nullptr) { + _accessed_attributes.insert(name); + } + return ret; +} + +void +AttributeAccessRecorder::getAttributeList(std::vector<const IAttributeVector*>& list) const +{ + _ctx->getAttributeList(list); +} + +void +AttributeAccessRecorder::releaseEnumGuards() +{ + _ctx->releaseEnumGuards(); +} + +std::vector<vespalib::string> +AttributeAccessRecorder::get_accessed_attributes() const +{ + std::vector<vespalib::string> result; + result.reserve(_accessed_attributes.size()); + for (auto& attr : _accessed_attributes) { + result.emplace_back(attr); + } + return result; +} + +} diff --git a/streamingvisitors/src/vespa/searchvisitor/attribute_access_recorder.h b/streamingvisitors/src/vespa/searchvisitor/attribute_access_recorder.h new file mode 100644 index 00000000000..233c507bda3 --- /dev/null +++ b/streamingvisitors/src/vespa/searchvisitor/attribute_access_recorder.h @@ -0,0 +1,30 @@ +// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. + +#pragma once + +#include <vespa/searchcommon/attribute/iattributecontext.h> +#include <vespa/vespalib/stllike/hash_set.h> + +namespace streaming { + +/* + * This class wraps an IAttributeContext and records accesses to attribute + * vectors. + */ +class AttributeAccessRecorder : public search::attribute::IAttributeContext +{ + std::unique_ptr<search::attribute::IAttributeContext> _ctx; + mutable vespalib::hash_set<vespalib::string> _accessed_attributes; + +public: + AttributeAccessRecorder(std::unique_ptr<IAttributeContext> ctx); + ~AttributeAccessRecorder() override; + void asyncForAttribute(const vespalib::string& name, std::unique_ptr<search::attribute::IAttributeFunctor> func) const override; + const search::attribute::IAttributeVector* getAttribute(const string& name) const override; + const search::attribute::IAttributeVector * getAttributeStableEnum(const string& name) const override; + void getAttributeList(std::vector<const search::attribute::IAttributeVector *>& list) const override; + void releaseEnumGuards() override; + std::vector<vespalib::string> get_accessed_attributes() const; +}; + +} diff --git a/streamingvisitors/src/vespa/searchvisitor/indexenvironment.cpp b/streamingvisitors/src/vespa/searchvisitor/indexenvironment.cpp index 1242195c9df..0e4c082dea3 100644 --- a/streamingvisitors/src/vespa/searchvisitor/indexenvironment.cpp +++ b/streamingvisitors/src/vespa/searchvisitor/indexenvironment.cpp @@ -13,8 +13,6 @@ IndexEnvironment::IndexEnvironment(const ITableManager & tableManager) : _fields(), _fieldNames(), _motivation(RANK), - _rankAttributes(), - _dumpAttributes(), _ranking_assets_repo() { } @@ -41,15 +39,7 @@ IndexEnvironment::addField(const vespalib::string& name, } void -IndexEnvironment::hintAttributeAccess(const string & name) const { - if (name.empty()) { - return; - } - if (_motivation == RANK) { - _rankAttributes.insert(name); - } else { - _dumpAttributes.insert(name); - } +IndexEnvironment::hintAttributeAccess(const string &) const { } void diff --git a/streamingvisitors/src/vespa/searchvisitor/indexenvironment.h b/streamingvisitors/src/vespa/searchvisitor/indexenvironment.h index 88ea5a5ada8..f741af77e35 100644 --- a/streamingvisitors/src/vespa/searchvisitor/indexenvironment.h +++ b/streamingvisitors/src/vespa/searchvisitor/indexenvironment.h @@ -29,8 +29,6 @@ private: std::vector<search::fef::FieldInfo> _fields; StringInt32Map _fieldNames; mutable FeatureMotivation _motivation; - mutable std::set<vespalib::string> _rankAttributes; - mutable std::set<vespalib::string> _dumpAttributes; std::shared_ptr<const search::fef::IRankingAssetsRepo> _ranking_assets_repo; public: @@ -88,10 +86,6 @@ public: void set_ranking_assets_repo(std::shared_ptr<const search::fef::IRankingAssetsRepo> ranking_assets_repo); - const std::set<vespalib::string> & getHintedRankAttributes() const { return _rankAttributes; } - - const std::set<vespalib::string> & getHintedDumpAttributes() const { return _dumpAttributes; } - //TODO Wire in proper distribution key uint32_t getDistributionKey() const override { return 0; } diff --git a/streamingvisitors/src/vespa/searchvisitor/queryenvironment.cpp b/streamingvisitors/src/vespa/searchvisitor/queryenvironment.cpp index 0765074e315..306f7f5d655 100644 --- a/streamingvisitors/src/vespa/searchvisitor/queryenvironment.cpp +++ b/streamingvisitors/src/vespa/searchvisitor/queryenvironment.cpp @@ -46,7 +46,7 @@ QueryEnvironment::QueryEnvironment(const string & location_str, const IAttributeManager * attrMgr) : _indexEnv(indexEnv), _properties(properties), - _attrCtx(attrMgr->createContext()), + _attrCtx(std::make_unique<AttributeAccessRecorder>(attrMgr->createContext())), _queryTerms(), _locations(parseLocation(location_str)) { diff --git a/streamingvisitors/src/vespa/searchvisitor/queryenvironment.h b/streamingvisitors/src/vespa/searchvisitor/queryenvironment.h index dd543a60244..c5dc442e424 100644 --- a/streamingvisitors/src/vespa/searchvisitor/queryenvironment.h +++ b/streamingvisitors/src/vespa/searchvisitor/queryenvironment.h @@ -2,12 +2,12 @@ #pragma once -#include <vespa/searchcommon/attribute/iattributecontext.h> +#include "attribute_access_recorder.h" +#include "indexenvironment.h" #include <vespa/searchlib/attribute/iattributemanager.h> #include <vespa/searchlib/fef/iindexenvironment.h> #include <vespa/searchlib/fef/iqueryenvironment.h> #include <vespa/searchlib/fef/properties.h> -#include "indexenvironment.h" namespace streaming { @@ -20,7 +20,7 @@ class QueryEnvironment : public search::fef::IQueryEnvironment private: const IndexEnvironment &_indexEnv; const search::fef::Properties &_properties; - search::attribute::IAttributeContext::UP _attrCtx; + std::unique_ptr<AttributeAccessRecorder> _attrCtx; std::vector<const search::fef::ITermData *> _queryTerms; std::vector<search::common::GeoLocationSpec> _locations; @@ -61,6 +61,8 @@ public: virtual const search::fef::IIndexEnvironment & getIndexEnvironment() const override { return _indexEnv; } void addTerm(const search::fef::ITermData *term) { _queryTerms.push_back(term); } + + std::vector<vespalib::string> get_accessed_attributes() const { return _attrCtx->get_accessed_attributes(); } }; } // namespace streaming diff --git a/streamingvisitors/src/vespa/searchvisitor/rankprocessor.h b/streamingvisitors/src/vespa/searchvisitor/rankprocessor.h index 443a2626bf7..4bac204c8e1 100644 --- a/streamingvisitors/src/vespa/searchvisitor/rankprocessor.h +++ b/streamingvisitors/src/vespa/searchvisitor/rankprocessor.h @@ -78,6 +78,7 @@ public: HitCollector & getHitCollector() { return *_hitCollector; } uint32_t getDocId() const { return _docId; } search::fef::IQueryEnvironment& get_query_env() { return _queryEnv; } + QueryEnvironment& get_real_query_env() { return _queryEnv; } }; } // namespace streaming diff --git a/streamingvisitors/src/vespa/searchvisitor/searchvisitor.cpp b/streamingvisitors/src/vespa/searchvisitor/searchvisitor.cpp index f9473167b07..c53dfae294a 100644 --- a/streamingvisitors/src/vespa/searchvisitor/searchvisitor.cpp +++ b/streamingvisitors/src/vespa/searchvisitor/searchvisitor.cpp @@ -548,11 +548,12 @@ SearchVisitor::PositionInserter::onStructStart(const Content & c) } void -SearchVisitor::RankController::processHintedAttributes(const IndexEnvironment & indexEnv, bool rank, - const search::IAttributeManager & attrMan, - std::vector<AttrInfo> & attributeFields) +SearchVisitor::RankController::processAccessedAttributes(const QueryEnvironment &queryEnv, bool rank, + const search::IAttributeManager & attrMan, + std::vector<AttrInfo> & attributeFields) { - const std::set<vespalib::string> & attributes = (rank ? indexEnv.getHintedRankAttributes() : indexEnv.getHintedDumpAttributes()); + auto attributes = queryEnv.get_accessed_attributes(); + auto& indexEnv = queryEnv.getIndexEnvironment(); for (const vespalib::string & name : attributes) { LOG(debug, "Process attribute access hint (%s): '%s'", rank ? "rank" : "dump", name.c_str()); const search::fef::FieldInfo * fieldInfo = indexEnv.getFieldByName(name); @@ -601,22 +602,18 @@ SearchVisitor::RankController::setupRankProcessors(Query & query, std::vector<AttrInfo> & attributeFields) { _rankSetup = &_rankManagerSnapshot->getRankSetup(_rankProfile); - - // register attribute vectors needed for ranking - const IndexEnvironment & indexEnv = _rankManagerSnapshot->getIndexEnvironment(_rankProfile); - processHintedAttributes(indexEnv, true, attrMan, attributeFields); - _rankProcessor = std::make_unique<RankProcessor>(_rankManagerSnapshot, _rankProfile, query, location, _queryProperties, &attrMan); LOG(debug, "Initialize rank processor"); _rankProcessor->initForRanking(wantedHitCount); + // register attribute vectors needed for ranking + processAccessedAttributes(_rankProcessor->get_real_query_env(), true, attrMan, attributeFields); if (_dumpFeatures) { - // register attribute vectors needed for dumping - processHintedAttributes(indexEnv, false, attrMan, attributeFields); - _dumpProcessor = std::make_unique<RankProcessor>(_rankManagerSnapshot, _rankProfile, query, location, _queryProperties, &attrMan); LOG(debug, "Initialize dump processor"); _dumpProcessor->initForDumping(wantedHitCount); + // register attribute vectors needed for dumping + processAccessedAttributes(_dumpProcessor->get_real_query_env(), false, attrMan, attributeFields); } _hasRanking = true; diff --git a/streamingvisitors/src/vespa/searchvisitor/searchvisitor.h b/streamingvisitors/src/vespa/searchvisitor/searchvisitor.h index 72cd08ff781..515d032b21b 100644 --- a/streamingvisitors/src/vespa/searchvisitor/searchvisitor.h +++ b/streamingvisitors/src/vespa/searchvisitor/searchvisitor.h @@ -133,11 +133,13 @@ private: RankProcessor::UP _dumpProcessor; /** - * Process attribute hints and add needed attributes to the given list. + * Process attribute accessed and add needed attributes to the + * given list. **/ - static void processHintedAttributes(const IndexEnvironment & indexEnv, bool rank, - const search::IAttributeManager & attrMan, - std::vector<AttrInfo> & attributeFields); + static void processAccessedAttributes(const QueryEnvironment& queryEnv, + bool rank, + const search::IAttributeManager& attrMan, + std::vector<AttrInfo>& attributeFields); public: RankController(); diff --git a/vespa-athenz/src/main/java/com/yahoo/vespa/athenz/zpe/DefaultZpe.java b/vespa-athenz/src/main/java/com/yahoo/vespa/athenz/zpe/DefaultZpe.java index 5bf25e3e6be..b58c3e9c721 100644 --- a/vespa-athenz/src/main/java/com/yahoo/vespa/athenz/zpe/DefaultZpe.java +++ b/vespa-athenz/src/main/java/com/yahoo/vespa/athenz/zpe/DefaultZpe.java @@ -10,6 +10,8 @@ import com.yahoo.vespa.athenz.api.ZToken; import com.yahoo.vespa.athenz.zpe.AuthorizationResult.Type; import java.security.cert.X509Certificate; +import java.util.logging.Level; +import java.util.logging.Logger; /** * The default implementation of {@link Zpe}. @@ -19,6 +21,13 @@ import java.security.cert.X509Certificate; */ public class DefaultZpe implements Zpe { + // Disable log spam from ZPE library + private static final Logger fpksLogger = + Logger.getLogger(com.yahoo.athenz.zpe.pkey.file.FilePublicKeyStore.class.getName()); + static { + fpksLogger.setLevel(Level.SEVERE); + } + public DefaultZpe() { AuthZpeClient.init(); // Disable access token cert offset validation to allow existing tokens with refreshed certs |