diff options
author | Henning Baldersheim <balder@yahoo-inc.com> | 2020-08-07 07:05:01 +0000 |
---|---|---|
committer | Henning Baldersheim <balder@yahoo-inc.com> | 2020-08-07 07:06:57 +0000 |
commit | 4277c645ae52053fd760277a686134dd15d1bb6a (patch) | |
tree | 955461e9a9461a76d1d046aeeca2e212cf437192 | |
parent | e483631af3d65e217234455fe68ce840fca989a3 (diff) |
Improve code health of test code.
6 files changed, 108 insertions, 138 deletions
diff --git a/searchcore/src/apps/tests/persistenceconformance_test.cpp b/searchcore/src/apps/tests/persistenceconformance_test.cpp index 4056e49e37c..217d1edcd57 100644 --- a/searchcore/src/apps/tests/persistenceconformance_test.cpp +++ b/searchcore/src/apps/tests/persistenceconformance_test.cpp @@ -75,19 +75,19 @@ storeDocType(DocTypeVector *types, const DocumentType &type) struct SchemaConfigFactory { typedef DocumentDBConfig CS; typedef std::shared_ptr<SchemaConfigFactory> SP; - virtual ~SchemaConfigFactory() {} - static SchemaConfigFactory::SP get() { return SchemaConfigFactory::SP(new SchemaConfigFactory()); } + virtual ~SchemaConfigFactory() = default; + static SchemaConfigFactory::SP get() { return std::make_shared<SchemaConfigFactory>(); } virtual CS::IndexschemaConfigSP createIndexSchema(const DocumentType &docType) { (void) docType; - return CS::IndexschemaConfigSP(new IndexschemaConfig()); + return std::make_shared<IndexschemaConfig>(); } virtual CS::AttributesConfigSP createAttributes(const DocumentType &docType) { (void) docType; - return CS::AttributesConfigSP(new AttributesConfig()); + return std::make_shared<AttributesConfig>(); } virtual CS::SummaryConfigSP createSummary(const DocumentType &docType) { (void) docType; - return CS::SummaryConfigSP(new SummaryConfig()); + return std::make_shared<SummaryConfig>(); } }; @@ -98,12 +98,12 @@ private: SchemaConfigFactory::SP _schemaFactory; public: - ConfigFactory(const std::shared_ptr<const DocumentTypeRepo> &repo, - const DocumenttypesConfigSP &typeCfg, - const SchemaConfigFactory::SP &schemaFactory); + ConfigFactory(std::shared_ptr<const DocumentTypeRepo> repo, + DocumenttypesConfigSP typeCfg, + SchemaConfigFactory::SP schemaFactory); ~ConfigFactory(); - const std::shared_ptr<const DocumentTypeRepo> getTypeRepo() const { return _repo; } - const DocumenttypesConfigSP getTypeCfg() const { return _typeCfg; } + std::shared_ptr<const DocumentTypeRepo> getTypeRepo() const { return _repo; } + DocumenttypesConfigSP getTypeCfg() const { return _typeCfg; } DocTypeVector getDocTypes() const { DocTypeVector types; _repo->forEachDocumentType(*makeClosure(storeDocType, &types)); @@ -112,7 +112,7 @@ public: DocumentDBConfig::SP create(const DocTypeName &docTypeName) const { const DocumentType *docType = _repo->getDocumentType(docTypeName.getName()); - if (docType == NULL) { + if (docType == nullptr) { return DocumentDBConfig::SP(); } typedef DocumentDBConfig CS; @@ -123,7 +123,7 @@ public: SchemaBuilder::build(*indexschema, *schema); SchemaBuilder::build(*attributes, *schema); SchemaBuilder::build(*summary, *schema); - return DocumentDBConfig::SP(new DocumentDBConfig( + return std::make_shared<DocumentDBConfig>( 1, std::make_shared<RankProfilesConfig>(), std::make_shared<matching::RankingConstants>(), @@ -140,18 +140,18 @@ public: std::make_shared<DocumentDBMaintenanceConfig>(), search::LogDocumentStore::Config(), "client", - docTypeName.getName())); + docTypeName.getName()); } }; -ConfigFactory::ConfigFactory(const std::shared_ptr<const DocumentTypeRepo> &repo, const DocumenttypesConfigSP &typeCfg, - const SchemaConfigFactory::SP &schemaFactory) - : _repo(repo), - _typeCfg(typeCfg), - _schemaFactory(schemaFactory) +ConfigFactory::ConfigFactory(std::shared_ptr<const DocumentTypeRepo> repo, DocumenttypesConfigSP typeCfg, + SchemaConfigFactory::SP schemaFactory) + : _repo(std::move(repo)), + _typeCfg(std::move(typeCfg)), + _schemaFactory(std::move(schemaFactory)) {} -ConfigFactory::~ConfigFactory() {} +ConfigFactory::~ConfigFactory() = default; class DocumentDBFactory : public DummyDBOwner { private: @@ -167,7 +167,7 @@ private: public: DocumentDBFactory(const vespalib::string &baseDir, int tlsListenPort); - ~DocumentDBFactory(); + ~DocumentDBFactory() override; DocumentDB::SP create(BucketSpace bucketSpace, const DocTypeName &docType, const ConfigFactory &factory) { @@ -191,8 +191,7 @@ public: tuneFileDocDB, HwInfo())); mgr.forwardConfig(b); mgr.nextGeneration(0ms); - return DocumentDB::SP( - new DocumentDB(_baseDir, + return std::make_shared<DocumentDB>(_baseDir, mgr.getConfig(), _tlsSpec, _queryLimiter, @@ -209,7 +208,7 @@ public: _config_stores.getConfigStore(docType.toString()), std::make_shared<vespalib::ThreadStackExecutor> (16, 128 * 1024), - HwInfo())); + HwInfo()); } }; @@ -224,40 +223,33 @@ DocumentDBFactory::DocumentDBFactory(const vespalib::string &baseDir, int tlsLis _metricsWireService(), _summaryExecutor(8, 128 * 1024) {} -DocumentDBFactory::~DocumentDBFactory() {} +DocumentDBFactory::~DocumentDBFactory() = default; class DocumentDBRepo { private: DocumentDBMap _docDbs; public: typedef std::unique_ptr<DocumentDBRepo> UP; - DocumentDBRepo(const ConfigFactory &cfgFactory, - DocumentDBFactory &docDbFactory) : - _docDbs() + DocumentDBRepo(const ConfigFactory &cfgFactory, DocumentDBFactory &docDbFactory) + : _docDbs() { DocTypeVector types = cfgFactory.getDocTypes(); - for (size_t i = 0; i < types.size(); ++i) { - BucketSpace bucketSpace(makeBucketSpace(types[i].getName())); - DocumentDB::SP docDb = docDbFactory.create(bucketSpace, - types[i], - cfgFactory); + for (const auto & type : types) { + BucketSpace bucketSpace(makeBucketSpace(type.getName())); + DocumentDB::SP docDb = docDbFactory.create(bucketSpace, type, cfgFactory); docDb->start(); docDb->waitForOnlineState(); - _docDbs[types[i]] = docDb; + _docDbs[type] = docDb; } } - void - close() - { - for (DocumentDBMap::iterator itr = _docDbs.begin(); - itr != _docDbs.end(); ++itr) { - itr->second->close(); + void close() { + for (auto & dbEntry : _docDbs) { + dbEntry.second->close(); } } - ~DocumentDBRepo() - { + ~DocumentDBRepo() { close(); } const DocumentDBMap &getDocDbs() const { return _docDbs; } @@ -269,20 +261,15 @@ class DocDBRepoHolder protected: DocumentDBRepo::UP _docDbRepo; - DocDBRepoHolder(DocumentDBRepo::UP docDbRepo) + explicit DocDBRepoHolder(DocumentDBRepo::UP docDbRepo) : _docDbRepo(std::move(docDbRepo)) { } - virtual - ~DocDBRepoHolder() - { - } + virtual ~DocDBRepoHolder() = default; - void - close() - { - if (_docDbRepo.get() != NULL) + void close() { + if (_docDbRepo) _docDbRepo->close(); } }; @@ -290,17 +277,13 @@ protected: class MyPersistenceEngineOwner : public IPersistenceEngineOwner { - virtual void - setClusterState(BucketSpace, const storage::spi::ClusterState &calc) override - { - (void) calc; - } + void setClusterState(BucketSpace, const storage::spi::ClusterState &) override { } }; struct MyResourceWriteFilter : public IResourceWriteFilter { - virtual bool acceptWriteOperation() const override { return true; } - virtual State getAcceptState() const override { return IResourceWriteFilter::State(); } + bool acceptWriteOperation() const override { return true; } + State getAcceptState() const override { return IResourceWriteFilter::State(); } }; class MyPersistenceEngine : public DocDBRepoHolder, @@ -320,35 +303,32 @@ public: void addHandlers(const vespalib::string &docType) { - if (_docDbRepo.get() == NULL) + if (!_docDbRepo) return; const DocumentDBMap &docDbs = _docDbRepo->getDocDbs(); - for (DocumentDBMap::const_iterator itr = docDbs.begin(); - itr != docDbs.end(); ++itr) { - if (!docType.empty() && docType != itr->first.getName()) { + for (const auto & dbEntry : docDbs) { + if (!docType.empty() && docType != dbEntry.first.getName()) { continue; } - LOG(info, "putHandler(%s)", itr->first.toString().c_str()); - IPersistenceHandler::SP proxy( - new PersistenceHandlerProxy(itr->second)); - putHandler(getWLock(), itr->second->getBucketSpace(), itr->first, proxy); + LOG(info, "putHandler(%s)", dbEntry.first.toString().c_str()); + auto proxy = std::make_shared<PersistenceHandlerProxy>(dbEntry.second); + putHandler(getWLock(), dbEntry.second->getBucketSpace(), dbEntry.first, proxy); } } void removeHandlers() { - if (_docDbRepo.get() == NULL) + if ( ! _docDbRepo) return; const DocumentDBMap &docDbs = _docDbRepo->getDocDbs(); - for (DocumentDBMap::const_iterator itr = docDbs.begin(); - itr != docDbs.end(); ++itr) { - IPersistenceHandler::SP proxy(removeHandler(getWLock(), itr->second->getBucketSpace(), itr->first)); + for (const auto & dbEntry : docDbs) { + IPersistenceHandler::SP proxy(removeHandler(getWLock(), dbEntry.second->getBucketSpace(), dbEntry.first)); + (void) proxy; } } - virtual - ~MyPersistenceEngine() + ~MyPersistenceEngine() override { destroyIterators(); removeHandlers(); // Block calls to document db from engine @@ -367,11 +347,11 @@ private: MyResourceWriteFilter _writeFilter; public: MyPersistenceFactory(const vespalib::string &baseDir, int tlsListenPort, - const SchemaConfigFactory::SP &schemaFactory, - const vespalib::string &docType = "") + SchemaConfigFactory::SP schemaFactory, + const vespalib::string & docType = "") : _baseDir(baseDir), _docDbFactory(baseDir, tlsListenPort), - _schemaFactory(schemaFactory), + _schemaFactory(std::move(schemaFactory)), _docDbRepo(), _docType(docType), _engineOwner(), @@ -382,25 +362,25 @@ public: ~MyPersistenceFactory() override { clear(); } - virtual PersistenceProvider::UP getPersistenceImplementation(const std::shared_ptr<const DocumentTypeRepo> &repo, - const DocumenttypesConfig &typesCfg) override { - ConfigFactory cfgFactory(repo, DocumenttypesConfigSP(new DocumenttypesConfig(typesCfg)), _schemaFactory); - _docDbRepo.reset(new DocumentDBRepo(cfgFactory, _docDbFactory)); + PersistenceProvider::UP getPersistenceImplementation(const std::shared_ptr<const DocumentTypeRepo> &repo, + const DocumenttypesConfig &typesCfg) override { + ConfigFactory cfgFactory(repo, std::make_shared<DocumenttypesConfig>(typesCfg), _schemaFactory); + _docDbRepo = std::make_unique<DocumentDBRepo>(cfgFactory, _docDbFactory); PersistenceEngine::UP engine(new MyPersistenceEngine(_engineOwner, _writeFilter, std::move(_docDbRepo), _docType)); - assert(_docDbRepo.get() == NULL); // Repo should be handed over - return PersistenceProvider::UP(engine.release()); + assert( ! _docDbRepo); // Repo should be handed over + return engine; } - virtual void clear() override { + void clear() override { FastOS_FileInterface::EmptyAndRemoveDirectory(_baseDir.c_str()); } - virtual bool hasPersistence() const override { return true; } - virtual bool supportsActiveState() const override { return true; } - virtual bool supportsBucketSpaces() const override { return true; } + bool hasPersistence() const override { return true; } + bool supportsActiveState() const override { return true; } + bool supportsBucketSpaces() const override { return true; } }; diff --git a/searchcore/src/tests/proton/server/documentretriever_test.cpp b/searchcore/src/tests/proton/server/documentretriever_test.cpp index 30a87bd216e..40224902b8f 100644 --- a/searchcore/src/tests/proton/server/documentretriever_test.cpp +++ b/searchcore/src/tests/proton/server/documentretriever_test.cpp @@ -314,6 +314,17 @@ struct Fixture { attr->commit(); } + Fixture & + addIndexField(const Schema::IndexField &field) { + schema.addIndexField(field); + return *this; + } + + void + build() { + _retriever = std::make_unique<DocumentRetriever>(_dtName, repo, schema, meta_store, attr_manager, doc_store); + } + Fixture() : repo(getRepoConfig()), meta_store(std::make_shared<BucketDBOwner>()), @@ -335,46 +346,29 @@ struct Fixture { lid = putRes.getLid(); ASSERT_TRUE(putRes.ok()); schema::CollectionType ct = schema::CollectionType::SINGLE; - addAttribute<IntegerAttribute>( - dyn_field_i, dyn_value_i, DataType::INT32, ct); - addAttribute<FloatingPointAttribute>( - dyn_field_d, dyn_value_d, DataType::DOUBLE, ct); - addAttribute<StringAttribute>( - dyn_field_s, dyn_value_s, DataType::STRING, ct); - addAttribute<FloatingPointAttribute>( - dyn_field_n, DataType::FLOAT, ct); - addAttribute<IntegerAttribute>( - dyn_field_nai, DataType::INT32, ct); - addAttribute<StringAttribute>( - dyn_field_nas, DataType::STRING, ct); - addAttribute<IntegerAttribute>( - zcurve_field, dynamic_zcurve_value, DataType::INT64, ct); + addAttribute<IntegerAttribute>(dyn_field_i, dyn_value_i, DataType::INT32, ct); + addAttribute<FloatingPointAttribute>(dyn_field_d, dyn_value_d, DataType::DOUBLE, ct); + addAttribute<StringAttribute>(dyn_field_s, dyn_value_s, DataType::STRING, ct); + addAttribute<FloatingPointAttribute>(dyn_field_n, DataType::FLOAT, ct); + addAttribute<IntegerAttribute>(dyn_field_nai, DataType::INT32, ct); + addAttribute<StringAttribute>(dyn_field_nas, DataType::STRING, ct); + addAttribute<IntegerAttribute>(zcurve_field, dynamic_zcurve_value, DataType::INT64, ct); addTensorAttribute(dyn_field_tensor.c_str(), *dynamic_tensor); - PredicateAttribute *attr = addAttribute<PredicateAttribute>( - dyn_field_p, DataType::BOOLEANTREE, ct); + PredicateAttribute *attr = addAttribute<PredicateAttribute>(dyn_field_p, DataType::BOOLEANTREE, ct); attr->getIndex().indexEmptyDocument(lid); attr->commit(); ct = schema::CollectionType::ARRAY; - addAttribute<IntegerAttribute>( - dyn_arr_field_i, dyn_value_i, DataType::INT32, ct); - addAttribute<FloatingPointAttribute>( - dyn_arr_field_d, dyn_value_d, DataType::DOUBLE, ct); - addAttribute<StringAttribute>( - dyn_arr_field_s, dyn_value_s, DataType::STRING, ct); - addAttribute<FloatingPointAttribute>( - dyn_arr_field_n, DataType::FLOAT, ct); - addAttribute<IntegerAttribute>( - zcurve_array_field, dynamic_zcurve_value, DataType::INT64, ct); + addAttribute<IntegerAttribute>(dyn_arr_field_i, dyn_value_i, DataType::INT32, ct); + addAttribute<FloatingPointAttribute>(dyn_arr_field_d, dyn_value_d, DataType::DOUBLE, ct); + addAttribute<StringAttribute>(dyn_arr_field_s, dyn_value_s, DataType::STRING, ct); + addAttribute<FloatingPointAttribute>(dyn_arr_field_n, DataType::FLOAT, ct); + addAttribute<IntegerAttribute>(zcurve_array_field, dynamic_zcurve_value, DataType::INT64, ct); ct = schema::CollectionType::WEIGHTEDSET; - addAttribute<IntegerAttribute>( - dyn_wset_field_i, dyn_value_i, DataType::INT32, ct); - addAttribute<FloatingPointAttribute>( - dyn_wset_field_d, dyn_value_d, DataType::DOUBLE, ct); - addAttribute<StringAttribute>( - dyn_wset_field_s, dyn_value_s, DataType::STRING, ct); - addAttribute<FloatingPointAttribute>( - dyn_wset_field_n, DataType::FLOAT, ct); - _retriever = std::make_unique<DocumentRetriever>(_dtName, repo, schema, meta_store, attr_manager, doc_store); + addAttribute<IntegerAttribute>(dyn_wset_field_i, dyn_value_i, DataType::INT32, ct); + addAttribute<FloatingPointAttribute>(dyn_wset_field_d, dyn_value_d, DataType::DOUBLE, ct); + addAttribute<StringAttribute>(dyn_wset_field_s, dyn_value_s, DataType::STRING, ct); + addAttribute<FloatingPointAttribute>(dyn_wset_field_n, DataType::FLOAT, ct); + build(); } void clearAttributes(std::vector<vespalib::string> names) { @@ -386,15 +380,13 @@ struct Fixture { } }; -TEST_F("require that document retriever can retrieve document meta data", - Fixture) { +TEST_F("require that document retriever can retrieve document meta data", Fixture) { DocumentMetaData meta_data = f._retriever->getDocumentMetaData(doc_id); EXPECT_EQUAL(f.lid, meta_data.lid); EXPECT_EQUAL(f.timestamp, meta_data.timestamp); } -TEST_F("require that document retriever can retrieve bucket meta data", - Fixture) { +TEST_F("require that document retriever can retrieve bucket meta data", Fixture) { DocumentMetaData::Vector result; f._retriever->getBucketMetaData(makeSpiBucket(f.bucket_id, PartitionId(0)), result); ASSERT_EQUAL(1u, result.size()); @@ -476,7 +468,7 @@ TEST_F("require that attributes are patched into stored document", Fixture) { } TEST_F("require that attributes are patched into stored document unless also index field", Fixture) { - f.schema.addIndexField(Schema::IndexField(dyn_field_s, DataType::STRING)); + f.addIndexField(Schema::IndexField(dyn_field_s, DataType::STRING)).build(); DocumentMetaData meta_data = f._retriever->getDocumentMetaData(doc_id); Document::UP doc = f._retriever->getDocument(meta_data.lid); ASSERT_TRUE(doc.get()); @@ -574,9 +566,8 @@ TEST_F("require that tensor attribute can be retrieved", Fixture) { ASSERT_TRUE(doc.get()); FieldValue::UP value = doc->getValue(dyn_field_tensor); - ASSERT_TRUE(value.get()); - TensorFieldValue *tensor_value = - dynamic_cast<TensorFieldValue *>(value.get()); + ASSERT_TRUE(value); + TensorFieldValue *tensor_value = dynamic_cast<TensorFieldValue *>(value.get()); ASSERT_TRUE(tensor_value->getAsTensorPtr()->equals(*dynamic_tensor)); } diff --git a/searchcore/src/vespa/searchcore/proton/server/documentretriever.cpp b/searchcore/src/vespa/searchcore/proton/server/documentretriever.cpp index 09550bd74dd..7466bcd99d5 100644 --- a/searchcore/src/vespa/searchcore/proton/server/documentretriever.cpp +++ b/searchcore/src/vespa/searchcore/proton/server/documentretriever.cpp @@ -143,7 +143,7 @@ public: } } - virtual bool allowVisitCaching() const override { + bool allowVisitCaching() const override { return _visitor.allowVisitCaching(); } @@ -154,7 +154,8 @@ private: } // namespace -Document::UP DocumentRetriever::getDocument(DocumentIdT lid) const +Document::UP +DocumentRetriever::getDocument(DocumentIdT lid) const { Document::UP doc = _doc_store.read(lid, getDocumentTypeRepo()); if (doc) { diff --git a/searchcore/src/vespa/searchcore/proton/server/documentretrieverbase.cpp b/searchcore/src/vespa/searchcore/proton/server/documentretrieverbase.cpp index 65a4f7e7c4a..183e9eb0c0a 100644 --- a/searchcore/src/vespa/searchcore/proton/server/documentretrieverbase.cpp +++ b/searchcore/src/vespa/searchcore/proton/server/documentretrieverbase.cpp @@ -26,7 +26,7 @@ DocumentRetrieverBase::DocumentRetrieverBase( _hasFields(hasFields) { const document::DocumentType * docType(_repo.getDocumentType(_docTypeName.getName())); - _emptyDoc.reset(new document::Document(*docType, DocumentId("id:empty:" + _docTypeName.getName() + "::empty"))); + _emptyDoc = std::make_unique<document::Document>(*docType, DocumentId("id:empty:" + _docTypeName.getName() + "::empty")); _emptyDoc->setRepo(_repo); } @@ -58,7 +58,7 @@ DocumentRetrieverBase::getDocumentMetaData(const DocumentId &id) const { const search::IAttributeManager * DocumentRetrieverBase::getAttrMgr() const { - return NULL; + return nullptr; } @@ -71,7 +71,7 @@ DocumentRetrieverBase::parseSelect(const vespalib::string &selection) const return _selectCache[selection]; } - CachedSelect::SP nselect(new CachedSelect()); + auto nselect = std::make_shared<CachedSelect>(); nselect->set(selection, _docTypeName.getName(), diff --git a/searchcore/src/vespa/searchcore/proton/server/persistencehandlerproxy.cpp b/searchcore/src/vespa/searchcore/proton/server/persistencehandlerproxy.cpp index af4af7f64fe..cd6df7c5816 100644 --- a/searchcore/src/vespa/searchcore/proton/server/persistencehandlerproxy.cpp +++ b/searchcore/src/vespa/searchcore/proton/server/persistencehandlerproxy.cpp @@ -1,7 +1,6 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. #include "persistencehandlerproxy.h" -#include "documentretriever.h" #include "documentdb.h" #include <vespa/searchcore/proton/feedoperation/createbucketoperation.h> #include <vespa/searchcore/proton/feedoperation/deletebucketoperation.h> @@ -10,7 +9,6 @@ #include <vespa/searchcore/proton/feedoperation/removeoperation.h> #include <vespa/searchcore/proton/feedoperation/splitbucketoperation.h> #include <vespa/searchcore/proton/feedoperation/updateoperation.h> -#include <vespa/document/fieldvalue/document.h> #include <vespa/document/update/documentupdate.h> using storage::spi::Bucket; @@ -18,8 +16,8 @@ using storage::spi::Timestamp; namespace proton { -PersistenceHandlerProxy::PersistenceHandlerProxy(const DocumentDB::SP &documentDB) - : _documentDB(documentDB), +PersistenceHandlerProxy::PersistenceHandlerProxy(DocumentDB::SP documentDB) + : _documentDB(std::move(documentDB)), _feedHandler(_documentDB->getFeedHandler()), _bucketHandler(_documentDB->getBucketHandler()), _clusterStateHandler(_documentDB->getClusterStateHandler()) diff --git a/searchcore/src/vespa/searchcore/proton/server/persistencehandlerproxy.h b/searchcore/src/vespa/searchcore/proton/server/persistencehandlerproxy.h index 5a82b46f91c..38ddb667213 100644 --- a/searchcore/src/vespa/searchcore/proton/server/persistencehandlerproxy.h +++ b/searchcore/src/vespa/searchcore/proton/server/persistencehandlerproxy.h @@ -18,9 +18,9 @@ private: BucketHandler &_bucketHandler; ClusterStateHandler &_clusterStateHandler; public: - PersistenceHandlerProxy(const std::shared_ptr<DocumentDB> &documentDB); + explicit PersistenceHandlerProxy(std::shared_ptr<DocumentDB> documentDB); - virtual ~PersistenceHandlerProxy(); + ~PersistenceHandlerProxy() override; void initialize() override; |