[#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

3 files changed, 78 insertions, 12 deletions

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