From 819ec028962c604fc0e64384213cfe0daabaf794 Mon Sep 17 00:00:00 2001 From: Tor Brede Vekterli Date: Wed, 10 Apr 2019 14:10:20 +0000 Subject: 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 --- .../src/tests/mbusprot/storageprotocoltest.cpp | 5 --- .../mbusprot/legacyprotocolserialization.h | 5 +++ .../storageapi/mbusprot/protobuf/common.proto | 43 +++++----------------- .../vespa/storageapi/mbusprot/protobuf/feed.proto | 13 ------- .../storageapi/mbusprot/protobuf/maintenance.proto | 8 ---- .../storageapi/mbusprot/protobuf/visiting.proto | 1 - .../storageapi/mbusprot/protocolserialization7.cpp | 37 ++++++++----------- 7 files changed, 30 insertions(+), 82 deletions(-) (limited to 'storageapi') 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 { 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 _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; } -- cgit v1.2.3