diff options
author | Henning Baldersheim <balder@yahoo-inc.com> | 2020-10-29 14:39:49 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2020-10-29 14:39:49 +0100 |
commit | 82e66315db2c47421111e010a3bc9fc4681da616 (patch) | |
tree | d1cb5158e747d8f36846714fed1712de1c8842c8 /storage | |
parent | 330351bc7c0ff1c1bd39b4455f0bac1030c43365 (diff) | |
parent | b81f0401645d8cb1dd5c6373bf16c1fad50adb21 (diff) |
Merge pull request #15074 from vespa-engine/toregge/keep-unmapped-hasmask-bits
Keep hasMask bits for nodes not involved in merge operation step
Diffstat (limited to 'storage')
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. |