proto: disable XXX_NoUnkeyedLiteral and XXX_sizecache fields by nvb · Pull Request #38404 · cockroachdb/cockroach

nvb · 2019-06-25T17:30:05Z

The PR removes the XXX_NoUnkeyedLiteral and XXX_sizecache fields from generated protobufs.

XXX_NoUnkeyedLiteral claims to come with a small perf win for proto marshaling, but it looks like it's only used with proto.Marshal and not with the Marshal method (which CRDB code and gRPC code always use). Meanwhile, it has two moderate disadvantages:

it adds 32 bits to every proto struct. This isn't a big deal in most cases, but in code that allocates large slices of little proto structs (like the spanlatch.Manager, which is what tipped me off to this new field), it has a non-negligible cost.
it makes value comparison between proto structs dangerously unreliable. We compare small protos by value all over the place (see hlc.Timestamp) and switching to an Equals method would come with a cost to code complexity and runtime performance.

After removing XXX_NoUnkeyedLiteral, XXX_sizecache is now at the end of generated protos, it is now taking up space even though it is an empty struct: golang/go#9401. Since this is just a glorified lint which is already enforced by go vet's “composites” check, it doesn't seem worth the cost.

Benchmarks

name                    old reads/s   new reads/s   delta
kv95/enc=false/nodes=3    35.5k ± 1%    35.8k ± 1%  +0.86%  (p=0.214 n=5+5)

name                    old writes/s  new writes/s  delta
kv95/enc=false/nodes=3    1.87k ± 2%    1.89k ± 2%  +0.94%  (p=0.214 n=5+5)

cockroach-teamcity · 2019-06-25T17:30:16Z

This change is

ajwerner

Reviewed 27 of 68 files at r1, 68 of 68 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @RaduBerinde)

The field claims to come with a small perf win for proto marshalling, but it looks like it's only used with `proto.Marshal` and not with the `Marshal` method (which CRDB code and gRPC code always use). Meanwhile, the cache has two moderate disadvantages: 1. it adds 32 bits to every proto struct. This isn't a big deal in most cases, but in code that allocates large slices of little proto structs (like the `spanlatch.Manager`, which is what tipped me off to this new field), it has a non-negligible cost. 2. it makes value comparison between proto structs dangerously unreliable. We compare small protos by value all over the place (see `hlc.Timestamp`) and switching to an `Equals` method would come with a cost to code complexity and runtime performance. All in all, the `XXX_sizecache` field doesn't seem to be worth it. Release note: None

Because the field is now at the end of generated protos, it is now taking up space even though it is an empty struct: golang/go#9401 Since this is just a glorified lint which is already enforced by `go vet`'s “composites” check, it doesn't seem worth the cost. Release note: None

nvb · 2019-06-25T21:17:15Z

bors r+

craig · 2019-06-25T21:39:46Z

Build failed

GitHub CI (Cockroach)

nvb · 2019-06-25T21:53:18Z

Canceled (Tests failed: 1 (1 new), passed: 2866, ignored: 15)

With no trace in the logs. Assuming that's a flake because CI passed.

bors r+

38404: proto: disable XXX_NoUnkeyedLiteral and XXX_sizecache fields r=nvanbenschoten a=nvanbenschoten Fixes #37706. The PR removes the `XXX_NoUnkeyedLiteral` and `XXX_sizecache` fields from generated protobufs. `XXX_NoUnkeyedLiteral` claims to come with a small perf win for proto marshaling, but it looks like it's only used with `proto.Marshal` and not with the `Marshal` method (which CRDB code and gRPC code always use). Meanwhile, it has two moderate disadvantages: 1. it adds 32 bits to every proto struct. This isn't a big deal in most cases, but in code that allocates large slices of little proto structs (like the `spanlatch.Manager`, which is what tipped me off to this new field), it has a non-negligible cost. 2. it makes value comparison between proto structs dangerously unreliable. We compare small protos by value all over the place (see `hlc.Timestamp`) and switching to an `Equals` method would come with a cost to code complexity and runtime performance. After removing `XXX_NoUnkeyedLiteral`, `XXX_sizecache` is now at the end of generated protos, it is now taking up space even though it is an empty struct: golang/go#9401. Since this is just a glorified lint which is already enforced by `go vet`'s “composites” check, it doesn't seem worth the cost. ### Benchmarks ``` name old reads/s new reads/s delta kv95/enc=false/nodes=3 35.5k ± 1% 35.8k ± 1% +0.86% (p=0.214 n=5+5) name old writes/s new writes/s delta kv95/enc=false/nodes=3 1.87k ± 2% 1.89k ± 2% +0.94% (p=0.214 n=5+5) ``` Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>

craig · 2019-06-25T22:16:31Z

Build succeeded

GitHub CI (Cockroach)

…he fields in protos This commit removes the `XXX_NoUnkeyedLiteral`, `XXX_unrecognized`, and `XXX_sizecache` auto-generated fields from generated protobuf structs in the raft package. This was done for all of the same reasons CockroachDB removed the generation of these fields in cockroachdb/cockroach#38404. They come with very limited advantages but moderate disadvantages. `XXX_NoUnkeyedLiteral` and `XXX_sizecache` were only enabled recently in cc7b4fa, and this appears to have been unintentional. Meanwhile, `XXX_unrecognized` has been around for longer and has arguably more reason to stay because it can assist with forwards compatibility. However, any real mixed-version upgrade story for this package is mostly untold at this point, and keeping this field seems just as likely to cause unexpected bugs (e.g. a field was propagated but not updated correctly when passed through an old version) as it seems to fix real issues, so it also doesn't warrant its cost. This reduces the in-memory representation size of all Raft protos. Notably, it reduces the memory size of an `Entry` proto from *80 bytes* to *48 bytes* and the memory size of a `Message` proto from *392 bytes* to *264 bytes*. Both of these structs are used frequently, and often in slices, where this wasted space really starts to add up. This was motivated by a regression in microbenchmarks in CockroachDB due to cc7b4fa, which was caught in cockroachdb/cockroach#62212.

* *: add topic Added topic to Varlog. * proto: add Topic into proto (#479) * add topic * add topic into proto * wip * fix CommitContextOf * add highWatermark into report * Update internal/metadata_repository/raft_metadata_repository.go Co-authored-by: jun.song(송인준)/kakao <jun.song@kakaocorp.com> * Update internal/metadata_repository/raft_metadata_repository.go Co-authored-by: jun.song(송인준)/kakao <jun.song@kakaocorp.com> * Update internal/metadata_repository/raft_metadata_repository.go Co-authored-by: jun.song(송인준)/kakao <jun.song@kakaocorp.com> * Update internal/metadata_repository/raft_metadata_repository.go Co-authored-by: jun.song(송인준)/kakao <jun.song@kakaocorp.com> * Update internal/metadata_repository/raft_metadata_repository.go * Update internal/metadata_repository/storage.go Co-authored-by: jun.song(송인준)/kakao <jun.song@kakaocorp.com> * fix code * fix code Co-authored-by: pharrell.jang <pharrell.jang@kakaocorp.com> Co-authored-by: jun.song(송인준)/kakao <jun.song@kakaocorp.com> * *: use int32 for storage node id and log stream id (#481) Changed type of `types.StorageNodeID` and `types.LogStreamID` from uint32 to int32. Resolves [#VARLOG-548](https://jira.daumkakao.com/browse/VARLOG-548). * topic: managemant topic (#485) * add topic * add topic into proto * wip * fix CommitContextOf * add highWatermark into report * wip * management topic * add test for register topic * add test for unregister topic * fmt * fix code * fix test * fix code * fix code Co-authored-by: pharrell.jang <pharrell.jang@kakaocorp.com> * sn: remove redundant types for replica (#483) There were redundant types to represent replica: - `pkg/logc/StorageNode` - `proto/snpb/Replica` - `proto/snpb/AppendRequest_BackupNode` This patch removes those and uses `proto/varlogpb/StorageNode` and types that wrap it. Resolves [#VARLOG-546](https://jira.daumkakao.com/browse/VARLOG-546). * sn: add topic id to log i/o (#486) This patch adds `TopicID` to the methods of Log I/O interface. It doesn't contain any meaningful implementations about `TopicID`. Types that now have `TopicID` are follows: - `internal/storagenode/logio.ReadWriter` - `internal/storagenode/reportcommitter/reportcommitter.Getter` - `proto/snpb.AppendRequest`, `proto/snpb.AppendResponse`, `proto/snpb.ReadRequest`, `proto/snpb.SubscribeRequest`, `proto/snpb.TrimRequest` - `proto/varlogpb.Replica` Resolves [#VARLOG-542](https://jira.daumkakao.com/browse/VARLOG-542). * it: fix flaky test - TestVarlogSubscribeWithAddLS (#487) The `TestVarlogSubscribeWithAddLS` created a goroutine adding new LogStream while appending log entries. It however did not manage the life cycle of the goroutine resulting in several issues: - use closed connection to append logs - wait for commit messages from MR indefinitely since the MR is already closed This patch simply adds `sync.WaitGroup` to the test to avoid the above issues. Resolves [#VARLOG-569](https://jira.daumkakao.com/browse/VARLOG-569). * proto: removing unnecessary fields from messages (#488) This patch removes unnecessary fields generated automatically from proto messages. These lines are like follows: ``` XXX_NoUnkeyedLiteral struct{} `json:"-"` XXX_unrecognized []byte `json:"-"` XXX_sizecache int32 `json:"-"` ``` They are something related to either optimizations or compatibility that are generated by the gogoproto. However, they come to small overhead in many cases. (See cockroachdb/cockroach#38404 and etcd-io/etcd@e51c697#diff-76a35072df72591a656e69cab6f6fa99aa386fd5ace35c9042851eb324ec16b5). This change adds the following options to every proto definition file: ``` option (gogoproto.goproto_unkeyed_all) = false; option (gogoproto.goproto_unrecognized_all) = false; option (gogoproto.goproto_sizecache_all) = false; ``` Resolves [#VARLOG-557](https://jira.daumkakao.com/browse/VARLOG-557). * *: dedup LogEntry types (#489) - Remove `pkg/types.LogEntry`, then use `proto/varlogpb.LogEntry`. Resolves [#VARLOG-558](https://jira.daumkakao.com/browse/VARLOG-558). * vendor: bump Pebble (#490) Resolves #VARLOG-556 * sn: rename AddLogStream RPC (#491) In this patch, the RPC `AddLogStream` renames to `AddLogStreamReplica` to clarify its behavior. The RPC `AddLogStreamReplica` adds a new replica of the given LogStream to the StorageNode. Resolves [#VARLOG-568](https://jira.daumkakao.com/browse/VARLOG-568). * topic: apply Topic into client (#493) * logio: apply topic * add topic test * fix TestMRTopicLastHighWatermark * fix managing log stream status in vms Resolves #VARLOG-559 * all: update golang 1.17.0 (#492) Resolves [#VARLOG-555](https://jira.daumkakao.com/browse/VARLOG-555). * all: fix code style (#494) This patch just fixes code styles. Resolves [#VARLOG-572](https://jira.daumkakao.com/browse/VARLOG-572). * build: use predefined protoc (#496) Resolves [#VARLOG-563](https://jira.daumkakao.com/browse/VARLOG-563). * sn,topic: checking topic id while handling RPCs (#495) This patch adds a feature that the StorageNode checks if the topic id is valid or not. To support this functionality it adds a topicID to a parameter of many functions. The executor does not care about the topicID, rather it will be considered by StorageNode. To do that, StorageNode maintains the executors by using the executorsmap keyed by logStreamTopicID. The logStreamTopicID is a packed type of the LogStreamID and TopicID. Resolves [#VARLOG-542](https://jira.daumkakao.com/browse/VARLOG-542). * lint: fix code style (#497) This is a follow-up PR for VARLOG-572. Resolves [#VARLOG-572](https://jira.daumkakao.com/browse/VARLOG-572). Co-authored-by: pharrell.jang(장형근)/kakao <pharrell.jang@daumkakao.com> Co-authored-by: pharrell.jang <pharrell.jang@kakaocorp.com>

* *: add topic Added topic to Varlog. * proto: add Topic into proto (#479) * add topic * add topic into proto * wip * fix CommitContextOf * add highWatermark into report * Update internal/metadata_repository/raft_metadata_repository.go Co-authored-by: jun.song(송인준)/kakao <jun.song@kakaocorp.com> * Update internal/metadata_repository/raft_metadata_repository.go Co-authored-by: jun.song(송인준)/kakao <jun.song@kakaocorp.com> * Update internal/metadata_repository/raft_metadata_repository.go Co-authored-by: jun.song(송인준)/kakao <jun.song@kakaocorp.com> * Update internal/metadata_repository/raft_metadata_repository.go Co-authored-by: jun.song(송인준)/kakao <jun.song@kakaocorp.com> * Update internal/metadata_repository/raft_metadata_repository.go * Update internal/metadata_repository/storage.go Co-authored-by: jun.song(송인준)/kakao <jun.song@kakaocorp.com> * fix code * fix code Co-authored-by: pharrell.jang <pharrell.jang@kakaocorp.com> Co-authored-by: jun.song(송인준)/kakao <jun.song@kakaocorp.com> * *: use int32 for storage node id and log stream id (#481) Changed type of `types.StorageNodeID` and `types.LogStreamID` from uint32 to int32. Resolves [#VARLOG-548](VARLOG-548). * topic: managemant topic (#485) * add topic * add topic into proto * wip * fix CommitContextOf * add highWatermark into report * wip * management topic * add test for register topic * add test for unregister topic * fmt * fix code * fix test * fix code * fix code Co-authored-by: pharrell.jang <pharrell.jang@kakaocorp.com> * sn: remove redundant types for replica (#483) There were redundant types to represent replica: - `pkg/logc/StorageNode` - `proto/snpb/Replica` - `proto/snpb/AppendRequest_BackupNode` This patch removes those and uses `proto/varlogpb/StorageNode` and types that wrap it. Resolves [#VARLOG-546](VARLOG-546). * sn: add topic id to log i/o (#486) This patch adds `TopicID` to the methods of Log I/O interface. It doesn't contain any meaningful implementations about `TopicID`. Types that now have `TopicID` are follows: - `internal/storagenode/logio.ReadWriter` - `internal/storagenode/reportcommitter/reportcommitter.Getter` - `proto/snpb.AppendRequest`, `proto/snpb.AppendResponse`, `proto/snpb.ReadRequest`, `proto/snpb.SubscribeRequest`, `proto/snpb.TrimRequest` - `proto/varlogpb.Replica` Resolves [#VARLOG-542](VARLOG-542). * it: fix flaky test - TestVarlogSubscribeWithAddLS (#487) The `TestVarlogSubscribeWithAddLS` created a goroutine adding new LogStream while appending log entries. It however did not manage the life cycle of the goroutine resulting in several issues: - use closed connection to append logs - wait for commit messages from MR indefinitely since the MR is already closed This patch simply adds `sync.WaitGroup` to the test to avoid the above issues. Resolves [#VARLOG-569](VARLOG-569). * proto: removing unnecessary fields from messages (#488) This patch removes unnecessary fields generated automatically from proto messages. These lines are like follows: ``` XXX_NoUnkeyedLiteral struct{} `json:"-"` XXX_unrecognized []byte `json:"-"` XXX_sizecache int32 `json:"-"` ``` They are something related to either optimizations or compatibility that are generated by the gogoproto. However, they come to small overhead in many cases. (See cockroachdb/cockroach#38404 and etcd-io/etcd@e51c697#diff-76a35072df72591a656e69cab6f6fa99aa386fd5ace35c9042851eb324ec16b5). This change adds the following options to every proto definition file: ``` option (gogoproto.goproto_unkeyed_all) = false; option (gogoproto.goproto_unrecognized_all) = false; option (gogoproto.goproto_sizecache_all) = false; ``` Resolves [#VARLOG-557](VARLOG-557). * *: dedup LogEntry types (#489) - Remove `pkg/types.LogEntry`, then use `proto/varlogpb.LogEntry`. Resolves [#VARLOG-558](VARLOG-558). * vendor: bump Pebble (#490) Resolves #VARLOG-556 * sn: rename AddLogStream RPC (#491) In this patch, the RPC `AddLogStream` renames to `AddLogStreamReplica` to clarify its behavior. The RPC `AddLogStreamReplica` adds a new replica of the given LogStream to the StorageNode. Resolves [#VARLOG-568](VARLOG-568). * topic: apply Topic into client (#493) * logio: apply topic * add topic test * fix TestMRTopicLastHighWatermark * fix managing log stream status in vms Resolves #VARLOG-559 * all: update golang 1.17.0 (#492) Resolves [#VARLOG-555](VARLOG-555). * all: fix code style (#494) This patch just fixes code styles. Resolves [#VARLOG-572](VARLOG-572). * build: use predefined protoc (#496) Resolves [#VARLOG-563](VARLOG-563). * sn,topic: checking topic id while handling RPCs (#495) This patch adds a feature that the StorageNode checks if the topic id is valid or not. To support this functionality it adds a topicID to a parameter of many functions. The executor does not care about the topicID, rather it will be considered by StorageNode. To do that, StorageNode maintains the executors by using the executorsmap keyed by logStreamTopicID. The logStreamTopicID is a packed type of the LogStreamID and TopicID. Resolves [#VARLOG-542](VARLOG-542). * lint: fix code style (#497) This is a follow-up PR for VARLOG-572. Resolves [#VARLOG-572](VARLOG-572). Co-authored-by: pharrell.jang(장형근)/kakao <pharrell.jang@daumkakao.com> Co-authored-by: pharrell.jang <pharrell.jang@kakaocorp.com>

* *: add topic Added topic to Varlog. * proto: add Topic into proto (#479) * add topic * add topic into proto * wip * fix CommitContextOf * add highWatermark into report * Update internal/metadata_repository/raft_metadata_repository.go Co-authored-by: jun.song(송인준)/kakao <jun.song@kakaocorp.com> * Update internal/metadata_repository/raft_metadata_repository.go Co-authored-by: jun.song(송인준)/kakao <jun.song@kakaocorp.com> * Update internal/metadata_repository/raft_metadata_repository.go Co-authored-by: jun.song(송인준)/kakao <jun.song@kakaocorp.com> * Update internal/metadata_repository/raft_metadata_repository.go Co-authored-by: jun.song(송인준)/kakao <jun.song@kakaocorp.com> * Update internal/metadata_repository/raft_metadata_repository.go * Update internal/metadata_repository/storage.go Co-authored-by: jun.song(송인준)/kakao <jun.song@kakaocorp.com> * fix code * fix code Co-authored-by: pharrell.jang <pharrell.jang@kakaocorp.com> Co-authored-by: jun.song(송인준)/kakao <jun.song@kakaocorp.com> * *: use int32 for storage node id and log stream id (#481) Changed type of `types.StorageNodeID` and `types.LogStreamID` from uint32 to int32. Resolves [#VARLOG-548](VARLOG-548). * topic: managemant topic (#485) * add topic * add topic into proto * wip * fix CommitContextOf * add highWatermark into report * wip * management topic * add test for register topic * add test for unregister topic * fmt * fix code * fix test * fix code * fix code Co-authored-by: pharrell.jang <pharrell.jang@kakaocorp.com> * sn: remove redundant types for replica (#483) There were redundant types to represent replica: - `pkg/logc/StorageNode` - `proto/snpb/Replica` - `proto/snpb/AppendRequest_BackupNode` This patch removes those and uses `proto/varlogpb/StorageNode` and types that wrap it. Resolves [#VARLOG-546](VARLOG-546). * sn: add topic id to log i/o (#486) This patch adds `TopicID` to the methods of Log I/O interface. It doesn't contain any meaningful implementations about `TopicID`. Types that now have `TopicID` are follows: - `internal/storagenode/logio.ReadWriter` - `internal/storagenode/reportcommitter/reportcommitter.Getter` - `proto/snpb.AppendRequest`, `proto/snpb.AppendResponse`, `proto/snpb.ReadRequest`, `proto/snpb.SubscribeRequest`, `proto/snpb.TrimRequest` - `proto/varlogpb.Replica` Resolves [#VARLOG-542](VARLOG-542). * it: fix flaky test - TestVarlogSubscribeWithAddLS (#487) The `TestVarlogSubscribeWithAddLS` created a goroutine adding new LogStream while appending log entries. It however did not manage the life cycle of the goroutine resulting in several issues: - use closed connection to append logs - wait for commit messages from MR indefinitely since the MR is already closed This patch simply adds `sync.WaitGroup` to the test to avoid the above issues. Resolves [#VARLOG-569](VARLOG-569). * proto: removing unnecessary fields from messages (#488) This patch removes unnecessary fields generated automatically from proto messages. These lines are like follows: ``` XXX_NoUnkeyedLiteral struct{} `json:"-"` XXX_unrecognized []byte `json:"-"` XXX_sizecache int32 `json:"-"` ``` They are something related to either optimizations or compatibility that are generated by the gogoproto. However, they come to small overhead in many cases. (See cockroachdb/cockroach#38404 and etcd-io/etcd@e51c697#diff-76a35072df72591a656e69cab6f6fa99aa386fd5ace35c9042851eb324ec16b5). This change adds the following options to every proto definition file: ``` option (gogoproto.goproto_unkeyed_all) = false; option (gogoproto.goproto_unrecognized_all) = false; option (gogoproto.goproto_sizecache_all) = false; ``` Resolves [#VARLOG-557](VARLOG-557). * *: dedup LogEntry types (#489) - Remove `pkg/types.LogEntry`, then use `proto/varlogpb.LogEntry`. Resolves [#VARLOG-558](VARLOG-558). * vendor: bump Pebble (#490) Resolves #VARLOG-556 * sn: rename AddLogStream RPC (#491) In this patch, the RPC `AddLogStream` renames to `AddLogStreamReplica` to clarify its behavior. The RPC `AddLogStreamReplica` adds a new replica of the given LogStream to the StorageNode. Resolves [#VARLOG-568](VARLOG-568). * topic: apply Topic into client (#493) * logio: apply topic * add topic test * fix TestMRTopicLastHighWatermark * fix managing log stream status in vms Resolves #VARLOG-559 * all: update golang 1.17.0 (#492) Resolves [#VARLOG-555](VARLOG-555). * all: fix code style (#494) This patch just fixes code styles. Resolves [#VARLOG-572](VARLOG-572). * build: use predefined protoc (#496) Resolves [#VARLOG-563](VARLOG-563). * sn,topic: checking topic id while handling RPCs (#495) This patch adds a feature that the StorageNode checks if the topic id is valid or not. To support this functionality it adds a topicID to a parameter of many functions. The executor does not care about the topicID, rather it will be considered by StorageNode. To do that, StorageNode maintains the executors by using the executorsmap keyed by logStreamTopicID. The logStreamTopicID is a packed type of the LogStreamID and TopicID. Resolves [#VARLOG-542](VARLOG-542). * lint: fix code style (#497) This is a follow-up PR for VARLOG-572. Resolves [#VARLOG-572](VARLOG-572). Co-authored-by: pharrell.jang(장형근)/kakao <pharrell.jang@kakaocorp.com> Co-authored-by: pharrell.jang <pharrell.jang@kakaocorp.com>

* *: add topic Added topic to Varlog. * proto: add Topic into proto (#479) * add topic * add topic into proto * wip * fix CommitContextOf * add highWatermark into report * Update internal/metadata_repository/raft_metadata_repository.go Co-authored-by: Injun Song <ijsong@gmail.com> * Update internal/metadata_repository/raft_metadata_repository.go Co-authored-by: Injun Song <ijsong@gmail.com> * Update internal/metadata_repository/raft_metadata_repository.go Co-authored-by: Injun Song <ijsong@gmail.com> * Update internal/metadata_repository/raft_metadata_repository.go Co-authored-by: Injun Song <ijsong@gmail.com> * Update internal/metadata_repository/raft_metadata_repository.go * Update internal/metadata_repository/storage.go Co-authored-by: Injun Song <ijsong@gmail.com> * fix code * fix code Co-authored-by: Hyungkeun Jang <pharrell.jang@kakaocorp.com> Co-authored-by: Injun Song <ijsong@gmail.com> * *: use int32 for storage node id and log stream id (#481) Changed type of `types.StorageNodeID` and `types.LogStreamID` from uint32 to int32. Resolves [#VARLOG-548](VARLOG-548). * topic: managemant topic (#485) * add topic * add topic into proto * wip * fix CommitContextOf * add highWatermark into report * wip * management topic * add test for register topic * add test for unregister topic * fmt * fix code * fix test * fix code * fix code Co-authored-by: Hyungkeun Jang <pharrell.jang@kakaocorp.com> * sn: remove redundant types for replica (#483) There were redundant types to represent replica: - `pkg/logc/StorageNode` - `proto/snpb/Replica` - `proto/snpb/AppendRequest_BackupNode` This patch removes those and uses `proto/varlogpb/StorageNode` and types that wrap it. Resolves [#VARLOG-546](VARLOG-546). * sn: add topic id to log i/o (#486) This patch adds `TopicID` to the methods of Log I/O interface. It doesn't contain any meaningful implementations about `TopicID`. Types that now have `TopicID` are follows: - `internal/storagenode/logio.ReadWriter` - `internal/storagenode/reportcommitter/reportcommitter.Getter` - `proto/snpb.AppendRequest`, `proto/snpb.AppendResponse`, `proto/snpb.ReadRequest`, `proto/snpb.SubscribeRequest`, `proto/snpb.TrimRequest` - `proto/varlogpb.Replica` Resolves [#VARLOG-542](VARLOG-542). * it: fix flaky test - TestVarlogSubscribeWithAddLS (#487) The `TestVarlogSubscribeWithAddLS` created a goroutine adding new LogStream while appending log entries. It however did not manage the life cycle of the goroutine resulting in several issues: - use closed connection to append logs - wait for commit messages from MR indefinitely since the MR is already closed This patch simply adds `sync.WaitGroup` to the test to avoid the above issues. Resolves [#VARLOG-569](VARLOG-569). * proto: removing unnecessary fields from messages (#488) This patch removes unnecessary fields generated automatically from proto messages. These lines are like follows: ``` XXX_NoUnkeyedLiteral struct{} `json:"-"` XXX_unrecognized []byte `json:"-"` XXX_sizecache int32 `json:"-"` ``` They are something related to either optimizations or compatibility that are generated by the gogoproto. However, they come to small overhead in many cases. (See cockroachdb/cockroach#38404 and etcd-io/etcd@e51c697#diff-76a35072df72591a656e69cab6f6fa99aa386fd5ace35c9042851eb324ec16b5). This change adds the following options to every proto definition file: ``` option (gogoproto.goproto_unkeyed_all) = false; option (gogoproto.goproto_unrecognized_all) = false; option (gogoproto.goproto_sizecache_all) = false; ``` Resolves [#VARLOG-557](VARLOG-557). * *: dedup LogEntry types (#489) - Remove `pkg/types.LogEntry`, then use `proto/varlogpb.LogEntry`. Resolves [#VARLOG-558](VARLOG-558). * vendor: bump Pebble (#490) Resolves #VARLOG-556 * sn: rename AddLogStream RPC (#491) In this patch, the RPC `AddLogStream` renames to `AddLogStreamReplica` to clarify its behavior. The RPC `AddLogStreamReplica` adds a new replica of the given LogStream to the StorageNode. Resolves [#VARLOG-568](VARLOG-568). * topic: apply Topic into client (#493) * logio: apply topic * add topic test * fix TestMRTopicLastHighWatermark * fix managing log stream status in vms Resolves #VARLOG-559 * all: update golang 1.17.0 (#492) Resolves [#VARLOG-555](VARLOG-555). * all: fix code style (#494) This patch just fixes code styles. Resolves [#VARLOG-572](VARLOG-572). * build: use predefined protoc (#496) Resolves [#VARLOG-563](VARLOG-563). * sn,topic: checking topic id while handling RPCs (#495) This patch adds a feature that the StorageNode checks if the topic id is valid or not. To support this functionality it adds a topicID to a parameter of many functions. The executor does not care about the topicID, rather it will be considered by StorageNode. To do that, StorageNode maintains the executors by using the executorsmap keyed by logStreamTopicID. The logStreamTopicID is a packed type of the LogStreamID and TopicID. Resolves [#VARLOG-542](VARLOG-542). * lint: fix code style (#497) This is a follow-up PR for VARLOG-572. Resolves [#VARLOG-572](VARLOG-572). Co-authored-by: pharrell.jang(장형근)/kakao <pharrell.jang@kakaocorp.com> Co-authored-by: Hyungkeun Jang <pharrell.jang@kakaocorp.com>

* *: add topic Added topic to Varlog. * proto: add Topic into proto (#479) * add topic * add topic into proto * wip * fix CommitContextOf * add highWatermark into report * Update internal/metadata_repository/raft_metadata_repository.go Co-authored-by: Injun Song <ijsong@gmail.com> * Update internal/metadata_repository/raft_metadata_repository.go Co-authored-by: Injun Song <ijsong@gmail.com> * Update internal/metadata_repository/raft_metadata_repository.go Co-authored-by: Injun Song <ijsong@gmail.com> * Update internal/metadata_repository/raft_metadata_repository.go Co-authored-by: Injun Song <ijsong@gmail.com> * Update internal/metadata_repository/raft_metadata_repository.go * Update internal/metadata_repository/storage.go Co-authored-by: Injun Song <ijsong@gmail.com> * fix code * fix code Co-authored-by: Hyungkeun Jang <pharrell.jang@kakaocorp.com> Co-authored-by: Injun Song <ijsong@gmail.com> * *: use int32 for storage node id and log stream id (#481) Changed type of `types.StorageNodeID` and `types.LogStreamID` from uint32 to int32. Resolves [#VARLOG-548](VARLOG-548). * topic: managemant topic (#485) * add topic * add topic into proto * wip * fix CommitContextOf * add highWatermark into report * wip * management topic * add test for register topic * add test for unregister topic * fmt * fix code * fix test * fix code * fix code Co-authored-by: Hyungkeun Jang <pharrell.jang@kakaocorp.com> * sn: remove redundant types for replica (#483) There were redundant types to represent replica: - `pkg/logc/StorageNode` - `proto/snpb/Replica` - `proto/snpb/AppendRequest_BackupNode` This patch removes those and uses `proto/varlogpb/StorageNode` and types that wrap it. Resolves [#VARLOG-546](VARLOG-546). * sn: add topic id to log i/o (#486) This patch adds `TopicID` to the methods of Log I/O interface. It doesn't contain any meaningful implementations about `TopicID`. Types that now have `TopicID` are follows: - `internal/storagenode/logio.ReadWriter` - `internal/storagenode/reportcommitter/reportcommitter.Getter` - `proto/snpb.AppendRequest`, `proto/snpb.AppendResponse`, `proto/snpb.ReadRequest`, `proto/snpb.SubscribeRequest`, `proto/snpb.TrimRequest` - `proto/varlogpb.Replica` Resolves [#VARLOG-542](VARLOG-542). * it: fix flaky test - TestVarlogSubscribeWithAddLS (#487) The `TestVarlogSubscribeWithAddLS` created a goroutine adding new LogStream while appending log entries. It however did not manage the life cycle of the goroutine resulting in several issues: - use closed connection to append logs - wait for commit messages from MR indefinitely since the MR is already closed This patch simply adds `sync.WaitGroup` to the test to avoid the above issues. Resolves [#VARLOG-569](VARLOG-569). * proto: removing unnecessary fields from messages (#488) This patch removes unnecessary fields generated automatically from proto messages. These lines are like follows: ``` XXX_NoUnkeyedLiteral struct{} `json:"-"` XXX_unrecognized []byte `json:"-"` XXX_sizecache int32 `json:"-"` ``` They are something related to either optimizations or compatibility that are generated by the gogoproto. However, they come to small overhead in many cases. (See cockroachdb/cockroach#38404 and etcd-io/etcd@e51c697#diff-76a35072df72591a656e69cab6f6fa99aa386fd5ace35c9042851eb324ec16b5). This change adds the following options to every proto definition file: ``` option (gogoproto.goproto_unkeyed_all) = false; option (gogoproto.goproto_unrecognized_all) = false; option (gogoproto.goproto_sizecache_all) = false; ``` Resolves [#VARLOG-557](VARLOG-557). * *: dedup LogEntry types (#489) - Remove `pkg/types.LogEntry`, then use `proto/varlogpb.LogEntry`. Resolves [#VARLOG-558](VARLOG-558). * vendor: bump Pebble (#490) Resolves #VARLOG-556 * sn: rename AddLogStream RPC (#491) In this patch, the RPC `AddLogStream` renames to `AddLogStreamReplica` to clarify its behavior. The RPC `AddLogStreamReplica` adds a new replica of the given LogStream to the StorageNode. Resolves [#VARLOG-568](VARLOG-568). * topic: apply Topic into client (#493) * logio: apply topic * add topic test * fix TestMRTopicLastHighWatermark * fix managing log stream status in vms Resolves #VARLOG-559 * all: update golang 1.17.0 (#492) Resolves [#VARLOG-555](VARLOG-555). * all: fix code style (#494) This patch just fixes code styles. Resolves [#VARLOG-572](VARLOG-572). * build: use predefined protoc (#496) Resolves [#VARLOG-563](VARLOG-563). * sn,topic: checking topic id while handling RPCs (#495) This patch adds a feature that the StorageNode checks if the topic id is valid or not. To support this functionality it adds a topicID to a parameter of many functions. The executor does not care about the topicID, rather it will be considered by StorageNode. To do that, StorageNode maintains the executors by using the executorsmap keyed by logStreamTopicID. The logStreamTopicID is a packed type of the LogStreamID and TopicID. Resolves [#VARLOG-542](VARLOG-542). * lint: fix code style (#497) This is a follow-up PR for VARLOG-572. Resolves [#VARLOG-572](VARLOG-572). Co-authored-by: Hyungkeun Jang <pharrell.jang@kakaocorp.com> Co-authored-by: Hyungkeun Jang <pharrell.jang@kakaocorp.com>

nvb requested review from RaduBerinde and ajwerner June 25, 2019 17:30

nvb requested a review from a team as a code owner June 25, 2019 17:30

nvb requested review from a team June 25, 2019 17:30

nvb requested a review from a team as a code owner June 25, 2019 17:30

nvb requested review from a team June 25, 2019 17:30

ajwerner approved these changes Jun 25, 2019

View reviewed changes

RaduBerinde approved these changes Jun 25, 2019

View reviewed changes

nvb force-pushed the nvanbenschoten/sizeCache branch 3 times, most recently from 265bcc9 to 9e30f1c Compare June 25, 2019 20:31

nvb added 2 commits June 25, 2019 17:15

nvb force-pushed the nvanbenschoten/sizeCache branch from 9e30f1c to 95878e0 Compare June 25, 2019 21:16

craig bot merged commit 95878e0 into cockroachdb:master Jun 25, 2019

nvb deleted the nvanbenschoten/sizeCache branch June 25, 2019 23:42

nvb mentioned this pull request Mar 20, 2021

raft: disable XXX_NoUnkeyedLiteral, XXX_unrecognized, and XXX_sizecache fields in protos etcd-io/etcd#12790

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

proto: disable XXX_NoUnkeyedLiteral and XXX_sizecache fields#38404

proto: disable XXX_NoUnkeyedLiteral and XXX_sizecache fields#38404
craig[bot] merged 2 commits intocockroachdb:masterfrom
nvb:nvanbenschoten/sizeCache

nvb commented Jun 25, 2019

Uh oh!

cockroach-teamcity commented Jun 25, 2019

Uh oh!

ajwerner left a comment

Uh oh!

nvb commented Jun 25, 2019

Uh oh!

craig bot commented Jun 25, 2019

Uh oh!

nvb commented Jun 25, 2019

Uh oh!

craig bot commented Jun 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

nvb commented Jun 25, 2019

Benchmarks

Uh oh!

cockroach-teamcity commented Jun 25, 2019

Uh oh!

ajwerner left a comment

Choose a reason for hiding this comment

Uh oh!

nvb commented Jun 25, 2019

Uh oh!

craig bot commented Jun 25, 2019

Build failed

Uh oh!

nvb commented Jun 25, 2019

Uh oh!

craig bot commented Jun 25, 2019

Build succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants