[#185] Implement rpc/client for tree service #299
No reviewers
TrueCloudLab/storage-services-developers
TrueCloudLab/storage-core-committers
Labels
No labels
P0
P1
P2
P3
good first issue
pool
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
No milestone
No project
No assignees
4 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: TrueCloudLab/frostfs-sdk-go#299
Loading…
Reference in a new issue
No description provided.
Delete branch "nzinkevich/frostfs-sdk-go:feat/tree_service_rpc_client"
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?
Signed-off-by: Nikita Zinkevich n.zinkevich@yadro.com
@ -0,0 +352,4 @@
return r.body
}
return new(HealthcheckRequestBody)
Why does this method return dummy value instead
nil
?@ -0,0 +308,4 @@
type ApplyRequestBody struct {
ContainerID []byte
TreeID string
Operation *LogMove
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 accessLogMove
fields when it's actuallynil
.Don't you consider to make all fields private as we do for all api-go related types? And provide
Get/Set
methods?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
variableFor the record, I (subjectively) prefer having public fields in the structs of API message definitions.
However all structures in
api
package defined withGet/Set
methods, so it makes sense to keep it for tree as well.@ -0,0 +101,4 @@
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
instead of
@ -0,0 +1,1590 @@
package tree
Is this file autogenerated? If so can we add all autogenerated files in separate commit?
Unfortunately, these files weren't actually autogenerated
@ -23,0 +21,4 @@
mu sync.RWMutex
address string
opts []grpc.DialOption
conn io.Closer
Why do we need separate field? Is not enough having
client *rpcclient.Client
?@ -61,13 +71,13 @@ func (c *treeClient) redialIfNecessary(ctx context.Context) (healthHasChanged bo
defer c.mu.Unlock()
if c.conn == nil {
I would expect checking for
if c.client == nil
@ -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{}),
If we use this options, than we don't need previously defined
options
@ -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 {
It's better to keep previous signature with inner nil checking
4a9a3572fa
to973b09d232
973b09d232
tof34736b59a
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.
See comment
@ -134,2 +126,3 @@
if c.conn == nil {
conn := c.client.Conn()
if c.client == nil || conn == nil {
If we check
c.client
fornil
thenconn := c.client.Conn()
above can cause panic. (Probably this will never happen but code logically incorrect)@ -445,2 +441,2 @@
},
}
body := new(tree.GetSubTreeRequestBody)
body.SetContainerID(prm.CID[:])
minor: Probably we can pass
prm.CID
to method and inside transform to bytesI meant accept
cid.ID
as parameter type, but okf34736b59a
to0352b5b191
New commits pushed, approval review dismissed automatically according to repository settings