[#185] Implement rpc/client for tree service #299

Merged
alexvanin merged 1 commit from nzinkevich/frostfs-sdk-go:feat/tree_service_rpc_client into master 2024-12-02 14:58:07 +00:00
Member

Signed-off-by: Nikita Zinkevich n.zinkevich@yadro.com

Signed-off-by: Nikita Zinkevich <n.zinkevich@yadro.com>
nzinkevich self-assigned this 2024-11-20 13:57:51 +00:00
nzinkevich added 1 commit 2024-11-20 13:57:52 +00:00
[#185] Implement rpc/client for tree service
All checks were successful
DCO / DCO (pull_request) Successful in 1m4s
Tests and linters / Tests (pull_request) Successful in 1m24s
Tests and linters / Lint (pull_request) Successful in 2m5s
4a9a3572fa
Signed-off-by: Nikita Zinkevich <n.zinkevich@yadro.com>
nzinkevich requested review from storage-services-committers 2024-11-20 13:59:15 +00:00
nzinkevich requested review from storage-services-developers 2024-11-20 13:59:16 +00:00
aarifullin reviewed 2024-11-20 17:07:31 +00:00
@ -0,0 +352,4 @@
return r.body
}
return new(HealthcheckRequestBody)
Member

Why does this method return dummy value instead nil?

Why does this method return dummy value instead `nil`?
aarifullin marked this conversation as resolved
aarifullin reviewed 2024-11-20 17:17:25 +00:00
@ -0,0 +308,4 @@
type ApplyRequestBody struct {
ContainerID []byte
TreeID string
Operation *LogMove
Member

Even this field (not only this, but also the rest in this file) is directly accessible from this struct instance, you don't provide any getters to make the usage safe. If a client code directly uses Operation, then it may get big-boom if it will access LogMove fields when it's actually nil.

Don't you consider to make all fields private as we do for all api-go related types? And provide Get/Set methods?

Even this field (not only this, but also the rest in this file) is directly accessible from this struct instance, you don't provide any getters to make the usage safe. If a client code directly uses `Operation`, then it may get big-boom if it will access `LogMove` fields when it's actually `nil`. Don't you consider to make all fields private as we do for all api-go related types? And provide `Get/Set` methods?
Member

Probably we can write something like in autogenerated code: we have exported fields but also setters with checking for nil. I believe it can be useful when we create new *Request variable

Probably we can write something like in autogenerated code: we have exported fields but also setters with checking for nil. I believe it can be useful when we create new `*Request` variable
Owner

For the record, I (subjectively) prefer having public fields in the structs of API message definitions.

However all structures in api package defined with Get/Set methods, so it makes sense to keep it for tree as well.

For the record, I (subjectively) prefer having public fields in the structs of API message definitions. However all structures in `api` package defined with `Get/Set` methods, so it makes sense to keep it for tree as well.
aarifullin marked this conversation as resolved
dkirillov reviewed 2024-11-26 12:13:24 +00:00
api/rpc/tree.go Outdated
@ -0,0 +101,4 @@
func GetSubTree(cli *client.Client,
req *tree.GetSubTreeRequest,
opts ...client.CallOption) (*GetSubTreeResponseReader, error) {
Member

Can we use the same format across all file?
I mean write

func GetSubTree(cli *client.Client,
	req *tree.GetSubTreeRequest,
	opts ...client.CallOption,
) (*GetSubTreeResponseReader, error) {

instead of

	func GetSubTree(cli *client.Client,
	req *tree.GetSubTreeRequest,
	opts ...client.CallOption) (*GetSubTreeResponseReader, error) {
Can we use the same format across all file? I mean write ```golang func GetSubTree(cli *client.Client, req *tree.GetSubTreeRequest, opts ...client.CallOption, ) (*GetSubTreeResponseReader, error) { ``` instead of ```golang func GetSubTree(cli *client.Client, req *tree.GetSubTreeRequest, opts ...client.CallOption) (*GetSubTreeResponseReader, error) { ```
dkirillov marked this conversation as resolved
@ -0,0 +1,1590 @@
package tree
Member

Is this file autogenerated? If so can we add all autogenerated files in separate commit?

Is this file autogenerated? If so can we add all autogenerated files in separate commit?
Author
Member

Unfortunately, these files weren't actually autogenerated

Unfortunately, these files weren't actually autogenerated
dkirillov marked this conversation as resolved
@ -23,0 +21,4 @@
mu sync.RWMutex
address string
opts []grpc.DialOption
conn io.Closer
Member

Why do we need separate field? Is not enough having client *rpcclient.Client?

Why do we need separate field? Is not enough having `client *rpcclient.Client`?
dkirillov marked this conversation as resolved
@ -61,13 +71,13 @@ func (c *treeClient) redialIfNecessary(ctx context.Context) (healthHasChanged bo
defer c.mu.Unlock()
if c.conn == nil {
Member

I would expect checking for if c.client == nil

I would expect checking for `if c.client == nil`
dkirillov marked this conversation as resolved
@ -98,2 +106,2 @@
return nil, nil, fmt.Errorf("grpc create node tree service: %w", err)
}
cli := rpcclient.New(append(
rpcclient.WithNetworkURIAddress(c.address, &tls.Config{}),
Member

If we use this options, than we don't need previously defined options

If we use this options, than we don't need previously defined `options`
dkirillov marked this conversation as resolved
@ -365,3 +366,3 @@
// Return an error there to trigger retry and ignore it after,
// to keep compatibility with 'GetNodeByPath' implementation.
if inErr == nil && len(resp.GetBody().GetNodes()) == 0 {
if inErr == nil && len(resp.Body.Nodes) == 0 {
Member

It's better to keep previous signature with inner nil checking

It's better to keep previous signature with inner nil checking
dkirillov marked this conversation as resolved
nzinkevich force-pushed feat/tree_service_rpc_client from 4a9a3572fa to 973b09d232 2024-11-26 12:30:24 +00:00 Compare
nzinkevich force-pushed feat/tree_service_rpc_client from 973b09d232 to f34736b59a 2024-11-26 12:55:51 +00:00 Compare
alexvanin approved these changes 2024-11-27 12:58:47 +00:00
Dismissed
alexvanin left a comment
Owner

Seems good for me. We are going to update this code as soon as tree service becomes a part of frostfs-api. It is a great basis for that.

Before merge, I would like to run some integration tests in S3 gateway with new tree client. I want to ask @dkirillov and @nzinkevich to run some s3-tests upfront, and they comment if everything went well or not.

Seems good for me. We are going to update this code as soon as tree service becomes a part of `frostfs-api`. It is a great basis for that. Before merge, I would like to run some integration tests in S3 gateway with new tree client. I want to ask @dkirillov and @nzinkevich to run some [s3-tests](https://git.frostfs.info/TrueCloudLab/s3-tests) upfront, and they comment if everything went well or not.
Author
Member

image

![image](/attachments/99a2f8ee-7159-4b94-9969-9746118071eb)
nzinkevich requested review from storage-core-committers 2024-11-28 10:20:15 +00:00
nzinkevich requested review from storage-core-developers 2024-11-28 10:20:16 +00:00
dkirillov requested changes 2024-11-28 16:10:56 +00:00
dkirillov left a comment
Member

See comment

See comment
@ -134,2 +126,3 @@
if c.conn == nil {
conn := c.client.Conn()
if c.client == nil || conn == nil {
Member

If we check c.client for nil then conn := c.client.Conn() above can cause panic. (Probably this will never happen but code logically incorrect)

If we check `c.client` for `nil` then `conn := c.client.Conn()` above can cause panic. (Probably this will never happen but code logically incorrect)
dkirillov marked this conversation as resolved
dkirillov reviewed 2024-11-29 06:31:08 +00:00
@ -445,2 +441,2 @@
},
}
body := new(tree.GetSubTreeRequestBody)
body.SetContainerID(prm.CID[:])
Member

minor: Probably we can pass prm.CID to method and inside transform to bytes

minor: Probably we can pass `prm.CID` to method and inside transform to bytes
Member

I meant accept cid.ID as parameter type, but ok

I meant accept `cid.ID` as parameter type, but ok
nzinkevich force-pushed feat/tree_service_rpc_client from f34736b59a to 0352b5b191 2024-11-29 12:23:40 +00:00 Compare
nzinkevich dismissed alexvanin's review 2024-11-29 12:23:40 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

alexvanin approved these changes 2024-12-02 10:33:25 +00:00
aarifullin approved these changes 2024-12-02 10:39:38 +00:00
alexvanin requested review from dkirillov 2024-12-02 14:57:56 +00:00
alexvanin merged commit 0352b5b191 into master 2024-12-02 14:58:07 +00:00
alexvanin deleted branch feat/tree_service_rpc_client 2024-12-02 14:58:17 +00:00
Sign in to join this conversation.
No reviewers
TrueCloudLab/storage-services-developers
TrueCloudLab/storage-core-committers
No milestone
No project
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-sdk-go#299
No description provided.