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

Cli supports zset data structures (#249) #310

Merged
merged 9 commits into from
Jul 30, 2024

Conversation

halalala222
Copy link
Contributor

1. prevent repeated creation of grpc connections and various command clients to avoid goroutine leaks and reduce the number of goroutines created.

before pr :

use go pprof tools

flyDB client start

HvUjdftMFV

run put test test 11 times

LEQONJJeQm

after run put test test 11 times

mpNMBhxNJS

cause by :

The specified call for each gRPC client will create a gRPC connection using grpc.Dial. However, after the call is executed, the connection will not be closed.

example : string put

// newGrpcClient returns a grpc client
func newGrpcClient(addr string) (gstring.GStringServiceClient, error) {
    conn, err := grpc.Dial(addr, grpc.WithTransportCredentials(insecure.NewCredentials()))
    if err != nil {
       return nil, err
    }
    client := gstring.NewGStringServiceClient(conn)
    return client, nil
}
// Put puts a key-value pair into the db by client api
func (c *Client) Put(key string, value interface{}) error {
	client, err := newGrpcClient(c.Addr)
	if err != nil {
		return errors.New("new grpc client error: " + err.Error())
	}
	//...
	return nil
}

after pr :

flyDB client start

2mlTP7bleC

run put test test 17 times

d2579121-15a8-4bfe-b729-30863a5ee532

after run put test test 17 times

N0r4Pfw17A

2. cli support zset

note : for ZAdds , Grumble does not support having multiple lists within multiple args,because no args can follow a list. Therefore, it is not possible to implement this functionality.

image
image

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for your contribution, we will promptly review your code if there are no errors and pass ci. We will merge your pull request into the master branch.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not recommend creating consts files for a zset, but please add appropriate comments to the file if necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid hardcoding strings like 'key' in the codebase,not only for zset other data structures like set,hash, can use for.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this, so please add comments to it, for example for zset data structures

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry, I misunderstood you before. I'll go and modify my code later.

@qishenonly
Copy link
Member

Thank you very much for solving potential security vulnerabilities in FlyDB. After we check that your code is correct, we will merge your pr into the main branch tomorrow.

@qishenonly qishenonly merged commit 145f2e0 into ByteStorage:master Jul 30, 2024
4 checks passed
@halalala222 halalala222 deleted the cli-support-zet branch July 31, 2024 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants