Pass document::Bucket to AbortBucketOperationsCommand::shouldAbort method.

6 files changed, 39 insertions, 48 deletions

diff --git a/storage/src/tests/persistence/filestorage/operationabortingtest.cpp b/storage/src/tests/persistence/filestorage/operationabortingtest.cpp
index aab0aa2c0fb..d4b749772a2 100644
--- a/storage/src/tests/persistence/filestorage/operationabortingtest.cpp
+++ b/storage/src/tests/persistence/filestorage/operationabortingtest.cpp
@@ -193,13 +193,37 @@ OperationAbortingTest::validateReplies(
 
 namespace {
 
+class ExplicitBucketSetPredicate : public AbortBucketOperationsCommand::AbortPredicate {
+    using BucketSet = vespalib::hash_set<document::BucketId, document::BucketId::hash>;
+    BucketSet _bucketsToAbort;
+
+    bool doShouldAbort(const document::Bucket &bucket) const override;
+public:
+    ~ExplicitBucketSetPredicate();
+
+    template <typename Iterator>
+    ExplicitBucketSetPredicate(Iterator first, Iterator last)
+        : _bucketsToAbort(first, last)
+    { }
+
+    const BucketSet& getBucketsToAbort() const {
+        return _bucketsToAbort;
+    }
+};
+
+bool
+ExplicitBucketSetPredicate::doShouldAbort(const document::Bucket &bucket) const {
+    return _bucketsToAbort.find(bucket.getBucketId()) != _bucketsToAbort.end();
+}
+
+ExplicitBucketSetPredicate::~ExplicitBucketSetPredicate() { }
+
 template <typename Container>
 AbortBucketOperationsCommand::SP
 makeAbortCmd(const Container& buckets)
 {
     std::unique_ptr<AbortBucketOperationsCommand::AbortPredicate> pred(
-            new AbortBucketOperationsCommand::ExplicitBucketSetPredicate(
-                    buckets.begin(), buckets.end()));
+            new ExplicitBucketSetPredicate(buckets.begin(), buckets.end()));
     AbortBucketOperationsCommand::SP cmd(
             new AbortBucketOperationsCommand(std::move(pred)));
     return cmd;
diff --git a/storage/src/tests/storageserver/changedbucketownershiphandlertest.cpp b/storage/src/tests/storageserver/changedbucketownershiphandlertest.cpp
index 732aff5faf6..18e93d00494 100644
--- a/storage/src/tests/storageserver/changedbucketownershiphandlertest.cpp
+++ b/storage/src/tests/storageserver/changedbucketownershiphandlertest.cpp
@@ -197,7 +197,7 @@ bool
 hasAbortedAllOf(const AbortBucketOperationsCommand::SP& cmd, const Vec& v)
 {
     for (auto& b : v) {
-        if (!cmd->shouldAbort(b)) {
+        if (!cmd->shouldAbort(makeDocumentBucket(b))) {
             return false;
         }
     }
@@ -209,7 +209,7 @@ bool
 hasAbortedNoneOf(const AbortBucketOperationsCommand::SP& cmd, const Vec& v)
 {
     for (auto& b : v) {
-        if (cmd->shouldAbort(b)) {
+        if (cmd->shouldAbort(makeDocumentBucket(b))) {
             return false;
         }
     }
diff --git a/storage/src/vespa/storage/persistence/filestorage/filestorhandlerimpl.cpp b/storage/src/vespa/storage/persistence/filestorage/filestorhandlerimpl.cpp
index 718c36b78d4..fffd559866d 100644
--- a/storage/src/vespa/storage/persistence/filestorage/filestorhandlerimpl.cpp
+++ b/storage/src/vespa/storage/persistence/filestorage/filestorhandlerimpl.cpp
@@ -337,7 +337,7 @@ FileStorHandlerImpl::abortQueuedCommandsForBuckets(
                                 "bucket operation was bound to");
     for (iter_t it(t.queue.begin()), e(t.queue.end()); it != e;) {
         api::StorageMessage& msg(*it->_command);
-        if (messageMayBeAborted(msg) && cmd.shouldAbort(it->_bucket.getBucketId())) {
+        if (messageMayBeAborted(msg) && cmd.shouldAbort(it->_bucket)) {
             LOG(debug,
                 "Aborting operation %s as it is bound for bucket %s",
                 msg.toString().c_str(),
@@ -360,7 +360,7 @@ FileStorHandlerImpl::diskHasActiveOperationForAbortedBucket(
         const AbortBucketOperationsCommand& cmd) const
 {
     for (auto& lockedBucket : disk.lockedBuckets) {
-        if (cmd.shouldAbort(lockedBucket.first.getBucketId())) {
+        if (cmd.shouldAbort(lockedBucket.first)) {
             LOG(spam,
                 "Disk had active operation for aborted bucket %s, "
                 "waiting for it to complete...",
diff --git a/storage/src/vespa/storage/persistence/messages.cpp b/storage/src/vespa/storage/persistence/messages.cpp
index 78f4b02a192..876067e4e15 100644
--- a/storage/src/vespa/storage/persistence/messages.cpp
+++ b/storage/src/vespa/storage/persistence/messages.cpp
@@ -151,17 +151,6 @@ RecheckBucketInfoCommand::makeReply() {
     return std::make_unique<RecheckBucketInfoReply>(*this);
 }
 
-bool
-AbortBucketOperationsCommand::ExplicitBucketSetPredicate::doShouldAbort(const document::BucketId& bid) const {
-    return _bucketsToAbort.find(bid) != _bucketsToAbort.end();
-}
-
-AbortBucketOperationsCommand::ExplicitBucketSetPredicate::ExplicitBucketSetPredicate(const BucketSet& bucketsToAbort)
-    : _bucketsToAbort(bucketsToAbort)
-{ }
-
-AbortBucketOperationsCommand::ExplicitBucketSetPredicate::~ExplicitBucketSetPredicate() { }
-
 AbortBucketOperationsCommand::AbortBucketOperationsCommand(std::unique_ptr<AbortPredicate> predicate)
     : api::InternalCommand(ID),
     _predicate(std::move(predicate))
diff --git a/storage/src/vespa/storage/persistence/messages.h b/storage/src/vespa/storage/persistence/messages.h
index d47a389a8c1..7bbeea8a12a 100644
--- a/storage/src/vespa/storage/persistence/messages.h
+++ b/storage/src/vespa/storage/persistence/messages.h
@@ -206,33 +206,11 @@ class AbortBucketOperationsCommand : public api::InternalCommand
 {
 public:
     class AbortPredicate {
-        virtual bool doShouldAbort(const document::BucketId&) const = 0;
+        virtual bool doShouldAbort(const document::Bucket&) const = 0;
     public:
         virtual ~AbortPredicate() {}
-        bool shouldAbort(const document::BucketId& bid) const {
-            return doShouldAbort(bid);
-        }
-    };
-
-    using BucketSet = vespalib::hash_set<document::BucketId, document::BucketId::hash>;
-
-    // Primarily for unit test mocking; actual predicate impl should do lazy
-    // evaluations based on previous and current cluster states.
-    class ExplicitBucketSetPredicate : public AbortPredicate {
-        BucketSet _bucketsToAbort;
-
-        bool doShouldAbort(const document::BucketId& bid) const override;
-    public:
-        explicit ExplicitBucketSetPredicate(const BucketSet& bucketsToAbort);
-        ~ExplicitBucketSetPredicate();
-
-        template <typename Iterator>
-        ExplicitBucketSetPredicate(Iterator first, Iterator last)
-            : _bucketsToAbort(first, last)
-        { }
-
-        const BucketSet& getBucketsToAbort() const {
-            return _bucketsToAbort;
+        bool shouldAbort(const document::Bucket &bucket) const {
+            return doShouldAbort(bucket);
         }
     };
 
@@ -245,8 +223,8 @@ public:
     AbortBucketOperationsCommand(std::unique_ptr<AbortPredicate> predicate);
     ~AbortBucketOperationsCommand();
 
-    bool shouldAbort(const document::BucketId& bid) const {
-        return _predicate->shouldAbort(bid);
+    bool shouldAbort(const document::Bucket &bucket) const {
+        return _predicate->shouldAbort(bucket);
     }
 
     std::unique_ptr<api::StorageReply> makeReply() override;
diff --git a/storage/src/vespa/storage/storageserver/changedbucketownershiphandler.cpp b/storage/src/vespa/storage/storageserver/changedbucketownershiphandler.cpp
index e981e29e806..4b624199459 100644
--- a/storage/src/vespa/storage/storageserver/changedbucketownershiphandler.cpp
+++ b/storage/src/vespa/storage/storageserver/changedbucketownershiphandler.cpp
@@ -155,15 +155,15 @@ class StateDiffLazyAbortPredicate
     // where all distributors are down.
     bool _allDistributorsHaveGoneDown;
 
-    bool doShouldAbort(const document::BucketId& b) const override {
+    bool doShouldAbort(const document::Bucket &bucket) const override {
         if (_allDistributorsHaveGoneDown) {
             return true;
         }
-        uint16_t oldOwner(_oldState.ownerOf(b));
-        uint16_t newOwner(_newState.ownerOf(b));
+        uint16_t oldOwner(_oldState.ownerOf(bucket.getBucketId()));
+        uint16_t newOwner(_newState.ownerOf(bucket.getBucketId()));
         if (oldOwner != newOwner) {
             LOG(spam, "Owner of %s was %u, now %u. Operation should be aborted",
-                b.toString().c_str(), oldOwner, newOwner);
+                bucket.toString().c_str(), oldOwner, newOwner);
             return true;
         }
         return false;