Keep hasMask bits for nodes not involved in merge operation step.

5 files changed, 35 insertions, 4 deletions

diff --git a/storage/src/tests/persistence/has_mask_remapper_test.cpp b/storage/src/tests/persistence/has_mask_remapper_test.cpp
index 58f6f5b3fe7..de87797fb26 100644
--- a/storage/src/tests/persistence/has_mask_remapper_test.cpp
+++ b/storage/src/tests/persistence/has_mask_remapper_test.cpp
@@ -15,6 +15,7 @@ TEST(HasMaskRemapperTest, test_remap_none)
     for (uint32_t i = 0; i < (1u << merge_operation_nodes.size()); ++i) {
         EXPECT_EQ(i, remap_mask(i));
     }
+    EXPECT_EQ(31u, remap_mask(255u));
 }
 
 TEST(HasMaskRemapperTest, test_remap_subset)
@@ -39,4 +40,18 @@ TEST(HasMaskRemapperTest, test_remap_swapped_subset)
     EXPECT_EQ((std::vector<uint16_t>{0u, 2u, 1u, 3u}), remapped);
 }
 
+TEST(HasMaskRemapperTest, test_keep_unremapped_bits)
+{
+    NodeList reply_nodes{{0, true}, {1, true}, {3, false}};
+    HasMaskRemapper remap_mask(merge_operation_nodes, reply_nodes);
+    EXPECT_EQ(20u, remap_mask(0u, (1u << 5) - 1));
+    EXPECT_EQ(11u, remap_mask((1u << 3) - 1, 0u));
+    EXPECT_EQ(31u, remap_mask((1u << 3) - 1, (1u << 5) - 1));
+    EXPECT_EQ(24u, remap_mask(4u, 16u));
+    HasMaskRemapper same_nodes_remap_mask(merge_operation_nodes, merge_operation_nodes);
+    EXPECT_EQ(31u, same_nodes_remap_mask(255u, 0u));
+    EXPECT_EQ(224u, same_nodes_remap_mask(0u, 255u));
+    EXPECT_EQ(255u, same_nodes_remap_mask(255u, 255u));
+}
+
 }
diff --git a/storage/src/tests/persistence/mergehandlertest.cpp b/storage/src/tests/persistence/mergehandlertest.cpp
index 7dad8260492..02527883022 100644
--- a/storage/src/tests/persistence/mergehandlertest.cpp
+++ b/storage/src/tests/persistence/mergehandlertest.cpp
@@ -1078,6 +1078,8 @@ TEST_F(MergeHandlerTest, remove_from_diff) {
     diff[1]._hasMask = 0x6;
 
     status.diff.insert(status.diff.end(), diff.begin(), diff.end());
+    using NodeList = decltype(_nodes);
+    status.nodeList = NodeList{{0, true}, {1, true}, {2, true}};
 
     {
         std::vector<api::ApplyBucketDiffCommand::Entry> applyDiff(2);
diff --git a/storage/src/vespa/storage/persistence/filestorage/has_mask_remapper.cpp b/storage/src/vespa/storage/persistence/filestorage/has_mask_remapper.cpp
index 5aea3f94a57..9a9265bb03e 100644
--- a/storage/src/vespa/storage/persistence/filestorage/has_mask_remapper.cpp
+++ b/storage/src/vespa/storage/persistence/filestorage/has_mask_remapper.cpp
@@ -8,7 +8,8 @@ namespace storage {
 
 HasMaskRemapper::HasMaskRemapper(const std::vector<api::MergeBucketCommand::Node> &all_nodes,
                                  const std::vector<api::MergeBucketCommand::Node> &nodes)
-    : _mask_remap()
+    : _mask_remap(),
+      _all_remapped(0u)
 {
     if (nodes != all_nodes) {
         vespalib::hash_map<uint32_t, uint32_t> node_index_to_mask(all_nodes.size());
@@ -22,7 +23,10 @@ HasMaskRemapper::HasMaskRemapper(const std::vector<api::MergeBucketCommand::Node
             mask = node_index_to_mask[node.index];
             assert(mask != 0u);
             _mask_remap.push_back(mask);
+            _all_remapped |= mask;
         }
+    } else {
+        _all_remapped = ((1u << all_nodes.size()) - 1);
     }
 }
 
@@ -39,8 +43,16 @@ HasMaskRemapper::operator()(uint16_t mask) const
             }
         }
         mask = new_mask;
+    } else {
+        mask &= _all_remapped;
     }
     return mask;
 }
 
+uint16_t
+HasMaskRemapper::operator()(uint16_t mask, uint16_t keep_from_full_mask) const
+{
+    return (operator()(mask) | (keep_from_full_mask & ~_all_remapped));
+}
+
 }
diff --git a/storage/src/vespa/storage/persistence/filestorage/has_mask_remapper.h b/storage/src/vespa/storage/persistence/filestorage/has_mask_remapper.h
index 94a97ed943a..6daf4bc8e75 100644
--- a/storage/src/vespa/storage/persistence/filestorage/has_mask_remapper.h
+++ b/storage/src/vespa/storage/persistence/filestorage/has_mask_remapper.h
@@ -12,6 +12,7 @@ namespace storage {
 class HasMaskRemapper
 {
     std::vector<uint16_t> _mask_remap;
+    uint16_t _all_remapped;
 
 public:
     HasMaskRemapper(const std::vector<api::MergeBucketCommand::Node> &all_nodes,
@@ -19,6 +20,7 @@ public:
     ~HasMaskRemapper();
 
     uint16_t operator()(uint16_t mask) const;
+    uint16_t operator()(uint16_t mask, uint16_t keep_from_full_mask) const;
 };
 
 }
diff --git a/storage/src/vespa/storage/persistence/filestorage/mergestatus.cpp b/storage/src/vespa/storage/persistence/filestorage/mergestatus.cpp
index b5f493cc921..2ecef59b567 100644
--- a/storage/src/vespa/storage/persistence/filestorage/mergestatus.cpp
+++ b/storage/src/vespa/storage/persistence/filestorage/mergestatus.cpp
@@ -54,7 +54,7 @@ MergeStatus::removeFromDiff(
             // time around, or if no copies have that doc anymore. (Can happen
             // due to reverting or corruption)
             if (it2->_entry._hasMask == hasMask
-                || it2->_entry._hasMask == 0)
+                || ((it2->_entry._hasMask == 0) && (remap_mask(0u, it->_hasMask) == 0u)))
             {
                 if (it2->_entry._hasMask == 0) {
                     LOG(debug, "Merge entry %s no longer exists on any nodes",
@@ -73,9 +73,9 @@ MergeStatus::removeFromDiff(
                 /* 
                  * Remap from per-reply mask for the ApplyBucketDiffReply to a
                  * per-merge-operation mask with same bit assignment as _hasMask in
-                 * the diff vector.
+                 * the diff vector. Keep bits for nodes not involved in reply.
                  */
-                uint16_t mask = remap_mask(it2->_entry._hasMask);
+                uint16_t mask = remap_mask(it2->_entry._hasMask, it->_hasMask);
                 if (mask != it->_hasMask) {
                     // Hasmasks have changed, meaning bucket contents changed on
                     // one or more of the nodes during merging.