From 36fd5eef8d36d1bc2ba5b6f020b32f7dc28937fa Mon Sep 17 00:00:00 2001 From: Tor Egge Date: Thu, 23 Feb 2017 10:12:52 +0000 Subject: Fix comments. Break long lines. Rename reference attribute method to better match semantics. Add more test cases. Rename unit test class name and simplify implementation. --- .../gid_to_lid_change_handler_test.cpp | 120 +++++++++++---------- .../gid_to_lid_change_listener_test.cpp | 2 +- .../gid_to_lid_change_registrator_test.cpp | 4 + .../proton/reference/gid_to_lid_change_handler.cpp | 15 ++- .../proton/reference/gid_to_lid_change_handler.h | 6 +- .../reference/gid_to_lid_change_listener.cpp | 2 +- 6 files changed, 89 insertions(+), 60 deletions(-) (limited to 'searchcore') diff --git a/searchcore/src/tests/proton/reference/gid_to_lid_change_handler/gid_to_lid_change_handler_test.cpp b/searchcore/src/tests/proton/reference/gid_to_lid_change_handler/gid_to_lid_change_handler_test.cpp index 705197630a5..4d5bbd52a14 100644 --- a/searchcore/src/tests/proton/reference/gid_to_lid_change_handler/gid_to_lid_change_handler_test.cpp +++ b/searchcore/src/tests/proton/reference/gid_to_lid_change_handler/gid_to_lid_change_handler_test.cpp @@ -27,32 +27,32 @@ vespalib::string doc1("id:test:music::1"); } -class MyTarget { +class ListenerStats { using lock_guard = std::lock_guard; std::mutex _lock; - std::vector> _changes; + uint32_t _changes; uint32_t _createdListeners; uint32_t _registeredListeners; uint32_t _destroyedListeners; public: - MyTarget() + ListenerStats() : _lock(), - _changes(), + _changes(0u), _createdListeners(0u), _registeredListeners(0u), _destroyedListeners(0u) { } - ~MyTarget() + ~ListenerStats() { EXPECT_EQUAL(_createdListeners, _destroyedListeners); } - void notifyGidToLidChange(GlobalId gid, uint32_t lid) { + void notifyGidToLidChange() { lock_guard guard(_lock); - _changes.emplace_back(gid, lid); + ++_changes; } void markCreatedListener() { lock_guard guard(_lock); ++_createdListeners; } void markRegisteredListener() { lock_guard guard(_lock); ++_registeredListeners; } @@ -61,7 +61,6 @@ public: uint32_t getCreatedListeners() const { return _createdListeners; } uint32_t getRegisteredListeners() const { return _registeredListeners; } uint32_t getDestroyedListeners() const { return _destroyedListeners; } - const std::vector> &getChanges() const { return _changes; } void assertCounts(uint32_t expCreatedListeners, uint32_t expRegisteredListeners, @@ -71,29 +70,29 @@ public: EXPECT_EQUAL(expCreatedListeners, getCreatedListeners()); EXPECT_EQUAL(expRegisteredListeners, getRegisteredListeners()); EXPECT_EQUAL(expDestroyedListeners, getDestroyedListeners()); - EXPECT_EQUAL(expChanges, _changes.size()); + EXPECT_EQUAL(expChanges, _changes); } }; class MyListener : public IGidToLidChangeListener { - MyTarget &_target; + ListenerStats &_stats; vespalib::string _name; vespalib::string _docTypeName; public: - MyListener(MyTarget &target, + MyListener(ListenerStats &stats, const vespalib::string &name, const vespalib::string &docTypeName) : IGidToLidChangeListener(), - _target(target), + _stats(stats), _name(name), _docTypeName(docTypeName) { - _target.markCreatedListener(); + _stats.markCreatedListener(); } - virtual ~MyListener() { _target.markDestroyedListener(); } - virtual void notifyGidToLidChange(GlobalId gid, uint32_t lid) override { _target.notifyGidToLidChange(gid, lid); } - virtual void notifyRegistered() override { _target.markRegisteredListener(); } + virtual ~MyListener() { _stats.markDestroyedListener(); } + virtual void notifyGidToLidChange(GlobalId, uint32_t) override { _stats.notifyGidToLidChange(); } + virtual void notifyRegistered() override { _stats.markRegisteredListener(); } virtual const vespalib::string &getName() const override { return _name; } virtual const vespalib::string &getDocTypeName() const override { return _docTypeName; } }; @@ -102,13 +101,13 @@ struct Fixture { vespalib::ThreadStackExecutor _masterExecutor; ExecutorThreadService _master; - std::vector> _targets; + std::vector> _statss; std::shared_ptr _handler; Fixture() : _masterExecutor(1, 128 * 1024), _master(_masterExecutor), - _targets(), + _statss(), _handler(std::make_shared(&_master)) { } @@ -124,9 +123,9 @@ struct Fixture _master.sync(); } - MyTarget &addTarget() { - _targets.push_back(std::make_shared()); - return *_targets.back(); + ListenerStats &addStats() { + _statss.push_back(std::make_shared()); + return *_statss.back(); } void addListener(std::unique_ptr listener) { @@ -149,54 +148,67 @@ struct Fixture TEST_F("Test that we can register a listener", Fixture) { - auto &target = f.addTarget(); - auto listener = std::make_unique(target, "test", "testdoc"); - TEST_DO(target.assertCounts(1, 0, 0, 0)); + auto &stats = f.addStats(); + auto listener = std::make_unique(stats, "test", "testdoc"); + TEST_DO(stats.assertCounts(1, 0, 0, 0)); f.addListener(std::move(listener)); - TEST_DO(target.assertCounts(1, 1, 0, 0)); + TEST_DO(stats.assertCounts(1, 1, 0, 0)); f.notifyGidToLidChange(toGid(doc1), 10); - TEST_DO(target.assertCounts(1, 1, 0, 1)); + TEST_DO(stats.assertCounts(1, 1, 0, 1)); f.removeListeners("testdoc", {}); - TEST_DO(target.assertCounts(1, 1, 1, 1)); + TEST_DO(stats.assertCounts(1, 1, 1, 1)); } TEST_F("Test that we can register multiple listeners", Fixture) { - auto &target1 = f.addTarget(); - auto &target2 = f.addTarget(); - auto &target3 = f.addTarget(); - auto listener1 = std::make_unique(target1, "test1", "testdoc"); - auto listener2 = std::make_unique(target2, "test2", "testdoc"); - auto listener3 = std::make_unique(target3, "test3", "testdoc2"); - TEST_DO(target1.assertCounts(1, 0, 0, 0)); - TEST_DO(target2.assertCounts(1, 0, 0, 0)); - TEST_DO(target3.assertCounts(1, 0, 0, 0)); + auto &stats1 = f.addStats(); + auto &stats2 = f.addStats(); + auto &stats3 = f.addStats(); + auto listener1 = std::make_unique(stats1, "test1", "testdoc"); + auto listener2 = std::make_unique(stats2, "test2", "testdoc"); + auto listener3 = std::make_unique(stats3, "test3", "testdoc2"); + TEST_DO(stats1.assertCounts(1, 0, 0, 0)); + TEST_DO(stats2.assertCounts(1, 0, 0, 0)); + TEST_DO(stats3.assertCounts(1, 0, 0, 0)); f.addListener(std::move(listener1)); f.addListener(std::move(listener2)); f.addListener(std::move(listener3)); - TEST_DO(target1.assertCounts(1, 1, 0, 0)); - TEST_DO(target2.assertCounts(1, 1, 0, 0)); - TEST_DO(target3.assertCounts(1, 1, 0, 0)); + TEST_DO(stats1.assertCounts(1, 1, 0, 0)); + TEST_DO(stats2.assertCounts(1, 1, 0, 0)); + TEST_DO(stats3.assertCounts(1, 1, 0, 0)); f.notifyGidToLidChange(toGid(doc1), 10); - TEST_DO(target1.assertCounts(1, 1, 0, 1)); - TEST_DO(target2.assertCounts(1, 1, 0, 1)); - TEST_DO(target3.assertCounts(1, 1, 0, 1)); + TEST_DO(stats1.assertCounts(1, 1, 0, 1)); + TEST_DO(stats2.assertCounts(1, 1, 0, 1)); + TEST_DO(stats3.assertCounts(1, 1, 0, 1)); f.removeListeners("testdoc", {"test1"}); - TEST_DO(target1.assertCounts(1, 1, 0, 1)); - TEST_DO(target2.assertCounts(1, 1, 1, 1)); - TEST_DO(target3.assertCounts(1, 1, 0, 1)); + TEST_DO(stats1.assertCounts(1, 1, 0, 1)); + TEST_DO(stats2.assertCounts(1, 1, 1, 1)); + TEST_DO(stats3.assertCounts(1, 1, 0, 1)); f.removeListeners("testdoc", {}); - TEST_DO(target1.assertCounts(1, 1, 1, 1)); - TEST_DO(target2.assertCounts(1, 1, 1, 1)); - TEST_DO(target3.assertCounts(1, 1, 0, 1)); + TEST_DO(stats1.assertCounts(1, 1, 1, 1)); + TEST_DO(stats2.assertCounts(1, 1, 1, 1)); + TEST_DO(stats3.assertCounts(1, 1, 0, 1)); f.removeListeners("testdoc2", {"test3"}); - TEST_DO(target1.assertCounts(1, 1, 1, 1)); - TEST_DO(target2.assertCounts(1, 1, 1, 1)); - TEST_DO(target3.assertCounts(1, 1, 0, 1)); + TEST_DO(stats1.assertCounts(1, 1, 1, 1)); + TEST_DO(stats2.assertCounts(1, 1, 1, 1)); + TEST_DO(stats3.assertCounts(1, 1, 0, 1)); f.removeListeners("testdoc2", {"foo"}); - TEST_DO(target1.assertCounts(1, 1, 1, 1)); - TEST_DO(target2.assertCounts(1, 1, 1, 1)); - TEST_DO(target3.assertCounts(1, 1, 1, 1)); + TEST_DO(stats1.assertCounts(1, 1, 1, 1)); + TEST_DO(stats2.assertCounts(1, 1, 1, 1)); + TEST_DO(stats3.assertCounts(1, 1, 1, 1)); +} + +TEST_F("Test that we keep old listener when registering duplicate", Fixture) +{ + auto &stats = f.addStats(); + auto listener = std::make_unique(stats, "test1", "testdoc"); + TEST_DO(stats.assertCounts(1, 0, 0, 0)); + f.addListener(std::move(listener)); + TEST_DO(stats.assertCounts(1, 1, 0, 0)); + listener = std::make_unique(stats, "test1", "testdoc"); + TEST_DO(stats.assertCounts(2, 1, 0, 0)); + f.addListener(std::move(listener)); + TEST_DO(stats.assertCounts(2, 1, 1, 0)); } } 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 418b8079d88..651cd30ce68 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 @@ -144,7 +144,7 @@ TEST_F("Test that we can use gid to lid change listener", Fixture) TEST_DO(f.assertRefLid(10, 3)); } -TEST_F("Test that notifyRegistered method in gid to lid change listener works", Fixture) +TEST_F("Test that referenced lids are populated when listener is registered", Fixture) { f.ensureDocIdLimit(6); f.set(1, toGid(doc1)); diff --git a/searchcore/src/tests/proton/reference/gid_to_lid_change_registrator/gid_to_lid_change_registrator_test.cpp b/searchcore/src/tests/proton/reference/gid_to_lid_change_registrator/gid_to_lid_change_registrator_test.cpp index 072d618d359..a8ffc30c6c7 100644 --- a/searchcore/src/tests/proton/reference/gid_to_lid_change_registrator/gid_to_lid_change_registrator_test.cpp +++ b/searchcore/src/tests/proton/reference/gid_to_lid_change_registrator/gid_to_lid_change_registrator_test.cpp @@ -98,6 +98,8 @@ TEST_F("Test that we can register a listener", Fixture) TEST_DO(f.assertAdds({})); TEST_DO(f.assertRemoves({})); registrator->addListener(std::make_unique("testdoc", "f1")); + TEST_DO(f.assertAdds({{"testdoc","f1"}})); + TEST_DO(f.assertRemoves({})); registrator.reset(); TEST_DO(f.assertAdds({{"testdoc","f1"}})); TEST_DO(f.assertRemoves({{"testdoc",{"f1"}}})); @@ -110,6 +112,8 @@ TEST_F("Test that we can register multiple listeners", Fixture) TEST_DO(f.assertRemoves({})); registrator->addListener(std::make_unique("testdoc", "f1")); registrator->addListener(std::make_unique("testdoc", "f2")); + TEST_DO(f.assertAdds({{"testdoc","f1"},{"testdoc","f2"}})); + TEST_DO(f.assertRemoves({})); registrator.reset(); TEST_DO(f.assertAdds({{"testdoc","f1"},{"testdoc","f2"}})); TEST_DO(f.assertRemoves({{"testdoc",{"f1","f2"}}})); diff --git a/searchcore/src/vespa/searchcore/proton/reference/gid_to_lid_change_handler.cpp b/searchcore/src/vespa/searchcore/proton/reference/gid_to_lid_change_handler.cpp index 39f4b6b6cd1..0ae8642cf9e 100644 --- a/searchcore/src/vespa/searchcore/proton/reference/gid_to_lid_change_handler.cpp +++ b/searchcore/src/vespa/searchcore/proton/reference/gid_to_lid_change_handler.cpp @@ -90,6 +90,18 @@ GidToLidChangeHandler::removeListeners(const vespalib::string &docTypeName, } } +namespace { + +bool shouldRemoveListener(const IGidToLidChangeListener &listener, + const vespalib::string &docTypeName, + const std::set &keepNames) +{ + return ((listener.getDocTypeName() == docTypeName) && + (keepNames.find(listener.getName()) == keepNames.end())); +} + +} + void GidToLidChangeHandler::performRemoveListener(const vespalib::string &docTypeName, const std::set &keepNames) @@ -98,8 +110,7 @@ GidToLidChangeHandler::performRemoveListener(const vespalib::string &docTypeName if (_master) { auto itr = _listeners.begin(); while (itr != _listeners.end()) { - if (((*itr)->getDocTypeName() == docTypeName) && - (keepNames.find((*itr)->getName()) == keepNames.end())) { + if (shouldRemoveListener(**itr, docTypeName, keepNames)) { itr = _listeners.erase(itr); } else { ++itr; diff --git a/searchcore/src/vespa/searchcore/proton/reference/gid_to_lid_change_handler.h b/searchcore/src/vespa/searchcore/proton/reference/gid_to_lid_change_handler.h index 1c6fabf3c77..01a13beaaff 100644 --- a/searchcore/src/vespa/searchcore/proton/reference/gid_to_lid_change_handler.h +++ b/searchcore/src/vespa/searchcore/proton/reference/gid_to_lid_change_handler.h @@ -43,12 +43,14 @@ public: void close(); /* - * Add listener unless a listener with matching cookie already exists. + * Add listener unless a listener with matching docTypeName and + * name already exists. */ virtual void addListener(std::unique_ptr listener) override; /** - * Remove listener with matching cookie + * Remove listeners with matching docTypeName unless name is present in + * keepNames. */ virtual void removeListeners(const vespalib::string &docTypeName, const std::set &keepNames) override; diff --git a/searchcore/src/vespa/searchcore/proton/reference/gid_to_lid_change_listener.cpp b/searchcore/src/vespa/searchcore/proton/reference/gid_to_lid_change_listener.cpp index 7fedb93714c..271399fbdb5 100644 --- a/searchcore/src/vespa/searchcore/proton/reference/gid_to_lid_change_listener.cpp +++ b/searchcore/src/vespa/searchcore/proton/reference/gid_to_lid_change_listener.cpp @@ -40,7 +40,7 @@ GidToLidChangeListener::notifyRegistered() std::promise promise; std::future future = promise.get_future(); _attributeFieldWriter.execute(_attr->getName(), - [this, &promise]() { _attr->notifyGidToLidChangeListenerRegistered(); promise.set_value(true); }); + [this, &promise]() { _attr->populateReferencedLids(); promise.set_value(true); }); (void) future.get(); } -- cgit v1.2.3