Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: command after sync serialization #422

Merged
merged 3 commits into from
Sep 6, 2023

Conversation

bsbds
Copy link
Collaborator

@bsbds bsbds commented Aug 22, 2023

Described in: #420

Based on: #421

Please briefly answer these questions:

  • what problem are you trying to solve? (or if there's no problem, what's the motivation for this change?)

  • what changes does this pull request make?

  • are there any non-obvious implications of these changes? (does it break compatibility with previous versions, etc)

@codecov
Copy link

codecov bot commented Aug 22, 2023

Codecov Report

Patch coverage: 51.11% and project coverage change: +0.11% 🎉

Comparison is base (24a5f1f) 54.02% compared to head (7e2053b) 54.14%.
Report is 2 commits behind head on master.

❗ Current head 7e2053b differs from pull request most recent head b30fa33. Consider uploading reports for the commit b30fa33 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #422      +/-   ##
==========================================
+ Coverage   54.02%   54.14%   +0.11%     
==========================================
  Files         101      101              
  Lines       16630    16739     +109     
  Branches    16630    16739     +109     
==========================================
+ Hits         8985     9063      +78     
- Misses       7101     7124      +23     
- Partials      544      552       +8     
Files Changed Coverage Δ
curp-external-api/src/cmd.rs 30.23% <ø> (ø)
curp/src/client.rs 16.76% <0.00%> (-0.20%) ⬇️
curp/src/lib.rs 100.00% <ø> (ø)
curp/src/rpc/connect.rs 35.84% <ø> (ø)
curp/src/rpc/mod.rs 22.03% <0.00%> (-1.82%) ⬇️
curp/src/server/curp_node.rs 23.70% <0.00%> (ø)
xlineapi/src/lib.rs 71.69% <ø> (ø)
curp-test-utils/src/test_cmd.rs 80.53% <22.22%> (-1.92%) ⬇️
curp/src/error.rs 58.19% <66.66%> (+23.58%) ⬆️
xline/src/server/command.rs 70.45% <90.62%> (+0.89%) ⬆️
... and 2 more

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bsbds bsbds force-pushed the curp-request-serialization branch 2 times, most recently from 9039419 to f7d812f Compare August 23, 2023 05:05
@LingKa28
Copy link

The files generated from .proto files for Rust are organized into workspaces, but if my generated files are all in one project. The command.proto and error.proto files have the same name, and the Empty struct is duplicated. At runtime, the project will error due to the duplication between the two files.

proto: file "error.proto" is already registered
proto: file "error.proto" has a name conflict over errorpb.Empty
proto: message errorpb.Empty is already registered
proto: file "command.proto" is already registered

But, this issue can be ignored by setting an environment variable.

GOLANG_PROTOBUF_REGISTRATION_CONFLICT="ignore"

@bsbds
Copy link
Collaborator Author

bsbds commented Aug 24, 2023

The files generated from .proto files for Rust are organized into workspaces, but if my generated files are all in one project. The command.proto and error.proto files have the same name, and the Empty struct is duplicated. At runtime, the project will error due to the duplication between the two files.

proto: file "error.proto" is already registered
proto: file "error.proto" has a name conflict over errorpb.Empty
proto: message errorpb.Empty is already registered
proto: file "command.proto" is already registered

But, this issue can be ignored by setting an environment variable.

GOLANG_PROTOBUF_REGISTRATION_CONFLICT="ignore"

I replaced the two Empty types with google.protobuf.Empty, now it should be consistent.

@bsbds bsbds force-pushed the curp-request-serialization branch 4 times, most recently from 6443686 to 7345366 Compare August 31, 2023 02:02
@bsbds bsbds force-pushed the curp-request-serialization branch 2 times, most recently from 227f4e5 to 1a8fc2e Compare September 4, 2023 02:31
curp/proto/error.proto Outdated Show resolved Hide resolved
curp-test-utils/src/test_cmd.rs Outdated Show resolved Hide resolved
curp/src/rpc/mod.rs Outdated Show resolved Hide resolved
curp/Cargo.toml Outdated Show resolved Hide resolved
curp/Cargo.toml Outdated Show resolved Hide resolved
xlineapi/Cargo.toml Outdated Show resolved Hide resolved
@Phoenix500526
Copy link
Collaborator

Please squash commits in this PR, some of them are too granular.

Signed-off-by: bsbds <69835502+bsbds@users.noreply.github.com>
Signed-off-by: bsbds <69835502+bsbds@users.noreply.github.com>
Signed-off-by: bsbds <69835502+bsbds@users.noreply.github.com>

chore: rename `PbSerialize` to `PbCodec`

Signed-off-by: bsbds <69835502+bsbds@users.noreply.github.com>
@Phoenix500526 Phoenix500526 merged commit b573f16 into xline-kv:master Sep 6, 2023
16 of 17 checks passed
@Phoenix500526 Phoenix500526 added the Improvement Refactor or optimization, including process, performance or something like that label Nov 18, 2023
@Phoenix500526 Phoenix500526 added this to the v0.6.0 milestone Nov 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement Refactor or optimization, including process, performance or something like that
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants