From 2eba9b74da460bff37eb4e6b6fb89c3df325e095 Mon Sep 17 00:00:00 2001 From: Henning Baldersheim Date: Thu, 12 Oct 2017 22:24:57 +0200 Subject: Allow -Wformat-security + some c++11ification based on Clions advise. --- .../vespa/storage/distributor/bucketdbupdater.cpp | 135 +++++++-------------- .../vespa/storage/distributor/bucketdbupdater.h | 78 ++++-------- 2 files changed, 66 insertions(+), 147 deletions(-) diff --git a/storage/src/vespa/storage/distributor/bucketdbupdater.cpp b/storage/src/vespa/storage/distributor/bucketdbupdater.cpp index 67942d3d447..9ce59b257c2 100644 --- a/storage/src/vespa/storage/distributor/bucketdbupdater.cpp +++ b/storage/src/vespa/storage/distributor/bucketdbupdater.cpp @@ -26,27 +26,23 @@ BucketDBUpdater::BucketDBUpdater(Distributor& owner, ManagedBucketSpace& bucketS { } -BucketDBUpdater::~BucketDBUpdater() {} +BucketDBUpdater::~BucketDBUpdater() = default; void BucketDBUpdater::flush() { - for (std::map::iterator - i(_sentMessages.begin()), end(_sentMessages.end()); - i != end; ++i) - { + for (auto & entry : _sentMessages) { // Cannot sendDown MergeBucketReplies during flushing, since // all lower links have been closed - if (i->second._mergeReplyGuard.get()) { - i->second._mergeReplyGuard->resetReply(); + if (entry.second._mergeReplyGuard) { + entry.second._mergeReplyGuard->resetReply(); } } _sentMessages.clear(); } void -BucketDBUpdater::print(std::ostream& out, bool verbose, - const std::string& indent) const +BucketDBUpdater::print(std::ostream& out, bool verbose, const std::string& indent) const { (void) verbose; (void) indent; out << "BucketDBUpdater"; @@ -55,17 +51,15 @@ BucketDBUpdater::print(std::ostream& out, bool verbose, bool BucketDBUpdater::hasPendingClusterState() const { - return _pendingClusterState.get() != nullptr; + return (bool)_pendingClusterState; } BucketOwnership BucketDBUpdater::checkOwnershipInPendingState(const document::BucketId& b) const { if (hasPendingClusterState()) { - const lib::ClusterState& state( - _pendingClusterState->getNewClusterState()); - const lib::Distribution& distribution( - _pendingClusterState->getDistribution()); + const lib::ClusterState& state(_pendingClusterState->getNewClusterState()); + const lib::Distribution& distribution(_pendingClusterState->getDistribution()); if (!_bucketSpaceComponent.ownsBucketInState(distribution, state, b)) { return BucketOwnership::createNotOwnedInState(state); } @@ -73,22 +67,6 @@ BucketDBUpdater::checkOwnershipInPendingState(const document::BucketId& b) const return BucketOwnership::createOwned(); } -void -BucketDBUpdater::clearPending(uint16_t node) -{ - for (std::map::iterator iter( - _sentMessages.begin()); iter != _sentMessages.end();) - { - if (iter->second.targetNode == node) { - std::map::iterator del = iter; - iter++; - _sentMessages.erase(del); - } else { - iter++; - } - } -} - void BucketDBUpdater::sendRequestBucketInfo( uint16_t node, @@ -145,9 +123,8 @@ BucketDBUpdater::removeSuperfluousBuckets( _bucketSpaceComponent.getBucketDatabase().forEach(proc); - for (uint32_t i = 0; i < proc.getBucketsToRemove().size(); ++i) { - _bucketSpaceComponent.getBucketDatabase() - .remove(proc.getBucketsToRemove()[i]); + for (const auto & entry :proc.getBucketsToRemove()) { + _bucketSpaceComponent.getBucketDatabase().remove(entry); } } @@ -247,9 +224,8 @@ BucketDBUpdater::onSetSystemState( BucketDBUpdater::MergeReplyGuard::~MergeReplyGuard() { - if (_reply.get()) { - _updater.getDistributorComponent().getDistributor() - .handleCompletedMerge(_reply); + if (_reply) { + _updater.getDistributorComponent().getDistributor().handleCompletedMerge(_reply); } } @@ -293,13 +269,8 @@ BucketDBUpdater::sendAllQueuedBucketRechecks() "via NotifyBucketChange commands", _enqueuedRechecks.size()); - typedef std::set::const_iterator const_iterator; - for (const_iterator it(_enqueuedRechecks.begin()), - e(_enqueuedRechecks.end()); it != e; ++it) - { - sendRequestBucketInfo(it->node, - it->bucket, - std::shared_ptr()); + for (const auto & entry :_enqueuedRechecks) { + sendRequestBucketInfo(entry.node, entry.bucket, std::shared_ptr()); } _enqueuedRechecks.clear(); } @@ -382,14 +353,14 @@ BucketDBUpdater::handleSingleBucketInfoFailure( if (req.bucket != document::BucketId(0)) { framework::MilliSecTime sendTime(_bucketSpaceComponent.getClock()); sendTime += framework::MilliSecTime(100); - _delayedRequests.push_back(std::make_pair(sendTime, req)); + _delayedRequests.emplace_back(sendTime, req); } } void BucketDBUpdater::resendDelayedMessages() { - if (_pendingClusterState.get()) { + if (_pendingClusterState) { _pendingClusterState->resendDelayedMessages(); } if (_delayedRequests.empty()) return; // Don't fetch time if not needed @@ -398,9 +369,7 @@ BucketDBUpdater::resendDelayedMessages() && currentTime >= _delayedRequests.front().first) { BucketRequest& req(_delayedRequests.front().second); - sendRequestBucketInfo(req.targetNode, - req.bucket, - std::shared_ptr()); + sendRequestBucketInfo(req.targetNode, req.bucket, std::shared_ptr()); _delayedRequests.pop_front(); } } @@ -408,19 +377,13 @@ BucketDBUpdater::resendDelayedMessages() void BucketDBUpdater::convertBucketInfoToBucketList( const std::shared_ptr& repl, - uint16_t targetNode, - BucketListMerger::BucketList& newList) + uint16_t targetNode, BucketListMerger::BucketList& newList) { - for (uint32_t i = 0; i < repl->getBucketInfo().size(); i++) { - LOG(debug, - "Received bucket information from node %u for bucket %s: %s", - targetNode, - repl->getBucketInfo()[i]._bucketId.toString().c_str(), - repl->getBucketInfo()[i]._info.toString().c_str()); + for (const auto & entry : repl->getBucketInfo()) { + LOG(debug, "Received bucket information from node %u for bucket %s: %s", targetNode, + entry._bucketId.toString().c_str(), entry._info.toString().c_str()); - newList.push_back(BucketListMerger::BucketEntry( - repl->getBucketInfo()[i]._bucketId, - repl->getBucketInfo()[i]._info)); + newList.emplace_back(entry._bucketId, entry._info); } } @@ -446,8 +409,7 @@ bool BucketDBUpdater::processSingleBucketInfoReply( const std::shared_ptr & repl) { - std::map::iterator iter = - _sentMessages.find(repl->getMsgId()); + auto iter = _sentMessages.find(repl->getMsgId()); // Has probably been deleted for some reason earlier. if (iter == _sentMessages.end()) { @@ -477,36 +439,30 @@ BucketDBUpdater::addBucketInfoForNode( { const BucketCopy* copy(e->getNode(node)); if (copy) { - existing.push_back(BucketListMerger::BucketEntry( - e.getBucketId(), copy->getBucketInfo())); + existing.emplace_back(e.getBucketId(), copy->getBucketInfo()); } } void -BucketDBUpdater::findRelatedBucketsInDatabase( - uint16_t node, - const document::BucketId& bucketId, - BucketListMerger::BucketList& existing) +BucketDBUpdater::findRelatedBucketsInDatabase(uint16_t node, const document::BucketId& bucketId, + BucketListMerger::BucketList& existing) { std::vector entries; _bucketSpaceComponent.getBucketDatabase().getAll(bucketId, entries); - for (uint32_t j = 0; j < entries.size(); ++j) { - addBucketInfoForNode(entries[j], node, existing); + for (const BucketDatabase::Entry & entry : entries) { + addBucketInfoForNode(entry, node, existing); } } void BucketDBUpdater::updateDatabase(uint16_t node, BucketListMerger& merger) { - for (uint32_t i = 0; i < merger.getRemovedEntries().size(); i++) { - _bucketSpaceComponent.removeNodeFromDB(merger.getRemovedEntries()[i], node); + for (const document::BucketId & bucket : merger.getRemovedEntries()) { + _bucketSpaceComponent.removeNodeFromDB(bucket, node); } - for (uint32_t i = 0; i < merger.getAddedEntries().size(); i++) { - const BucketListMerger::BucketEntry& entry( - merger.getAddedEntries()[i]); - + for (const BucketListMerger::BucketEntry& entry : merger.getAddedEntries()) { _bucketSpaceComponent.updateBucketDatabase( entry.first, BucketCopy(merger.getTimestamp(), node, entry.second), @@ -603,12 +559,11 @@ BucketDBUpdater::reportXmlStatus(vespalib::xml::XmlOutputStream& xos, << XmlTag("systemstate_active") << XmlContent(_bucketSpaceComponent.getClusterState().toString()) << XmlEndTag(); - if (_pendingClusterState.get() != 0) { + if (_pendingClusterState) { xos << *_pendingClusterState; } xos << XmlTag("systemstate_history"); - typedef std::list::const_reverse_iterator HistoryIter; - for (HistoryIter i(_history.rbegin()), e(_history.rend()); i != e; ++i) { + for (auto i(_history.rbegin()), e(_history.rend()); i != e; ++i) { xos << XmlTag("change") << XmlAttribute("from", i->_prevClusterState) << XmlAttribute("to", i->_newClusterState) @@ -617,18 +572,16 @@ BucketDBUpdater::reportXmlStatus(vespalib::xml::XmlOutputStream& xos, } xos << XmlEndTag() << XmlTag("single_bucket_requests"); - for (std::map::const_iterator iter - = _sentMessages.begin(); iter != _sentMessages.end(); iter++) + for (const auto & entry : _sentMessages) { xos << XmlTag("storagenode") - << XmlAttribute("index", iter->second.targetNode); - if (iter->second.bucket.getRawId() == 0) { + << XmlAttribute("index", entry.second.targetNode); + if (entry.second.bucket.getRawId() == 0) { xos << XmlAttribute("bucket", ALL); } else { - xos << XmlAttribute("bucket", iter->second.bucket.getId(), - XmlAttribute::HEX); + xos << XmlAttribute("bucket", entry.second.bucket.getId(), XmlAttribute::HEX); } - xos << XmlAttribute("sendtimestamp", iter->second.timestamp) + xos << XmlAttribute("sendtimestamp", entry.second.timestamp) << XmlEndTag(); } xos << XmlEndTag() << XmlEndTag(); @@ -642,17 +595,13 @@ BucketDBUpdater::BucketListGenerator::process(BucketDatabase::Entry& e) const BucketCopy* copy(e->getNode(_node)); if (copy) { - _entries.push_back( - BucketListMerger::BucketEntry( - bucketId, - copy->getBucketInfo())); + _entries.emplace_back(bucketId, copy->getBucketInfo()); } return true; } void -BucketDBUpdater::NodeRemover::logRemove(const document::BucketId& bucketId, - const char* msg) const +BucketDBUpdater::NodeRemover::logRemove(const document::BucketId& bucketId, const char* msg) const { LOG(spam, "Removing bucket %s: %s", bucketId.toString().c_str(), msg); LOG_BUCKET_OPERATION_NO_LOCK(bucketId, msg); @@ -748,7 +697,7 @@ BucketDBUpdater::NodeRemover::process(BucketDatabase::Entry& e) BucketDBUpdater::NodeRemover::~NodeRemover() { - if (_removedBuckets.size() > 0) { + if ( !_removedBuckets.empty()) { std::ostringstream ost; ost << "After system state change " << _oldState.getTextualDifference(_state) << ", we removed " @@ -760,7 +709,7 @@ BucketDBUpdater::NodeRemover::~NodeRemover() if (_removedBuckets.size() >= 10) { ost << " ..."; } - LOGBM(info, ost.str().c_str()); + LOGBM(info, "%s", ost.str().c_str()); } } diff --git a/storage/src/vespa/storage/distributor/bucketdbupdater.h b/storage/src/vespa/storage/distributor/bucketdbupdater.h index 6081f782cd5..d3b2bbf86ca 100644 --- a/storage/src/vespa/storage/distributor/bucketdbupdater.h +++ b/storage/src/vespa/storage/distributor/bucketdbupdater.h @@ -36,16 +36,11 @@ public: ~BucketDBUpdater(); void flush(); - BucketOwnership checkOwnershipInPendingState(const document::BucketId&) const; - void recheckBucketInfo(uint32_t nodeIdx, const document::BucketId& bid); bool onSetSystemState(const std::shared_ptr& cmd) override; - - bool onRequestBucketInfoReply( - const std::shared_ptr & repl) override; - + bool onRequestBucketInfoReply(const std::shared_ptr & repl) override; bool onMergeBucketReply(const std::shared_ptr& reply) override; bool onNotifyBucketChange(const std::shared_ptr&) override; void resendDelayedMessages(); @@ -72,8 +67,7 @@ private: ManagedBucketSpaceComponent _bucketSpaceComponent; class MergeReplyGuard { public: - MergeReplyGuard(BucketDBUpdater& updater, - const std::shared_ptr& reply) + MergeReplyGuard(BucketDBUpdater& updater, const std::shared_ptr& reply) : _updater(updater), _reply(reply) {} ~MergeReplyGuard(); @@ -90,9 +84,7 @@ private: BucketRequest() : targetNode(0), bucket(0), timestamp(0) {}; - BucketRequest(uint16_t t, - uint64_t currentTime, - const document::BucketId& b, + BucketRequest(uint16_t t, uint64_t currentTime, const document::BucketId& b, const std::shared_ptr& guard) : targetNode(t), bucket(b), @@ -112,8 +104,7 @@ private: EnqueuedBucketRecheck() : node(0), bucket() {} - EnqueuedBucketRecheck(uint16_t _node, - const document::BucketId& _bucket) + EnqueuedBucketRecheck(uint16_t _node, const document::BucketId& _bucket) : node(_node), bucket(_bucket) {} @@ -130,35 +121,21 @@ private: }; bool hasPendingClusterState() const; - - void clearPending(uint16_t node); - - bool pendingClusterStateAccepted( - const std::shared_ptr& repl); - bool bucketOwnedAccordingToPendingState( - const document::BucketId& bucketId) const; - bool processSingleBucketInfoReply( - const std::shared_ptr& repl); - void handleSingleBucketInfoFailure( - const std::shared_ptr& repl, - const BucketRequest& req); + bool pendingClusterStateAccepted(const std::shared_ptr& repl); + bool bucketOwnedAccordingToPendingState(const document::BucketId& bucketId) const; + bool processSingleBucketInfoReply(const std::shared_ptr& repl); + void handleSingleBucketInfoFailure(const std::shared_ptr& repl, + const BucketRequest& req); bool isPendingClusterStateCompleted() const; void processCompletedPendingClusterState(); - void mergeBucketInfoWithDatabase( - const std::shared_ptr& repl, - const BucketRequest& req); - void convertBucketInfoToBucketList( - const std::shared_ptr& repl, - uint16_t targetNode, - BucketListMerger::BucketList& newList); - void sendRequestBucketInfo( - uint16_t node, - const document::BucketId& bucket, - const std::shared_ptr& mergeReply); - void addBucketInfoForNode( - const BucketDatabase::Entry& e, - uint16_t node, - BucketListMerger::BucketList& existing) const; + void mergeBucketInfoWithDatabase(const std::shared_ptr& repl, + const BucketRequest& req); + void convertBucketInfoToBucketList(const std::shared_ptr& repl, + uint16_t targetNode, BucketListMerger::BucketList& newList); + void sendRequestBucketInfo(uint16_t node, const document::BucketId& bucket, + const std::shared_ptr& mergeReply); + void addBucketInfoForNode(const BucketDatabase::Entry& e, uint16_t node, + BucketListMerger::BucketList& existing) const; void ensureTransitionTimerStarted(); void completeTransitionTimer(); /** @@ -167,10 +144,8 @@ private: * in bucketId, or that bucketId is contained in, that have copies * on the given node. */ - void findRelatedBucketsInDatabase( - uint16_t node, - const document::BucketId& bucketId, - BucketListMerger::BucketList& existing); + void findRelatedBucketsInDatabase(uint16_t node, const document::BucketId& bucketId, + BucketListMerger::BucketList& existing); /** Updates the bucket database from the information generated by the given @@ -178,18 +153,15 @@ private: */ void updateDatabase(uint16_t node, BucketListMerger& merger); - void updateState(const lib::ClusterState& oldState, - const lib::ClusterState& newState); + void updateState(const lib::ClusterState& oldState, const lib::ClusterState& newState); - void removeSuperfluousBuckets(const lib::Distribution& newDistribution, - const lib::ClusterState& newState); + void removeSuperfluousBuckets(const lib::Distribution& newDistribution, const lib::ClusterState& newState); void replyToPreviousPendingClusterStateIfAny(); void enableCurrentClusterStateInDistributor(); void addCurrentStateToClusterStateHistory(); - void enqueueRecheckUntilPendingStateEnabled(uint16_t node, - const document::BucketId&); + void enqueueRecheckUntilPendingStateEnabled(uint16_t node, const document::BucketId&); void sendAllQueuedBucketRechecks(); friend class BucketDBUpdater_Test; @@ -198,8 +170,7 @@ private: class BucketListGenerator { public: - BucketListGenerator(uint16_t node, - BucketListMerger::BucketList& entries) + BucketListGenerator(uint16_t node, BucketListMerger::BucketList& entries) : _node(node), _entries(entries) {}; bool process(BucketDatabase::Entry&); @@ -237,8 +208,7 @@ private: return _removedBuckets; } private: - void setCopiesInEntry(BucketDatabase::Entry& e, - const std::vector& copies) const; + void setCopiesInEntry(BucketDatabase::Entry& e, const std::vector& copies) const; void removeEmptyBucket(const document::BucketId& bucketId); const lib::ClusterState _oldState; -- cgit v1.2.3