Address code review feedback

- don't state next tag in comments, we'll use `reserved` instead
- restructure bucket info to version the checksum and not the info itself
- add some comments

7 files changed, 30 insertions, 82 deletions

diff --git a/storageapi/src/tests/mbusprot/storageprotocoltest.cpp b/storageapi/src/tests/mbusprot/storageprotocoltest.cpp
index 52f988d22b3..57974691253 100644
--- a/storageapi/src/tests/mbusprot/storageprotocoltest.cpp
+++ b/storageapi/src/tests/mbusprot/storageprotocoltest.cpp
@@ -57,11 +57,6 @@ struct StorageProtocolTest : TestWithParam<vespalib::Version> {
     document::Bucket  _bucket;
     document::BucketId _dummy_remap_bucket{17, 12345};
     BucketInfo _dummy_bucket_info{1,2,3,4,5, true, false, 48};
-    vespalib::Version _version5_0{5, 0, 12};
-    vespalib::Version _version5_1{5, 1, 0};
-    vespalib::Version _version5_2{5, 93, 30};
-    vespalib::Version _version6_0{6, 240, 0};
-    vespalib::Version _version7_0{7, 0, 0}; // FIXME
     documentapi::LoadTypeSet _loadTypes;
     mbusprot::StorageProtocol _protocol;
     static auto constexpr CONDITION_STRING = "There's just one condition";
diff --git a/storageapi/src/vespa/storageapi/mbusprot/legacyprotocolserialization.h b/storageapi/src/vespa/storageapi/mbusprot/legacyprotocolserialization.h
index 0c5f786589c..ef4a6b28749 100644
--- a/storageapi/src/vespa/storageapi/mbusprot/legacyprotocolserialization.h
+++ b/storageapi/src/vespa/storageapi/mbusprot/legacyprotocolserialization.h
@@ -5,6 +5,11 @@
 
 namespace storage::mbusprot {
 
+/*
+ * Utility base class for pre-v7 (protobuf) serialization implementations.
+ *
+ * TODO remove on Vespa 8 alongside legacy serialization formats.
+ */
 class LegacyProtocolSerialization : public ProtocolSerialization {
     const std::shared_ptr<const document::DocumentTypeRepo> _repo;
 public:
diff --git a/storageapi/src/vespa/storageapi/mbusprot/protobuf/common.proto b/storageapi/src/vespa/storageapi/mbusprot/protobuf/common.proto
index 2745fcc28b0..d641449995d 100644
--- a/storageapi/src/vespa/storageapi/mbusprot/protobuf/common.proto
+++ b/storageapi/src/vespa/storageapi/mbusprot/protobuf/common.proto
@@ -8,53 +8,30 @@ package storage.mbusprot.protobuf;
 // Note: we use a *Request/*Response naming convention rather than *Command/*Reply,
 // as the former is the gRPC convention and that's where we intend to move.
 
-// Next tag to use: 2
 message BucketSpace {
    uint64 space_id = 1;
 }
 
-// Next tag to use: 2
 message BucketId {
    fixed64 raw_id = 1;
 }
 
-// Next tag to use: 3
 message Bucket {
     uint64  space_id      = 1;
     fixed64 raw_bucket_id = 2;
 }
 
-// Next tag to use: 9
-message BucketInfoV1 {
-    uint64  last_modified_timestamp = 1;
-    // TODO version the checksum instead?
-    fixed32 checksum       = 2;
-    uint32  doc_count      = 3;
-    uint32  total_doc_size = 4;
-    uint32  meta_count     = 5;
-    uint32  used_file_size = 6;
-    bool    ready          = 7;
-    bool    active         = 8;
-}
-
-// Next tag to use: 10
-message BucketInfoV2 {
-    uint64  last_modified_timestamp = 1;
-    // TODO version the checksum instead?
-    fixed64 checksum_lo    = 2;
-    fixed64 checksum_hi    = 3;
-    uint32  doc_count      = 4;
-    uint32  total_doc_size = 5;
-    uint32  meta_count     = 6;
-    uint32  used_file_size = 7;
-    bool    ready          = 8;
-    bool    active         = 9;
-}
-
 // Next tag to use: 3
 message BucketInfo {
-    BucketInfoV1 info_v1 = 1;
-    BucketInfoV2 info_v2 = 2;
+    uint64  last_modified_timestamp = 1;
+    fixed32 legacy_checksum = 2;
+    // TODO v2 checksum
+    uint32  doc_count       = 3;
+    uint32  total_doc_size  = 4;
+    uint32  meta_count      = 5;
+    uint32  used_file_size  = 6;
+    bool    ready           = 7;
+    bool    active          = 8;
 }
 
 message GlobalId {
@@ -82,8 +59,6 @@ message ResponseHeader {
     uint32 priority            = 4; // Always in range [0, 255]
 }
 
-// TODO extract bucket info response fields to own message!
-
 message Document {
     bytes payload = 1;
 }
diff --git a/storageapi/src/vespa/storageapi/mbusprot/protobuf/feed.proto b/storageapi/src/vespa/storageapi/mbusprot/protobuf/feed.proto
index 66af749b69e..58da24df836 100644
--- a/storageapi/src/vespa/storageapi/mbusprot/protobuf/feed.proto
+++ b/storageapi/src/vespa/storageapi/mbusprot/protobuf/feed.proto
@@ -11,7 +11,6 @@ message TestAndSetCondition {
     bytes selection = 1;
 }
 
-// Next tag to use: 6
 message PutRequest {
     Bucket   bucket                 = 1;
     Document document               = 2;
@@ -20,19 +19,16 @@ message PutRequest {
     TestAndSetCondition condition   = 5;
 }
 
-// Next tag to use: 4
 message PutResponse {
     BucketInfo bucket_info        = 1;
     BucketId   remapped_bucket_id = 2;
     bool       was_found          = 3;
 }
 
-// Next tag to use: 2
 message Update {
     bytes payload = 1;
 }
 
-// Next tag to use: 6
 message UpdateRequest {
     Bucket bucket                 = 1;
     Update update                 = 2;
@@ -41,14 +37,12 @@ message UpdateRequest {
     TestAndSetCondition condition = 5;
 }
 
-// Next tag to use: 4
 message UpdateResponse {
     BucketInfo bucket_info        = 1;
     BucketId   remapped_bucket_id = 2;
     uint64     updated_timestamp  = 3;
 }
 
-// Next tag to use: 5
 message RemoveRequest {
     Bucket bucket                 = 1;
     bytes  document_id            = 2;
@@ -56,14 +50,12 @@ message RemoveRequest {
     TestAndSetCondition condition = 4;
 }
 
-// Next tag to use: 4
 message RemoveResponse {
     BucketInfo bucket_info        = 1;
     BucketId   remapped_bucket_id = 2;
     uint64     removed_timestamp  = 3;
 }
 
-// Next tag to use: 5
 message GetRequest {
     Bucket bucket           = 1;
     bytes  document_id      = 2;
@@ -71,7 +63,6 @@ message GetRequest {
     uint64 before_timestamp = 4;
 }
 
-// Next tag to use: 5
 message GetResponse {
     Document   document                = 1;
     uint64     last_modified_timestamp = 2;
@@ -79,25 +70,21 @@ message GetResponse {
     BucketId   remapped_bucket_id      = 4;
 }
 
-// Next tag to use: 3
 message RevertRequest {
     Bucket   bucket               = 1;
     repeated uint64 revert_tokens = 2;
 }
 
-// Next tag to use: 3
 message RevertResponse {
     BucketInfo bucket_info        = 1;
     BucketId   remapped_bucket_id = 2;
 }
 
-// Next tag to use: 3
 message RemoveLocationRequest {
     Bucket bucket             = 1;
     bytes  document_selection = 2;
 }
 
-// Next tag to use: 3
 message RemoveLocationResponse {
     BucketInfo bucket_info        = 1;
     BucketId   remapped_bucket_id = 2;
diff --git a/storageapi/src/vespa/storageapi/mbusprot/protobuf/maintenance.proto b/storageapi/src/vespa/storageapi/mbusprot/protobuf/maintenance.proto
index f5fc94980b5..c4766d2900a 100644
--- a/storageapi/src/vespa/storageapi/mbusprot/protobuf/maintenance.proto
+++ b/storageapi/src/vespa/storageapi/mbusprot/protobuf/maintenance.proto
@@ -7,37 +7,31 @@ package storage.mbusprot.protobuf;
 
 import "common.proto";
 
-// Next tag to use: 3
 message DeleteBucketRequest {
     Bucket     bucket               = 1;
     BucketInfo expected_bucket_info = 2;
 }
 
-// Next tag to use: 3
 message DeleteBucketResponse {
     BucketInfo bucket_info        = 1;
     BucketId   remapped_bucket_id = 2;
 }
 
-// Next tag to use: 3
 message CreateBucketRequest {
     Bucket bucket           = 1;
     bool   create_as_active = 2;
 }
 
-// Next tag to use: 3
 message CreateBucketResponse {
     BucketInfo bucket_info        = 1;
     BucketId   remapped_bucket_id = 2;
 }
 
-// Next tag to use: 3
 message MergeNode {
     uint32 index = 1;
     bool   source_only = 2;
 }
 
-// Next tag to use: 6
 message MergeBucketRequest {
     Bucket             bucket                = 1;
     uint32             cluster_state_version = 2;
@@ -46,7 +40,6 @@ message MergeBucketRequest {
     repeated uint32    node_chain            = 5;
 }
 
-// Next tag to use: 2
 message MergeBucketResponse {
     BucketId remapped_bucket_id = 1;
 }
@@ -108,7 +101,6 @@ message RequestBucketInfoRequest {
         ExplicitBucketSet explicit_bucket_set = 2;
         AllBuckets        all_buckets         = 3;
     }
-    // TODO bucket info version requested
 }
 
 message BucketAndBucketInfo {
diff --git a/storageapi/src/vespa/storageapi/mbusprot/protobuf/visiting.proto b/storageapi/src/vespa/storageapi/mbusprot/protobuf/visiting.proto
index e3f5271599e..89ce39e52a0 100644
--- a/storageapi/src/vespa/storageapi/mbusprot/protobuf/visiting.proto
+++ b/storageapi/src/vespa/storageapi/mbusprot/protobuf/visiting.proto
@@ -7,7 +7,6 @@ package storage.mbusprot.protobuf;
 
 import "common.proto";
 
-
 message ClientVisitorParameter {
     bytes key   = 1;
     bytes value = 2;
diff --git a/storageapi/src/vespa/storageapi/mbusprot/protocolserialization7.cpp b/storageapi/src/vespa/storageapi/mbusprot/protocolserialization7.cpp
index 9352e2a75db..ca77977046c 100644
--- a/storageapi/src/vespa/storageapi/mbusprot/protocolserialization7.cpp
+++ b/storageapi/src/vespa/storageapi/mbusprot/protocolserialization7.cpp
@@ -45,15 +45,14 @@ document::BucketSpace get_bucket_space(const protobuf::BucketSpace& src) {
 }
 
 void set_bucket_info(protobuf::BucketInfo& dest, const api::BucketInfo& src) {
-    auto* info = dest.mutable_info_v1();
-    info->set_last_modified_timestamp(src.getLastModified());
-    info->set_checksum(src.getChecksum());
-    info->set_doc_count(src.getDocumentCount());
-    info->set_total_doc_size(src.getTotalDocumentSize());
-    info->set_meta_count(src.getMetaCount());
-    info->set_used_file_size(src.getUsedFileSize());
-    info->set_active(src.isActive());
-    info->set_ready(src.isReady());
+    dest.set_last_modified_timestamp(src.getLastModified());
+    dest.set_legacy_checksum(src.getChecksum());
+    dest.set_doc_count(src.getDocumentCount());
+    dest.set_total_doc_size(src.getTotalDocumentSize());
+    dest.set_meta_count(src.getMetaCount());
+    dest.set_used_file_size(src.getUsedFileSize());
+    dest.set_active(src.isActive());
+    dest.set_ready(src.isReady());
 }
 
 document::Bucket get_bucket(const protobuf::Bucket& src) {
@@ -62,19 +61,15 @@ document::Bucket get_bucket(const protobuf::Bucket& src) {
 }
 
 api::BucketInfo get_bucket_info(const protobuf::BucketInfo& src) {
-    if (!src.has_info_v1()) {
-        return {};
-    }
     api::BucketInfo info;
-    const auto& s = src.info_v1();
-    info.setLastModified(s.last_modified_timestamp());
-    info.setChecksum(s.checksum());
-    info.setDocumentCount(s.doc_count());
-    info.setTotalDocumentSize(s.total_doc_size());
-    info.setMetaCount(s.meta_count());
-    info.setUsedFileSize(s.used_file_size());
-    info.setActive(s.active());
-    info.setReady(s.ready());
+    info.setLastModified(src.last_modified_timestamp());
+    info.setChecksum(src.legacy_checksum());
+    info.setDocumentCount(src.doc_count());
+    info.setTotalDocumentSize(src.total_doc_size());
+    info.setMetaCount(src.meta_count());
+    info.setUsedFileSize(src.used_file_size());
+    info.setActive(src.active());
+    info.setReady(src.ready());
     return info;
 }