TrueCloudLab/frostfs-api-go
22
0
Fork 15
Code Issues 10 Pull requests 1 Projects Releases Activity Actions

WIP: Generate StableMarshaler/StableSize methods for protobufs #43

Closed
aarifullin wants to merge 1 commit from aarifullin/frostfs-api-go:feature/40-autogen_marsh/proto into master
pull from: aarifullin/frostfs-api-go:feature/40-autogen_marsh/proto
merge into: TrueCloudLab:master
Conversation 2 Commits 1 Files changed 151 +6433 -17609
aarifullin commented 2023-06-21 20:26:37 +00:00
Member
Copy link
  • Add plugin option for protogen in Makefile
  • Fix the generator for the plugin in util/protogen
  • Fix marshaler.go for refs

Signed-off-by: Airat Arifullin a.arifullin@yadro.com

* Add plugin option for protogen in Makefile * Fix the generator for the plugin in util/protogen * Fix marshaler.go for refs Signed-off-by: Airat Arifullin a.arifullin@yadro.com
aarifullin changed title from [#40] types: Generate StableMarshaler methods for refs to WIP: [#40] types: Generate StableMarshaler methods for refs 2023-06-21 20:26:48 +00:00
aarifullin force-pushed feature/40-autogen_marsh/proto from 9d0993aa6f to 29d9637345 2023-06-22 10:21:39 +00:00 Compare
aarifullin force-pushed feature/40-autogen_marsh/proto from 29d9637345 to ff0c16d853 2023-06-28 13:16:37 +00:00 Compare
aarifullin force-pushed feature/40-autogen_marsh/proto from ff0c16d853 to 88864fe7b0 2023-06-28 13:17:22 +00:00 Compare
aarifullin changed title from WIP: [#40] types: Generate StableMarshaler methods for refs to WIP: [#40] types: Generate StableMarshaler/StableSize methods for protobufs 2023-06-28 13:17:40 +00:00
aarifullin changed title from WIP: [#40] types: Generate StableMarshaler/StableSize methods for protobufs to [#40] types: Generate StableMarshaler/StableSize methods for protobufs 2023-06-28 13:19:09 +00:00
requested reviews from alexvanin, storage-core-committers, storage-core-developers, realloc 2023-06-28 13:34:32 +00:00
aarifullin reviewed 2023-06-28 13:41:40 +00:00
Makefile Outdated
@ -47,3 +47,3 @@
echo "⇒ Processing $$f "; \
protoc \
--proto_path=.:./vendor:/usr/local/include \
--proto_path=.:./vendor:/usr/local/include:/home/aarifullin/ws/frostfs-api \
aarifullin commented 2023-06-28 13:41:40 +00:00
Author
Member
Copy link

will be fixed

will be fixed
dstepanov-yadro reviewed 2023-06-28 14:37:12 +00:00
container/grpc/attributes.go Outdated
@ -0,0 +65,4 @@
// See also HomomorphicHashingState.
func (c *Container) SetHomomorphicHashingState(enable bool) {
for i := range c.Attributes {
if c.Attributes[i].GetKey() == SysAttributeHomomorphicHashing || c.Attributes[i].GetKey() == SysAttributeHomomorphicHashingNeoFS {
dstepanov-yadro commented 2023-06-28 14:37:12 +00:00
Member
Copy link

You use method access (.GetKey) and field access (.Attributes[i]) in one statement. Maybe it's better to select one? I prefer method access, because it has nil checks.

You use method access (`.GetKey`) and field access (`.Attributes[i]`) in one statement. Maybe it's better to select one? I prefer method access, because it has nil checks.
aarifullin commented 2023-06-29 08:20:07 +00:00
Author
Member
Copy link

it has nil checks

I like this point. I have fixed the snippet

> it has nil checks I like this point. I have fixed the snippet
dstepanov-yadro reviewed 2023-06-28 14:56:43 +00:00
signature/sign.go Outdated
@ -14,3 +14,2 @@
GetMetaHeader() *session.RequestMetaHeader
GetVerificationHeader() *session.RequestVerificationHeader
SetVerificationHeader(*session.RequestVerificationHeader)
GetVerifyHeader() *session.RequestVerificationHeader
dstepanov-yadro commented 2023-06-28 14:56:43 +00:00
Member
Copy link

Why renamed?

Why renamed?
aarifullin commented 2023-06-28 15:25:02 +00:00
Author
Member
Copy link

Line 254 in TrueCloudLab/frostfs-api@fc393d4
neo.fs.v2.session.RequestVerificationHeader verify_header = 3;

Because protobufs are generated with such getter (based on field name)

https://git.frostfs.info/TrueCloudLab/frostfs-api/src/commit/fc393d4605e7e4c5545e882ef20d5dc171e25ce8/object/service.proto#L254 Because protobufs are generated with such getter (based on field name)
dstepanov-yadro marked this conversation as resolved
dstepanov-yadro reviewed 2023-06-28 15:09:46 +00:00
util/protogen/main.go Outdated
@ -17,2 +20,2 @@
imp := string(f.GoImportPath)
if strings.HasSuffix(imp, "/tree") || strings.HasSuffix(imp, "/control") {
impPath := f.GoImportPath.String()
if strings.HasSuffix(impPath, "/tree") || strings.HasSuffix(impPath, "/control") ||
dstepanov-yadro commented 2023-06-28 15:09:46 +00:00
Member
Copy link

Could you explain this change?

Could you explain this change?
aarifullin commented 2023-06-28 15:31:31 +00:00
Author
Member
Copy link

This is probably my dirty trick. The plugin has not been used for frostfs-api-go so far, only for frostfs-node (/tree, /control). grpc is needed to generate pb-s in frostfs-api-go folders :) I'll consider better solution

This is probably my dirty trick. The plugin has not been used for `frostfs-api-go` so far, only for `frostfs-node` (`/tree`, `/control`). `grpc` is needed to generate pb-s in frostfs-api-go folders :) I'll consider better solution
aarifullin commented 2023-06-28 15:23:15 +00:00
Author
Member
Copy link

This how SDK will probably look like TrueCloudLab/frostfs-sdk-go#100

This how SDK will *probably* look like https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/pulls/100
aarifullin force-pushed feature/40-autogen_marsh/proto from 88864fe7b0 to 87584983ac 2023-06-29 08:19:45 +00:00 Compare
aarifullin force-pushed feature/40-autogen_marsh/proto from 87584983ac to e957278c57 2023-07-04 07:55:01 +00:00 Compare
aarifullin force-pushed feature/40-autogen_marsh/proto from e957278c57 to 80e6a0bbd6 2023-07-04 14:10:50 +00:00 Compare
fyrchik reviewed 2023-07-05 13:51:13 +00:00
Makefile Outdated
@ -48,2 +48,3 @@
protoc \
--proto_path=.:./vendor:/usr/local/include \
--proto_path=.:./vendor:/usr/local/include:/home/aarifullin/ws/frostfs-api \
--plugin=protoc-gen-go-frostfs=/home/aarifullin/ws/frostfs-api-go/util/protogen/protogen \
fyrchik commented 2023-07-05 13:39:18 +00:00
Owner
Copy link

./bin?

`./bin`?
aarifullin commented 2023-07-06 08:37:35 +00:00
Author
Member
Copy link

This will be fixed. BTW, I left the comment with my note at this line above but it was automatically outdated :(

This will be fixed. BTW, I left the comment with my note at this line above but it was automatically outdated :(
fyrchik commented 2023-07-11 14:12:29 +00:00
Owner
Copy link

I mean what is the problem with fixing it right now? Does current implementation has some benefits?

I mean what is the problem with fixing it right now? Does current implementation has some benefits?
accounting/json.go Outdated
@ -1,14 +0,0 @@
package accounting
fyrchik commented 2023-07-05 13:40:23 +00:00
Owner
Copy link

It seems only message_test.go files are left, do we need a grpc package at all?

It seems only `message_test.go` files are left, do we need a `grpc` package at all?
aarifullin commented 2023-07-06 08:46:57 +00:00
Author
Member
Copy link

Good question.

It seems only message_test.go files are left

The folders that contain not only message_test.go files

acl/
container/
object/
session/
status/

For me it's really better to keep pb files in /grpc/ folders:

  1. /grpc/ contains autogenerated pb-s and extra methods (types.go, service.go)
  2. It's not good idea to keep pb-s for some types in /grpc/ or in their root
  3. That requires change frostfs-api (go_package-s etc) and plugin
  4. So on...
Good question. > It seems only message_test.go files are left The folders that contain not only `message_test.go` files `acl/` `container/` `object/` `session/` `status/` For me it's really better to keep pb files in `/grpc/` folders: 1. `/grpc/` contains autogenerated pb-s and extra methods (`types.go`, `service.go`) 2. It's not good idea to keep pb-s for some types in `/grpc/` or in their root 3. That requires change `frostfs-api` (`go_package`-s etc) and plugin 4. So on...
fyrchik marked this conversation as resolved
netmap/grpc/types.go Outdated
@ -196,6 +224,29 @@ func (x *NetworkConfig) SetParameters(v []*NetworkConfig_Parameter) {
x.Parameters = v
}
// // NumberOfParameters returns number of network parameters.
fyrchik commented 2023-07-05 13:41:57 +00:00
Owner
Copy link

Double comment

Double comment
netmap/grpc/types.go Outdated
@ -199,0 +237,4 @@
// Breaks iteration on f's true return.
//
// Handler must not be nil.
func (x *NetworkConfig) IterateParameters(f func(*NetworkConfig_Parameter) bool) {
fyrchik commented 2023-07-05 13:43:53 +00:00
Owner
Copy link

While we are here, do you think it is worth having such non-autogenerated code here?

While we are here, do you think it is worth having such non-autogenerated code here?
aarifullin commented 2023-07-06 15:29:38 +00:00
Author
Member
Copy link

such non-autogenerated code here?

From my POV types.go, service.go files contain protobuf helper methods. So, yes, I think that's absolutely fine.
For me it is more weird to move helpers and wrappers from /grpc/ folder

> such non-autogenerated code here? From my POV `types.go`, `service.go` files contain protobuf helper methods. So, yes, I think that's absolutely fine. For me it is more weird to move helpers and wrappers from `/grpc/` folder
object/grpc/service.go Outdated
@ -8,0 +9,4 @@
type GetRangeResponseBodyRangePart isGetRangeResponse_Body_RangePart
type HeadResponseBodyHead = isHeadResponse_Body_Head
fyrchik commented 2023-07-05 13:44:56 +00:00
Owner
Copy link

Why do we need this synonyms?

Why do we need this synonyms?
aarifullin commented 2023-07-06 09:14:22 +00:00
Author
Member
Copy link

oneof generates protobuf like that

type GetResponse_Body struct {
	state         protoimpl.MessageState
	sizeCache     protoimpl.SizeCache
	unknownFields protoimpl.UnknownFields

	// Single message in the response stream.
	//
	// Types that are assignable to ObjectPart:
	//
	//	*GetResponse_Body_Init_
	//	*GetResponse_Body_Chunk
	//	*GetResponse_Body_SplitInfo
	ObjectPart isGetResponse_Body_ObjectPart `protobuf_oneof:"object_part"`
}

func (*GetResponse_Body_Init_) isGetResponse_Body_ObjectPart() {}

func (*GetResponse_Body_Chunk) isGetResponse_Body_ObjectPart() {}

func (*GetResponse_Body_SplitInfo) isGetResponse_Body_ObjectPart() {}

But the interface cannot be exported. So, alias is the trick

Example of usage:

func (s *streamObjectWriter) WriteHeader(_ context.Context, obj *object.Object) error {
	p := new(objectV2.GetResponse_Body_Init_)

	objV2 := obj.ToV2()
	p.Init = new(objectV2.GetResponse_Body_Init)
	p.Init.SetObjectId(objV2.GetObjectId())
	p.Init.SetHeader(objV2.GetHeader())
	p.Init.SetSignature(objV2.GetSignature())

	return s.GetObjectStream.Send(newResponse(p))
}

func (s *streamObjectWriter) WriteChunk(_ context.Context, chunk []byte) error {
	p := new(objectV2.GetResponse_Body_Chunk)
	p.SetChunk(chunk)

	return s.GetObjectStream.Send(newResponse(p))
}

func newResponse(p objectV2.GetResponseBodyObjectPart) *objectV2.GetResponse {
	r := new(objectV2.GetResponse)
	body := new(objectV2.GetResponse_Body)
	if v, ok := p.(*objectV2.GetResponse_Body_Init_); ok {
		body.SetInit(v.Init)
	} else if v, ok := p.(*objectV2.GetResponse_Body_Chunk); ok {
		body.SetChunk(v)
	} else if v, ok := p.(*objectV2.GetResponse_Body_SplitInfo); ok {
		body.SetSplitInfo(v.SplitInfo)
	} else {
		panic("unsupported response body part type")
	}
	r.SetBody(body)
	return r
}
`oneof` generates protobuf like that ``` type GetResponse_Body struct { state protoimpl.MessageState sizeCache protoimpl.SizeCache unknownFields protoimpl.UnknownFields // Single message in the response stream. // // Types that are assignable to ObjectPart: // // *GetResponse_Body_Init_ // *GetResponse_Body_Chunk // *GetResponse_Body_SplitInfo ObjectPart isGetResponse_Body_ObjectPart `protobuf_oneof:"object_part"` } ``` ``` func (*GetResponse_Body_Init_) isGetResponse_Body_ObjectPart() {} func (*GetResponse_Body_Chunk) isGetResponse_Body_ObjectPart() {} func (*GetResponse_Body_SplitInfo) isGetResponse_Body_ObjectPart() {} ``` But the interface cannot be exported. So, alias is the trick Example of usage: ``` func (s *streamObjectWriter) WriteHeader(_ context.Context, obj *object.Object) error { p := new(objectV2.GetResponse_Body_Init_) objV2 := obj.ToV2() p.Init = new(objectV2.GetResponse_Body_Init) p.Init.SetObjectId(objV2.GetObjectId()) p.Init.SetHeader(objV2.GetHeader()) p.Init.SetSignature(objV2.GetSignature()) return s.GetObjectStream.Send(newResponse(p)) } func (s *streamObjectWriter) WriteChunk(_ context.Context, chunk []byte) error { p := new(objectV2.GetResponse_Body_Chunk) p.SetChunk(chunk) return s.GetObjectStream.Send(newResponse(p)) } func newResponse(p objectV2.GetResponseBodyObjectPart) *objectV2.GetResponse { r := new(objectV2.GetResponse) body := new(objectV2.GetResponse_Body) if v, ok := p.(*objectV2.GetResponse_Body_Init_); ok { body.SetInit(v.Init) } else if v, ok := p.(*objectV2.GetResponse_Body_Chunk); ok { body.SetChunk(v) } else if v, ok := p.(*objectV2.GetResponse_Body_SplitInfo); ok { body.SetSplitInfo(v.SplitInfo) } else { panic("unsupported response body part type") } r.SetBody(body) return r } ```
fyrchik marked this conversation as resolved
rpc/message/test/message.go Outdated
@ -54,3 +17,1 @@
if bm, ok := msg.(binaryMessage); ok {
t.Run(fmt.Sprintf("Binary_%T", msg), func(t *testing.T) {
if bm, ok := msg1.(utilproto.StableMarshaller); ok {
fyrchik commented 2023-07-05 13:46:19 +00:00
Owner
Copy link

Where did JSON test go?

Where did JSON test go?
aarifullin commented 2023-07-06 15:41:11 +00:00
Author
Member
Copy link

Added. Please, note: since we no longer use proxy-types, we need to use std packages to work with protobufs proto, protojson for assigning, comparing, marshalling etc

As the result - I removed many converters and marshallers that dealt with standard golang types

Added. Please, note: since we no longer use proxy-types, we need to use std packages to work with protobufs `proto, protojson` for assigning, comparing, marshalling etc As the result - I removed many converters and marshallers that dealt with standard golang types
util/protogen/main.go Outdated
@ -26,2 +30,4 @@
// generateFile generates a *.pb.go file enforcing field-order serialization.
func generateFile(gen *protogen.Plugin, file *protogen.File) *protogen.GeneratedFile {
emitMask := MethodMask(0)
if imp := string(file.GoImportPath); strings.HasSuffix(imp, "/tree") || strings.HasSuffix(imp, "/control") {
fyrchik commented 2023-07-05 13:49:47 +00:00
Owner
Copy link

While we are here: what about using command-line options? tree is from a different repo, I think it makes sense to give user control.

While we are here: what about using command-line options? `tree` is from a different repo, I think it makes sense to give user control.
fyrchik marked this conversation as resolved
dstepanov-yadro approved these changes 2023-07-05 15:03:35 +00:00
aarifullin force-pushed feature/40-autogen_marsh/proto from 80e6a0bbd6 to 9e291ff6b1 2023-07-06 15:37:57 +00:00 Compare
acid-ant approved these changes 2023-07-07 12:21:50 +00:00
status/message_test.go Outdated
@ -8,4 +5,3 @@
statustest "git.frostfs.info/TrueCloudLab/frostfs-api-go/v2/status/test"
)
func TestMessageConvert(t *testing.T) {
acid-ant commented 2023-07-07 12:20:20 +00:00
Member
Copy link

Why not to remove it?

Why not to remove it?
aarifullin commented 2023-07-07 13:30:59 +00:00
Author
Member
Copy link

Thank u! I forgot to remove

Thank u! I forgot to remove
aarifullin force-pushed feature/40-autogen_marsh/proto from 9e291ff6b1 to c8b3fc0db2 2023-07-07 13:30:46 +00:00 Compare
aarifullin force-pushed feature/40-autogen_marsh/proto from c8b3fc0db2 to be3cfff1e1 2023-07-10 09:06:42 +00:00 Compare
aarifullin force-pushed feature/40-autogen_marsh/proto from be3cfff1e1 to bc16a32c24 2023-07-10 09:08:59 +00:00 Compare
aarifullin changed title from [#40] types: Generate StableMarshaler/StableSize methods for protobufs to WIP: [#40] types: Generate StableMarshaler/StableSize methods for protobufs 2023-07-17 08:13:17 +00:00
realloc changed title from WIP: [#40] types: Generate StableMarshaler/StableSize methods for protobufs to WIP: Generate StableMarshaler/StableSize methods for protobufs 2023-08-02 12:02:30 +00:00
fyrchik commented 2023-08-23 19:00:12 +00:00
Owner
Copy link

Postponed.

Postponed.
fyrchik closed this pull request 2023-08-23 19:00:12 +00:00
All checks were successful
Tests and linters / Tests (1.19) (pull_request) Successful in 45s
Details
Tests and linters / Lint (pull_request) Successful in 56s
Details
Tests and linters / Tests (1.20) (pull_request) Successful in 2m3s
Details
Tests and linters / Tests with -race (pull_request) Successful in 2m13s
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
alexvanin
realloc
dstepanov-yadro
acid-ant
TrueCloudLab/storage-core-developers
Labels
Clear labels   
P0

Highest

  
P1

High

  
P2

Medium

  
P3

Low

  
good first issue

Good for newcomers

  
Infrastructure

Infrastructure, Automation, CI/CD or DevOps related stuff

  
blocked

The issue or PR is blocked by something

  
bug

Something is not working

  
config

Configuration format update or breaking change

  
discussion

Talking about something in order to reach a decision or to exchange ideas

  
documentation

Documentation related

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
go

Language features adoption

  
help wanted

Extra attention is needed

  
internal

   
invalid

Something is wrong

  
kludge

This is a kludge and we know it

  
observability

Related to metrics, tracing and logs

  
perfomance

Perfomance related

  
question

More information is needed

  
refactoring

Refactoring

  
wontfix

This won't be fixed

No labels
P0
P1
P2
P3
good first issue
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
4 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-api-go#43
No description provided.
Delete branch "aarifullin/frostfs-api-go:feature/40-autogen_marsh/proto"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?