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

Shared pool among clients and fixed pool resources release #51

Merged
merged 6 commits into from
Apr 7, 2020
Merged

Conversation

filipecosta90
Copy link
Collaborator

@filipecosta90 filipecosta90 commented Apr 5, 2020

  • [add] added option to create clients with shared pool, via NewClientFromPool(pool *redis.Pool, name string) *Client
  • [fix] added option to releases the resources used by the pool ( single, and multi-host ) via Close()

fixes #16, #9

simple example of two clients sharing the pool:

        pool := &redis.Pool{Dial: func() (redis.Conn, error) {
		return redis.Dial("tcp", host, redis.DialPassword(password))
	}, MaxIdle: maxConns}
	client1 := NewClientFromPool(pool,"index1")
	client2 := NewClientFromPool(pool,"index2")

Verified

This commit was signed with the committer’s verified signature.
…o releases the resources used by the pool ( single, and multihost )
@codecov
Copy link

codecov bot commented Apr 5, 2020

Codecov Report

Merging #51 into master will decrease coverage by 0.05%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #51      +/-   ##
==========================================
- Coverage   72.65%   72.59%   -0.06%     
==========================================
  Files          12       12              
  Lines        1064     1080      +16     
==========================================
+ Hits          773      784      +11     
- Misses        232      236       +4     
- Partials       59       60       +1     
Impacted Files Coverage Δ
redisearch/pool.go 76.92% <66.66%> (-8.80%) ⬇️
redisearch/client.go 79.75% <100.00%> (+0.31%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ccd0b5...f96a40a. Read the comment docs.

@filipecosta90
Copy link
Collaborator Author

@ericfengchao can you review this one as well? Would like your input but was not able to mark you as reviewer

@gkorland gkorland requested review from MeirShpilraien and removed request for gkorland April 5, 2020 13:31
itamarhaber
itamarhaber previously approved these changes Apr 5, 2020
Copy link
Contributor

@itamarhaber itamarhaber left a comment

Choose a reason for hiding this comment

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

LGTM but lets indeed wait for @ericfengchao to look and try before merging.

@filipecosta90
Copy link
Collaborator Author

@itamarhaber and @ericfengchao can you double-check the changes :)

itamarhaber
itamarhaber previously approved these changes Apr 6, 2020
Copy link
Contributor

@itamarhaber itamarhaber left a comment

Choose a reason for hiding this comment

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

Although the change to Close will only report the last error I'm good with that (is returning an array of errors something that's done in golang anyway?)

@filipecosta90
Copy link
Collaborator Author

Although the change to Close will only report the last error I'm good with that (is returning an array of errors something that's done in golang anyway?)

Now that you've mentioned it @itamarhaber I believe the best way is to keep the function signature but to generate an error with all details of error occurrences (I'll change it and return back in a comment to see if you guys like that idea ).

…serving all errors on multihost pool CLose()
…estNewMultiHostPool/multi-host_single_address
defer p.Unlock()
for host, pool := range p.pools {
poolErr := pool.Close()
//preserve pool error if not nil but continue
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@itamarhaber what do you think about the following solution?

Copy link
Contributor

Choose a reason for hiding this comment

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

Lovely 🔨

Copy link

@ericfengchao ericfengchao left a comment

Choose a reason for hiding this comment

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

lgtm 💯 but ci was failing tho

@filipecosta90
Copy link
Collaborator Author

lgtm 100 but ci was failing tho

It's failing only due to coverage difference -0.06% with the added error concatenation. I believe we can merge it 👍

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.

how was the connection pool closed?
4 participants