[2441] Bring back ZoneDataLoader's old queueing behavior

This is so that we can avoid merging RdataSets every single time which
can be inefficient.

We still only queue (without flushing) upto the point where a different
owner name is encountered. This is to limit the size of NodeRRsets used.

src/lib/datasrc/memory/zone_data_loader.cc

@@ -49,6 +49,23 @@ typedef boost::function<void(isc::dns::ConstRRsetPtr)> LoadCallback;
 
 // A helper internal class for \c loadZoneData().  make it non-copyable
 // to avoid accidental copy.
+//
+// The current internal implementation no longer expects that both a
+// normal (non RRSIG) RRset and (when signed) its RRSIG are added at
+// once, but we do that here anyway to avoid merging RdataSets every
+// single time which can be inefficient.
+//
+// We hold all RRsets of the same owner name in node_rrsets_ and
+// node_rrsigsets_, and add the matching pairs of RRsets to the zone
+// when we see a new owner name. We do this to limit the size of
+// NodeRRsets below. However, RRsets can occur in any order.
+//
+// The caller is responsible for adding the RRsets of the last group
+// in the input sequence by explicitly calling flushNodeRRsets() at the
+// end.  It's cleaner and more robust if we let the destructor of this class
+// do it, but since we cannot guarantee the adding operation is exception free,
+// we don't choose that option to maintain the common expectation for
+// destructors.
 class ZoneDataLoader : boost::noncopyable {
 public:
     ZoneDataLoader(util::MemorySegment& mem_sgmt,
@@ -58,17 +75,40 @@ public:
     {}
 
     void addFromLoad(const isc::dns::ConstRRsetPtr& rrset);
+    void flushNodeRRsets();
+
+private:
+    typedef std::map<isc::dns::RRType, isc::dns::ConstRRsetPtr> NodeRRsets;
+    typedef NodeRRsets::value_type NodeRRsetsVal;
+
+    // A helper to identify the covered type of an RRSIG.
+    const isc::dns::Name& getCurrentName() const;
 
 private:
+    NodeRRsets node_rrsets_;
+    NodeRRsets node_rrsigsets_;
+    std::vector<isc::dns::ConstRRsetPtr> non_consecutive_rrsets_;
     ZoneDataUpdater updater_;
 };
 
 void
 ZoneDataLoader::addFromLoad(const ConstRRsetPtr& rrset) {
-    if (rrset->getType() == RRType::RRSIG()) {
-        updater_.add(ConstRRsetPtr(), rrset);
-    } else {
-        updater_.add(rrset, ConstRRsetPtr());
+    // If we see a new name, flush the temporary holders, adding the
+    // pairs of RRsets and RRSIGs of the previous name to the zone.
+    if ((!node_rrsets_.empty() || !node_rrsigsets_.empty() ||
+         !non_consecutive_rrsets_.empty()) &&
+        (getCurrentName() != rrset->getName())) {
+        flushNodeRRsets();
+    }
+
+    // Store this RRset until it can be added to the zone. If an rrtype
+    // that's already been seen is found, queue it in a different vector
+    // to be merged later.
+    const bool is_rrsig = rrset->getType() == RRType::RRSIG();
+    NodeRRsets& node_rrsets = is_rrsig ? node_rrsigsets_ : node_rrsets_;
+    const RRType& rrtype = is_rrsig ? getCoveredType(rrset) : rrset->getType();
+    if (!node_rrsets.insert(NodeRRsetsVal(rrtype, rrset)).second) {
+        non_consecutive_rrsets_.insert(non_consecutive_rrsets_.begin(), rrset);
     }
 
     if (rrset->getRRsig()) {
@@ -77,6 +117,50 @@ ZoneDataLoader::addFromLoad(const ConstRRsetPtr& rrset) {
 }
 
 void
+ZoneDataLoader::flushNodeRRsets() {
+    BOOST_FOREACH(NodeRRsetsVal val, node_rrsets_) {
+        // Identify the corresponding RRSIG for the RRset, if any.  If
+        // found add both the RRset and its RRSIG at once.
+        ConstRRsetPtr sig_rrset;
+        NodeRRsets::iterator sig_it = node_rrsigsets_.find(val.first);
+        if (sig_it != node_rrsigsets_.end()) {
+            sig_rrset = sig_it->second;
+            node_rrsigsets_.erase(sig_it);
+        }
+        updater_.add(val.second, sig_rrset);
+    }
+
+    // Normally rrsigsets map should be empty at this point, but it's still
+    // possible that an RRSIG that don't has covered RRset is added; they
+    // still remain in the map.  We add them to the zone separately.
+    BOOST_FOREACH(NodeRRsetsVal val, node_rrsigsets_) {
+        updater_.add(ConstRRsetPtr(), val.second);
+    }
+
+    // Add any non-consecutive rrsets too.
+    BOOST_FOREACH(ConstRRsetPtr rrset, non_consecutive_rrsets_) {
+        if (rrset->getType() == RRType::RRSIG()) {
+            updater_.add(ConstRRsetPtr(), rrset);
+        } else {
+            updater_.add(rrset, ConstRRsetPtr());
+        }
+    }
+
+    node_rrsets_.clear();
+    node_rrsigsets_.clear();
+    non_consecutive_rrsets_.clear();
+}
+
+const Name&
+ZoneDataLoader::getCurrentName() const {
+    if (!node_rrsets_.empty()) {
+        return (node_rrsets_.begin()->second->getName());
+    }
+    assert(!node_rrsigsets_.empty());
+    return (node_rrsigsets_.begin()->second->getName());
+}
+
+void
 logWarning(const dns::Name* zone_name, const dns::RRClass* rrclass,
            const std::string& reason)
 {
@@ -103,6 +187,8 @@ loadZoneDataInternal(util::MemorySegment& mem_sgmt,
 
     ZoneDataLoader loader(mem_sgmt, rrclass, zone_name, *holder.get());
     rrset_installer(boost::bind(&ZoneDataLoader::addFromLoad, &loader, _1));
+    // Add any last RRsets that were left
+    loader.flushNodeRRsets();
 
     const ZoneNode* origin_node = holder.get()->getOriginNode();
     const RdataSet* rdataset = origin_node->getData();

src/lib/datasrc/tests/memory/memory_client_unittest.cc

@@ -256,12 +256,12 @@ TEST_F(MemoryClientTest, loadFromIterator) {
     // First we have the SOA
     ConstRRsetPtr rrset(iterator->getNextRRset());
     EXPECT_TRUE(rrset);
-    EXPECT_EQ(RRType::NS(), rrset->getType());
+    EXPECT_EQ(RRType::SOA(), rrset->getType());
 
     // RRType::NS() RRset
     rrset = iterator->getNextRRset();
     EXPECT_TRUE(rrset);
-    EXPECT_EQ(RRType::SOA(), rrset->getType());
+    EXPECT_EQ(RRType::NS(), rrset->getType());
 
     // RRType::A() RRset
     rrset = iterator->getNextRRset();
@@ -296,7 +296,7 @@ TEST_F(MemoryClientTest, loadFromIterator) {
     // Emulating bogus iterator implementation that passes empty RRSIGs.
     EXPECT_THROW(client_->load(Name("example.org"),
                                *MockIterator::makeIterator(rrset_data, true)),
-                 ZoneDataUpdater::AddError);
+                 isc::Unexpected);
 }
 
 TEST_F(MemoryClientTest, loadMemoryAllocationFailures) {
@@ -380,11 +380,11 @@ TEST_F(MemoryClientTest, loadReloadZone) {
 
     const RdataSet* set = node->getData();
     EXPECT_NE(static_cast<const RdataSet*>(NULL), set);
-    EXPECT_EQ(RRType::NS(), set->type);
+    EXPECT_EQ(RRType::SOA(), set->type);
 
     set = set->getNext();
     EXPECT_NE(static_cast<const RdataSet*>(NULL), set);
-    EXPECT_EQ(RRType::SOA(), set->type);
+    EXPECT_EQ(RRType::NS(), set->type);
 
     set = set->getNext();
     EXPECT_EQ(static_cast<const RdataSet*>(NULL), set);
@@ -409,11 +409,11 @@ TEST_F(MemoryClientTest, loadReloadZone) {
 
     set = node->getData();
     EXPECT_NE(static_cast<const RdataSet*>(NULL), set);
-    EXPECT_EQ(RRType::NS(), set->type);
+    EXPECT_EQ(RRType::SOA(), set->type);
 
     set = set->getNext();
     EXPECT_NE(static_cast<const RdataSet*>(NULL), set);
-    EXPECT_EQ(RRType::SOA(), set->type);
+    EXPECT_EQ(RRType::NS(), set->type);
 
     set = set->getNext();
     EXPECT_EQ(static_cast<const RdataSet*>(NULL), set);
@@ -675,15 +675,15 @@ TEST_F(MemoryClientTest, getIterator) {
     client_->load(Name("example.org"), TEST_DATA_DIR "/example.org-empty.zone");
     ZoneIteratorPtr iterator(client_->getIterator(Name("example.org")));
 
-    // First we have the NS
+    // First we have the SOA
     ConstRRsetPtr rrset(iterator->getNextRRset());
     EXPECT_TRUE(rrset);
-    EXPECT_EQ(RRType::NS(), rrset->getType());
+    EXPECT_EQ(RRType::SOA(), rrset->getType());
 
-    // Then the SOA
+    // Then the NS
     rrset = iterator->getNextRRset();
     EXPECT_TRUE(rrset);
-    EXPECT_EQ(RRType::SOA(), rrset->getType());
+    EXPECT_EQ(RRType::NS(), rrset->getType());
 
     // There's nothing else in this iterator
     EXPECT_EQ(ConstRRsetPtr(), iterator->getNextRRset());
@@ -699,15 +699,15 @@ TEST_F(MemoryClientTest, getIteratorSeparateRRs) {
     // separate_rrs = false
     ZoneIteratorPtr iterator(client_->getIterator(Name("example.org")));
 
-    // First we have the NS
+    // First we have the SOA
     ConstRRsetPtr rrset(iterator->getNextRRset());
     EXPECT_TRUE(rrset);
-    EXPECT_EQ(RRType::NS(), rrset->getType());
+    EXPECT_EQ(RRType::SOA(), rrset->getType());
 
-    // Then, the SOA
+    // Then, the NS
     rrset = iterator->getNextRRset();
     EXPECT_TRUE(rrset);
-    EXPECT_EQ(RRType::SOA(), rrset->getType());
+    EXPECT_EQ(RRType::NS(), rrset->getType());
 
     // Only one RRType::A() RRset
     rrset = iterator->getNextRRset();
@@ -720,15 +720,15 @@ TEST_F(MemoryClientTest, getIteratorSeparateRRs) {
     // separate_rrs = true
     ZoneIteratorPtr iterator2(client_->getIterator(Name("example.org"), true));
 
-    // First we have the NS
+    // First we have the SOA
     rrset = iterator2->getNextRRset();
     EXPECT_TRUE(rrset);
-    EXPECT_EQ(RRType::NS(), rrset->getType());
+    EXPECT_EQ(RRType::SOA(), rrset->getType());
 
-    // Then, the SOA
+    // Then, the NS
     rrset = iterator2->getNextRRset();
     EXPECT_TRUE(rrset);
-    EXPECT_EQ(RRType::SOA(), rrset->getType());
+    EXPECT_EQ(RRType::NS(), rrset->getType());
 
     // First RRType::A() RRset
     rrset = iterator2->getNextRRset();
@@ -803,12 +803,12 @@ TEST_F(MemoryClientTest, findZoneData) {
 
     const RdataSet* set = node->getData();
     EXPECT_NE(static_cast<const RdataSet*>(NULL), set);
-    EXPECT_EQ(RRType::NS(), set->type);
+    EXPECT_EQ(RRType::SOA(), set->type);
 
     /* Check NS */
     set = set->getNext();
     EXPECT_NE(static_cast<const RdataSet*>(NULL), set);
-    EXPECT_EQ(RRType::SOA(), set->type);
+    EXPECT_EQ(RRType::NS(), set->type);
 
     set = set->getNext();
     EXPECT_EQ(static_cast<const RdataSet*>(NULL), set);