[3973] Addressed review comments.

src/lib/dhcpsrv/alloc_engine.cc

@@ -1308,7 +1308,9 @@ AllocEngine::reclaimExpiredLeases6(const size_t max_leases, const uint16_t timeo
             // Generate removal name change request for D2, if required.
             // This will return immediatelly if the DNS wasn't updated
             // when the lease was created.
-            queueRemovalNameChangeRequest(lease, *(lease->duid_));
+            if (lease->duid_) {
+                queueRemovalNameChangeRequest(lease, *(lease->duid_));
+            }
 
             // Reclaim the lease - depending on the configuration, set the
             // expired-reclaimed state or simply remove it.

src/lib/dhcpsrv/alloc_engine.h

@@ -716,11 +716,11 @@ private:
     ///
     /// @param lease Pointer to a lease for which NCR should be sent.
     /// @param identifier Identifier to be used to generate DHCID for
-    /// the DNS update. For DHCPv4 it will be hardware address, client
+    /// the DNS update. For DHCPv4 it will be hardware address or client
     /// identifier. For DHCPv6 it will be a DUID.
     ///
     /// @tparam LeasePtrType Pointer to a lease.
-    /// @tparam Identifier HW Address, Client Identifier or DUID.
+    /// @tparam IdentifierType HW Address, Client Identifier or DUID.
     template<typename LeasePtrType, typename IdentifierType>
     void queueRemovalNameChangeRequest(const LeasePtrType& lease,
                                        const IdentifierType& identifier) const;

src/lib/dhcpsrv/alloc_engine_messages.mes

@@ -16,12 +16,12 @@ $NAMESPACE isc::dhcp
 
 % ALLOC_ENGINE_LEASE_RECLAIMED successfully reclaimed lease %1
 This debug message is logged when the allocation engine successfully
-reclaims a lease.
+reclaims a lease. The lease is now available for assignment.
 
 % ALLOC_ENGINE_REMOVAL_NCR_FAILED sending removal name change request failed for lease %1: %2
 This error message is logged when sending a removal name change request
-to D2 failed. This name change request is usually generated when the
-lease reclamation routine acts upon expired leases. If a lease being
+to DHCP DDNS failed. This name change request is usually generated when
+the lease reclamation routine acts upon expired leases. If a lease being
 reclaimed has a corresponding DNS entry it needs to be removed.
 This message indicates that removal of the DNS entry has failed.
 Nevertheless the lease will be reclaimed. 
@@ -71,11 +71,12 @@ while performing the operation on the lease database.
 This debug message is logged when the allocation engine completes
 reclamation of a set of expired leases. The maximum number of leases
 to be reclaimed in a single pass of the lease reclamation routine
-is configurable. However, the number of reclaimed leases may also
-be limited by the timeout value. The message includes the number
-of reclaimed leases and the total time.
+is configurable using 'max-reclaim-leases' parameter. However,
+the number of reclaimed leases may also be limited by the timeout
+value, configured with 'max-reclaim-time'. The message includes the
+number of reclaimed leases and the total time.
 
-% ALLOC_ENGINE_V4_LEASES_RECLAMATION_START starting reclamation of expired leases (limit = %1 leases, timeout = %2 seconds)
+% ALLOC_ENGINE_V4_LEASES_RECLAMATION_START starting reclamation of expired leases (limit = %1 leases or %2 seconds)
 This debug message is issued when the allocation engine starts the
 reclamation of the expired leases. The maximum number of leases to
 be reclaimed and the timeout is included in the message. If any of
@@ -288,11 +289,12 @@ while performing the operation on the lease database.
 This debug message is logged when the allocation engine completes
 reclamation of a set of expired leases. The maximum number of leases
 to be reclaimed in a single pass of the lease reclamation routine
-is configurable. However, the number of reclaimed leases may also
-be limited by the timeout value. The message includes the number
-of reclaimed leases and the total time.
+is configurable using 'max-reclaim-leases' parameter. However,
+the number of reclaimed leases may also be limited by the timeout
+value, configured with 'max-reclaim-time'. The message includes the
+number of reclaimed leases and the total time.
 
-% ALLOC_ENGINE_V6_LEASES_RECLAMATION_START starting reclamation of expired leases (limit = %1 leases, timeout = %2 seconds)
+% ALLOC_ENGINE_V6_LEASES_RECLAMATION_START starting reclamation of expired leases (limit = %1 leases or %2 seconds)
 This debug message is issued when the allocation engine starts the
 reclamation of the expired leases. The maximum number of leases to
 be reclaimed and the timeout is included in the message. If any of

src/lib/dhcpsrv/tests/alloc_engine_expiration_unittest.cc

@@ -102,6 +102,8 @@ struct UpperBound {
 /// - DNS records for some of the leases must be removed when the lease
 ///   is reclaimed and DNS updates are enabled,
 /// - hooks must be invoked (if installed) for each reclaimed lease
+/// - statistics must be updated to increase the number of reclaimed
+///   leases and decrease the number of allocated leases
 ///
 /// The typical test requires many leases to be initialized and stored
 /// in the lease database for the test. The test fixture class creates
@@ -274,6 +276,9 @@ public:
     ///
     /// @param stat_name Statistic name.
     /// @param exp_value Expected value.
+    ///
+    /// @return true if the statistic manager holds a particular value,
+    /// false otherwise.
     bool testStatistics(const std::string& stat_name, const int64_t exp_value) const {
         try {
             ObservationPtr observation = StatsMgr::instance().getObservation(stat_name);
@@ -389,7 +394,8 @@ public:
     /// from the lease.
     ///
     /// @param lease Pointer to lease.
-    /// @return true if lease state is "expired-reclaimed".
+    /// @return true if lease state is "expired-reclaimed" and the FQDN
+    /// information has been removed from the lease.
     static bool leaseReclaimed(const LeasePtrType& lease) {
         return (lease && lease->stateExpiredReclaimed() &&
                 lease->hostname_.empty() && !lease->fqdn_fwd_ &&
@@ -446,7 +452,7 @@ public:
         return (true);
     }
 
-    /// @brief Returns Name Change Request from the D2 client queue.
+    /// @brief Returns removal name change request from the D2 client queue.
     ///
     /// @param lease Pointer to the lease to be matched with NCR.
     ///
@@ -458,7 +464,8 @@ public:
         for (size_t i = 0; i < mgr.getQueueSize(); ++i) {
             const NameChangeRequestPtr& ncr = mgr.peekAt(i);
             // If match found, return true.
-            if (ncr->getIpAddress() == lease->addr_.toText()) {
+            if ((ncr->getIpAddress() == lease->addr_.toText()) &&
+                (ncr->getChangeType() == CHG_REMOVE)) {
                 return (ncr);
             }
         }