RediSearch / redisearch-go

Go client for RediSearch
https://redisearch.io
BSD 3-Clause "New" or "Revised" License
293 stars 65 forks source link

Shared pool among clients and fixed pool resources release #51

Closed filipecosta90 closed 4 years ago

filipecosta90 commented 4 years ago

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")
codecov[bot] commented 4 years ago

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%) :arrow_down:
redisearch/client.go 79.75% <100.00%> (+0.31%) :arrow_up:

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 commented 4 years ago

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

filipecosta90 commented 4 years ago

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

filipecosta90 commented 4 years ago

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 ).

filipecosta90 commented 4 years ago

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 :+1: