From: Thomas Markwalder Date: Fri, 22 Mar 2024 22:50:04 +0000 (+0000) Subject: [#3295] Improve pickNextJob efficiency X-Git-Tag: Kea-2.5.8~105 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=6974f666516914586ad9c792f464f07617be4959;p=thirdparty%2Fkea.git [#3295] Improve pickNextJob efficiency src/bin/d2/d2_update_mgr.* D2UpdateMgr::pickNextJob() - check return of makeTransction() D2UpdateMgr::makeTransaction() - return true if transaction created src/bin/d2/tests/d2_update_mgr_unittests.cc TEST_F(D2UpdateMgrTest, pickNextJobSkips) - new test --- diff --git a/src/bin/d2/d2_update_mgr.cc b/src/bin/d2/d2_update_mgr.cc index 000aefee86..6be805b473 100644 --- a/src/bin/d2/d2_update_mgr.cc +++ b/src/bin/d2/d2_update_mgr.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2013-2023 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2013-2024 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -100,17 +100,26 @@ D2UpdateMgr::checkFinishedTransactions() { void D2UpdateMgr::pickNextJob() { // Start at the front of the queue, looking for the first entry for // which no transaction is in progress. If we find an eligible entry - // remove it from the queue and make a transaction for it. + // remove it from the queue and make a transaction for it. If the + // transaction creation fails try the next entry in the queue. // Requests and transactions are associated by DHCID. If a request has // the same DHCID as a transaction, they are presumed to be for the same // "end user". size_t queue_count = getQueueCount(); - for (size_t index = 0; index < queue_count; ++index) { + for (size_t index = 0; index < queue_count; ) { dhcp_ddns::NameChangeRequestPtr found_ncr = queue_mgr_->peekAt(index); - if (!hasTransaction(found_ncr->getDhcid())) { + if (hasTransaction(found_ncr->getDhcid())) { + // Leave it on the queue, move on to the next. + ++index; + } else { + // Dequeue it and try to make transaction for it. queue_mgr_->dequeueAt(index); - makeTransaction(found_ncr); - return; + if (makeTransaction(found_ncr)) { + return; + } + + // One less in the queue. + --queue_count; } } @@ -121,7 +130,7 @@ void D2UpdateMgr::pickNextJob() { .arg(getQueueCount()).arg(getTransactionCount()); } -void +bool D2UpdateMgr::makeTransaction(dhcp_ddns::NameChangeRequestPtr& next_ncr) { // First lets ensure there is not a transaction in progress for this // DHCID. (pickNextJob should ensure this, as it is the only real caller @@ -153,7 +162,7 @@ D2UpdateMgr::makeTransaction(dhcp_ddns::NameChangeRequestPtr& next_ncr) { LOG_ERROR(dhcp_to_d2_logger, DHCP_DDNS_NO_FWD_MATCH_ERROR) .arg(next_ncr->getRequestId()) .arg(next_ncr->toText()); - return; + return (false); } ++direction_count; @@ -179,7 +188,7 @@ D2UpdateMgr::makeTransaction(dhcp_ddns::NameChangeRequestPtr& next_ncr) { LOG_ERROR(dhcp_to_d2_logger, DHCP_DDNS_NO_REV_MATCH_ERROR) .arg(next_ncr->getRequestId()) .arg(next_ncr->toText()); - return; + return (false); } ++direction_count; @@ -193,7 +202,7 @@ D2UpdateMgr::makeTransaction(dhcp_ddns::NameChangeRequestPtr& next_ncr) { DHCP_DDNS_REQUEST_DROPPED) .arg(next_ncr->getRequestId()) .arg(next_ncr->toText()); - return; + return (false); } // We matched to the required servers, so construct the transaction. @@ -257,6 +266,7 @@ D2UpdateMgr::makeTransaction(dhcp_ddns::NameChangeRequestPtr& next_ncr) { // Start it. trans->startTransaction(); + return (true); } TransactionList::iterator diff --git a/src/bin/d2/d2_update_mgr.h b/src/bin/d2/d2_update_mgr.h index 593cfd1167..870440144c 100644 --- a/src/bin/d2/d2_update_mgr.h +++ b/src/bin/d2/d2_update_mgr.h @@ -1,4 +1,4 @@ -// Copyright (C) 2013-2021 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2013-2024 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -149,9 +149,11 @@ protected: /// /// @param ncr the NameChangeRequest for which to create a transaction. /// + /// @return True if a transaction was added, false otherwise. + /// /// @throw D2UpdateMgrError if a transaction for this DHCID already /// exists. Note this would be programmatic error. - void makeTransaction(isc::dhcp_ddns::NameChangeRequestPtr& ncr); + bool makeTransaction(isc::dhcp_ddns::NameChangeRequestPtr& ncr); public: /// @brief Gets the D2UpdateMgr's IOService. diff --git a/src/bin/d2/tests/d2_update_mgr_unittests.cc b/src/bin/d2/tests/d2_update_mgr_unittests.cc index 4a28db5f0f..ec8a2d957b 100644 --- a/src/bin/d2/tests/d2_update_mgr_unittests.cc +++ b/src/bin/d2/tests/d2_update_mgr_unittests.cc @@ -583,6 +583,60 @@ TEST_F(D2UpdateMgrTest, pickNextJob) { EXPECT_EQ(0, update_mgr_->getQueueCount()); } +/// @brief Tests D2UpdateManager's pickNextJob method. +/// This test verifies that pickNextJob skips over requests +/// for which makeTransation() fails. +TEST_F(D2UpdateMgrTest, pickNextJobSkips) { + // Ensure we have at least 4 canned requests with which to work. + ASSERT_TRUE(canned_count_ >= 4); + + // Enqueue a forward change NCR with an FQDN that has no forward match. + dhcp_ddns::NameChangeRequestPtr bogus_ncr; + bogus_ncr.reset(new dhcp_ddns::NameChangeRequest(*(canned_ncrs_[0]))); + bogus_ncr->setForwardChange(true); + bogus_ncr->setReverseChange(false); + bogus_ncr->setFqdn("bogus.forward.domain.com"); + ASSERT_NO_THROW(queue_mgr_->enqueue(bogus_ncr)); + + // Enqueue a reverse change NCR with an FQDN that has no reverse match. + bogus_ncr.reset(new dhcp_ddns::NameChangeRequest(*(canned_ncrs_[0]))); + bogus_ncr->setForwardChange(false); + bogus_ncr->setReverseChange(true); + bogus_ncr->setIpAddress("77.77.77.77"); + ASSERT_NO_THROW(queue_mgr_->enqueue(bogus_ncr)); + + // Put the valid transactions on the queue. + for (int i = 0; i < canned_count_; i++) { + ASSERT_NO_THROW(queue_mgr_->enqueue(canned_ncrs_[i])); + } + + ASSERT_EQ(2 + canned_count_, update_mgr_->getQueueCount()); + ASSERT_EQ(0, update_mgr_->getTransactionCount()); + + // Invoke pickNextJob once. + EXPECT_NO_THROW(update_mgr_->pickNextJob()); + EXPECT_EQ(1, update_mgr_->getTransactionCount()); + EXPECT_TRUE(update_mgr_->hasTransaction(canned_ncrs_[0]->getDhcid())); + + // Verify that the queue has all but one of the canned requests still queued. + EXPECT_EQ(canned_count_ - 1, update_mgr_->getQueueCount()); + + // Empty the queue and transaction list. + queue_mgr_->clearQueue(); + ASSERT_EQ(0, update_mgr_->getQueueCount()); + update_mgr_->clearTransactionList(); + EXPECT_EQ(0, update_mgr_->getTransactionCount()); + + // Add two no match requests. + ASSERT_NO_THROW(queue_mgr_->enqueue(bogus_ncr)); + ASSERT_NO_THROW(queue_mgr_->enqueue(bogus_ncr)); + ASSERT_EQ(2, update_mgr_->getQueueCount()); + + // Invoking pickNextJob() should empty the queue without adding transactions. + EXPECT_NO_THROW(update_mgr_->pickNextJob()); + EXPECT_EQ(0, update_mgr_->getQueueCount()); +} + /// @brief Tests D2UpdateManager's sweep method. /// Since sweep is primarily a wrapper around checkFinishedTransactions and /// pickNextJob, along with checks on maximum transaction limits, it mostly