diff options
11 files changed, 92 insertions, 89 deletions
diff --git a/searchcore/src/tests/proton/reference/gid_to_lid_change_listener/gid_to_lid_change_listener_test.cpp b/searchcore/src/tests/proton/reference/gid_to_lid_change_listener/gid_to_lid_change_listener_test.cpp index 08787e41438..4c6f9017000 100644 --- a/searchcore/src/tests/proton/reference/gid_to_lid_change_listener/gid_to_lid_change_listener_test.cpp +++ b/searchcore/src/tests/proton/reference/gid_to_lid_change_listener/gid_to_lid_change_listener_test.cpp @@ -7,6 +7,7 @@ #include <vespa/searchcore/proton/reference/gid_to_lid_change_listener.h> #include <vespa/searchlib/common/i_gid_to_lid_mapper_factory.h> #include <vespa/searchlib/common/i_gid_to_lid_mapper.h> +#include <vespa/searchlib/test/mock_gid_to_lid_mapping.h> #include <map> #include <vespa/log/log.h> LOG_SETUP("gid_to_lid_change_listener_test"); @@ -19,6 +20,7 @@ using search::attribute::Config; using search::attribute::BasicType; using search::attribute::Reference; using search::attribute::ReferenceAttribute; +using search::attribute::test::MockGidToLidMapperFactory; namespace proton { @@ -32,39 +34,14 @@ vespalib::string doc1("id:test:music::1"); vespalib::string doc2("id:test:music::2"); vespalib::string doc3("id:test:music::3"); -using MockGidToLidMap = std::map<GlobalId, uint32_t>; - -struct MyGidToLidMapper : public search::IGidToLidMapper +struct MyGidToLidMapperFactory : public MockGidToLidMapperFactory { - const MockGidToLidMap &_map; - MyGidToLidMapper(const MockGidToLidMap &map) - : _map(map) - { - } - virtual uint32_t mapGidToLid(const document::GlobalId &gid) const override { - auto itr = _map.find(gid); - if (itr != _map.end()) { - return itr->second; - } else { - return 0u; - } - } -}; - -struct MyGidToLidMapperFactory : public search::IGidToLidMapperFactory -{ - MockGidToLidMap _map; - MyGidToLidMapperFactory() - : _map() + : MockGidToLidMapperFactory() { _map.insert({toGid(doc1), 10}); _map.insert({toGid(doc2), 17}); } - - virtual std::unique_ptr<search::IGidToLidMapper> getMapper() const override { - return std::make_unique<MyGidToLidMapper>(_map); - } }; } diff --git a/searchcore/src/vespa/searchcore/proton/reference/gid_to_lid_mapper.cpp b/searchcore/src/vespa/searchcore/proton/reference/gid_to_lid_mapper.cpp index 7364e6ee6c0..db990dff3bc 100644 --- a/searchcore/src/vespa/searchcore/proton/reference/gid_to_lid_mapper.cpp +++ b/searchcore/src/vespa/searchcore/proton/reference/gid_to_lid_mapper.cpp @@ -1,11 +1,12 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. #include "gid_to_lid_mapper.h" +#include <vespa/searchcore/proton/documentmetastore/documentmetastore.h> namespace proton { GidToLidMapper::GidToLidMapper(vespalib::GenerationHandler::Guard &&guard, - const search::IDocumentMetaStore &dms) + const DocumentMetaStore &dms) : _guard(std::move(guard)), _dms(dms) { @@ -26,4 +27,13 @@ GidToLidMapper::mapGidToLid(const document::GlobalId &gid) const } } +void +GidToLidMapper::foreach(const search::IGidToLidMapperVisitor &visitor) const +{ + const auto &dms = _dms; + dms.beginFrozen().foreach_key([&dms,&visitor](uint32_t lid) + { visitor.visit(dms.getRawMetaData(lid).getGid(), lid); }); +} + + } // namespace proton diff --git a/searchcore/src/vespa/searchcore/proton/reference/gid_to_lid_mapper.h b/searchcore/src/vespa/searchcore/proton/reference/gid_to_lid_mapper.h index 7c5dc840af5..ed6bf91dbd3 100644 --- a/searchcore/src/vespa/searchcore/proton/reference/gid_to_lid_mapper.h +++ b/searchcore/src/vespa/searchcore/proton/reference/gid_to_lid_mapper.h @@ -4,10 +4,11 @@ #include <vespa/searchlib/common/i_gid_to_lid_mapper.h> #include <vespa/vespalib/util/generationhandler.h> -#include <vespa/searchlib/common/idocumentmetastore.h> namespace proton { +class DocumentMetaStore; + /* * Class for mapping from gid to lid. Instances should be short lived * due to read guards preventing resource reuse. @@ -15,12 +16,13 @@ namespace proton { class GidToLidMapper : public search::IGidToLidMapper { vespalib::GenerationHandler::Guard _guard; - const search::IDocumentMetaStore &_dms; + const DocumentMetaStore &_dms; public: GidToLidMapper(vespalib::GenerationHandler::Guard &&guard, - const search::IDocumentMetaStore &dms); + const DocumentMetaStore &dms); virtual ~GidToLidMapper(); virtual uint32_t mapGidToLid(const document::GlobalId &gid) const override; + virtual void foreach(const search::IGidToLidMapperVisitor &visitor) const override; }; } // namespace proton diff --git a/searchlib/src/tests/attribute/reference_attribute/reference_attribute_test.cpp b/searchlib/src/tests/attribute/reference_attribute/reference_attribute_test.cpp index 1b1bc1f8796..480b58d672d 100644 --- a/searchlib/src/tests/attribute/reference_attribute/reference_attribute_test.cpp +++ b/searchlib/src/tests/attribute/reference_attribute/reference_attribute_test.cpp @@ -188,7 +188,8 @@ struct Fixture void notifyReferencedRemove(const GlobalId &gid) { _attr->notifyReferencedRemove(gid); } - void populateReferencedLids() { + void setGidToLidMapperFactory(std::shared_ptr<MyGidToLidMapperFactory> factory) { + _attr->setGidToLidMapperFactory(factory); _attr->populateReferencedLids(); } }; @@ -289,8 +290,8 @@ TEST_F("require that we can save and load attribute", Fixture) TEST_F("require that update() uses gid-mapper to set referenced lid", Fixture) { f.ensureDocIdLimit(6); - std::shared_ptr<search::IGidToLidMapperFactory> factory = std::make_shared<MyGidToLidMapperFactory>(); - f._attr->setGidToLidMapperFactory(factory); + auto factory = std::make_shared<MyGidToLidMapperFactory>(); + f.setGidToLidMapperFactory(factory); f.set(1, toGid(doc1)); f.set(2, toGid(doc2)); f.set(4, toGid(doc1)); @@ -340,9 +341,8 @@ void preparePopulateReferencedLids(Fixture &f) void checkPopulateReferencedLids(Fixture &f) { - std::shared_ptr<search::IGidToLidMapperFactory> factory = std::make_shared<MyGidToLidMapperFactory>(); - f._attr->setGidToLidMapperFactory(factory); - f.populateReferencedLids(); + auto factory = std::make_shared<MyGidToLidMapperFactory>(); + f.setGidToLidMapperFactory(factory); TEST_DO(f.assertRefLid(1, 10)); TEST_DO(f.assertRefLid(2, 17)); TEST_DO(f.assertRefLid(3, 10)); @@ -388,33 +388,4 @@ TEST_F("Require that notifyReferencedPut and notifyReferencedRemove changes reve TEST_DO(f.assertLids(11, { })); } -TEST_F("Require that reverse mapping recovers from temporary out of order glitch", Fixture) -{ - auto factory = std::make_shared<MyGidToLidMapperFactory>(); - f._attr->setGidToLidMapperFactory(factory); - f.ensureDocIdLimit(4); - f.set(1, toGid(doc1)); - f.set(2, toGid(doc2)); - /* - * Changes in gid to lid mapping can be visible via gid to lid - * mapper before notifications arrive. If a lid is reused in the - * referenced document meta store then multiple entries in - * reference store might temporarily map to the same referenced - * lid. - */ - factory->remove(doc1); // remove referenced document - factory->add(doc3, 10); // reuse lid for new referenced document - f.set(3, toGid(doc3)); - TEST_DO(f.assertRefLid(1, 10)); - TEST_DO(f.assertRefLid(2, 17)); - TEST_DO(f.assertRefLid(3, 10)); - // Notify reference attribute about gid to lid mapping changes - f.notifyReferencedRemove(toGid(doc1)); - f.notifyReferencedPut(toGid(doc3), 10); - TEST_DO(f.assertRefLid(1, 0)); - TEST_DO(f.assertRefLid(2, 17)); - TEST_DO(f.assertRefLid(3, 10)); - TEST_DO(f.assertLids(10, { 3 })); -} - TEST_MAIN() { TEST_RUN_ALL(); } diff --git a/searchlib/src/vespa/searchlib/attribute/reference_attribute.cpp b/searchlib/src/vespa/searchlib/attribute/reference_attribute.cpp index 2cb05b14503..8666f0bdced 100644 --- a/searchlib/src/vespa/searchlib/attribute/reference_attribute.cpp +++ b/searchlib/src/vespa/searchlib/attribute/reference_attribute.cpp @@ -247,9 +247,6 @@ ReferenceAttribute::update(DocId doc, const GlobalId &gid) assert(doc < _indices.size()); EntryRef oldRef = _indices[doc]; Reference refToAdd(gid); - if (_gidToLidMapperFactory) { - refToAdd.setLid(_gidToLidMapperFactory->getMapper()->mapGidToLid(gid)); - } EntryRef newRef = _store.add(refToAdd).ref(); std::atomic_thread_fence(std::memory_order_release); _indices[doc] = newRef; @@ -320,14 +317,23 @@ ReferenceAttribute::setGidToLidMapperFactory(std::shared_ptr<IGidToLidMapperFact } void -ReferenceAttribute::notifyReferencedPut(const GlobalId &gid, DocId referencedLid) +ReferenceAttribute::notifyReferencedPutNoCommit(const GlobalId &gid, DocId referencedLid) { + assert(referencedLid != 0); EntryRef ref = _store.find(gid); - if (ref.valid()) { - const auto &entry = _store.get(ref); - _referenceMappings.notifyReferencedPut(entry, referencedLid); - commit(); + if (!ref.valid() || _store.get(ref).lid() == 0) { + Reference refToAdd(gid); + ref = _store.add(refToAdd).ref(); } + const auto &entry = _store.get(ref); + _referenceMappings.notifyReferencedPut(entry, referencedLid); +} + +void +ReferenceAttribute::notifyReferencedPut(const GlobalId &gid, DocId referencedLid) +{ + notifyReferencedPutNoCommit(gid, referencedLid); + commit(); } void @@ -336,24 +342,42 @@ ReferenceAttribute::notifyReferencedRemove(const GlobalId &gid) EntryRef ref = _store.find(gid); if (ref.valid()) { const auto &entry = _store.get(ref); + uint32_t oldReferencedLid = entry.lid(); _referenceMappings.notifyReferencedRemove(entry); + if (oldReferencedLid != 0) { + _store.remove(ref); + } commit(); } } +namespace { + +class ReferencedLidPopulator : public IGidToLidMapperVisitor +{ + ReferenceAttribute &_attr; +public: + ReferencedLidPopulator(ReferenceAttribute &attr) + : IGidToLidMapperVisitor(), + _attr(attr) + { + } + virtual ~ReferencedLidPopulator() override { } + virtual void visit(const document::GlobalId &gid, uint32_t lid) const override { + _attr.notifyReferencedPutNoCommit(gid, lid); + } +}; + +} + void ReferenceAttribute::populateReferencedLids() { if (_gidToLidMapperFactory) { std::unique_ptr<IGidToLidMapper> mapperUP = _gidToLidMapperFactory->getMapper(); const IGidToLidMapper &mapper = *mapperUP; - const auto &store = _store; - const auto saver = _store.getSaver(); - saver.foreach_key([&store,&mapper,this](EntryRef ref) - { const Reference &entry = store.get(ref); - entry.setLid(mapper.mapGidToLid(entry.gid())); - _referenceMappings.syncMappings(entry); - }); + ReferencedLidPopulator populator(*this); + mapper.foreach(populator); } commit(); } diff --git a/searchlib/src/vespa/searchlib/attribute/reference_attribute.h b/searchlib/src/vespa/searchlib/attribute/reference_attribute.h index ce7c908db99..7387c13fc50 100644 --- a/searchlib/src/vespa/searchlib/attribute/reference_attribute.h +++ b/searchlib/src/vespa/searchlib/attribute/reference_attribute.h @@ -78,6 +78,7 @@ public: ReverseMappingRefs getReverseMappingRefs() const { return _referenceMappings.getReverseMappingRefs(); } const ReverseMapping &getReverseMapping() const { return _referenceMappings.getReverseMapping(); } + void notifyReferencedPutNoCommit(const GlobalId &gid, DocId referencedLid); void notifyReferencedPut(const GlobalId &gid, DocId referencedLid); void notifyReferencedRemove(const GlobalId &gid); void populateReferencedLids(); diff --git a/searchlib/src/vespa/searchlib/attribute/reference_mappings.cpp b/searchlib/src/vespa/searchlib/attribute/reference_mappings.cpp index f2462cdc40d..ae93dfba225 100644 --- a/searchlib/src/vespa/searchlib/attribute/reference_mappings.cpp +++ b/searchlib/src/vespa/searchlib/attribute/reference_mappings.cpp @@ -54,13 +54,6 @@ ReferenceMappings::syncReverseMappingIndices(const Reference &entry) } void -ReferenceMappings::syncMappings(const Reference &entry) -{ - syncReverseMappingIndices(entry); - syncForwardMapping(entry); -} - -void ReferenceMappings::removeReverseMapping(const Reference &entry, uint32_t lid) { EntryRef revMapIdx = entry.revMapIdx(); diff --git a/searchlib/src/vespa/searchlib/attribute/reference_mappings.h b/searchlib/src/vespa/searchlib/attribute/reference_mappings.h index 73754d9cb13..43ae8a5bdb8 100644 --- a/searchlib/src/vespa/searchlib/attribute/reference_mappings.h +++ b/searchlib/src/vespa/searchlib/attribute/reference_mappings.h @@ -61,7 +61,6 @@ public: void notifyReferencedRemove(const Reference &entry); void removeReverseMapping(const Reference &entry, uint32_t lid); void addReverseMapping(const Reference &entry, uint32_t lid); - void syncMappings(const Reference &entry); // Maintain size of mapping from lid to referenced lid void onAddDocs(uint32_t docIdLimit); diff --git a/searchlib/src/vespa/searchlib/common/i_gid_to_lid_mapper.h b/searchlib/src/vespa/searchlib/common/i_gid_to_lid_mapper.h index f5317035a21..8a9e2dbd84f 100644 --- a/searchlib/src/vespa/searchlib/common/i_gid_to_lid_mapper.h +++ b/searchlib/src/vespa/searchlib/common/i_gid_to_lid_mapper.h @@ -3,12 +3,24 @@ #pragma once #include <cstdint> +#include <functional> namespace document { class GlobalId; } namespace search { /* + * Interface for gid to lid mapper visitor. + */ +class IGidToLidMapperVisitor +{ +public: + virtual ~IGidToLidMapperVisitor() { } + virtual void visit(const document::GlobalId &gid, uint32_t lid) const = 0; +}; + + +/* * Interface class for mapping from gid to lid. Instances should be short * lived due to implementations containing read guards preventing resource * reuse. @@ -18,6 +30,7 @@ class IGidToLidMapper public: virtual ~IGidToLidMapper() { } virtual uint32_t mapGidToLid(const document::GlobalId &gid) const = 0; + virtual void foreach(const IGidToLidMapperVisitor &visitor) const = 0; }; } // namespace search diff --git a/searchlib/src/vespa/searchlib/test/imported_attribute_fixture.h b/searchlib/src/vespa/searchlib/test/imported_attribute_fixture.h index edb1a789284..85a2b140039 100644 --- a/searchlib/src/vespa/searchlib/test/imported_attribute_fixture.h +++ b/searchlib/src/vespa/searchlib/test/imported_attribute_fixture.h @@ -101,6 +101,11 @@ struct ImportedAttributeFixture { void map_reference(DocId from_lid, GlobalId via_gid, DocId to_lid) { assert(from_lid < reference_attr->getNumDocs()); mapper_factory->_map[via_gid] = to_lid; + if (to_lid != 0) { + reference_attr->notifyReferencedPut(via_gid, to_lid); + } else { + reference_attr->notifyReferencedRemove(via_gid); + } reference_attr->update(from_lid, via_gid); reference_attr->commit(); } diff --git a/searchlib/src/vespa/searchlib/test/mock_gid_to_lid_mapping.h b/searchlib/src/vespa/searchlib/test/mock_gid_to_lid_mapping.h index c2ffc2c4dfd..86203c19970 100644 --- a/searchlib/src/vespa/searchlib/test/mock_gid_to_lid_mapping.h +++ b/searchlib/src/vespa/searchlib/test/mock_gid_to_lid_mapping.h @@ -29,6 +29,14 @@ struct MockGidToLidMapper : public search::IGidToLidMapper { return 0u; } } + + void foreach(const search::IGidToLidMapperVisitor &visitor) const override { + for (const auto &kv : _map) { + if (kv.second != 0) { + visitor.visit(kv.first, kv.second); + } + } + } }; struct MockGidToLidMapperFactory : public search::IGidToLidMapperFactory { |