CockroachDB Physical Backend #2713
Conversation
| if err != nil { | ||
| return fmt.Errorf("failed to prepare '%s': %v", name, err) | ||
| } | ||
| c.statements[name] = stmt |
There was a problem hiding this comment.
Can we add a nil check for stmt here?
There was a problem hiding this comment.
It appears a *sql.Stmt will always get returned on a nil error.
| if err != nil { | ||
| return nil, fmt.Errorf("failed to connect to cockroachdb: %v", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
Can we have a nil check for db here?
There was a problem hiding this comment.
I checked this and a *sql.DB will aways be returned when err is nil.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer rows.Close() |
There was a problem hiding this comment.
Should this be done before the err check?
There was a problem hiding this comment.
I think you want to do the defer after the error check since rows could be nil.
physical/cockroachdb.go
Outdated
| } | ||
| } | ||
|
|
||
| sort.Strings(keys) |
There was a problem hiding this comment.
This might prove to be expensive if the number of keys being returned are huge. Do we really need to sort them?
There was a problem hiding this comment.
This is the way most of the other backends handle the list operation. I wonder if it would be more expensive to do it in the SQL layer or here.
There was a problem hiding this comment.
I'll go with what you decide here. Personally, I don't see value in sorting as most of the entries are will be UUIDs.
|
Can you plumb through permitpool so people can put a limit on the number of connections? Also, there's a conflict :-) |
|
Good idea. I'll add this shortly. |
* oss/master: (161 commits) update gitignore changelog++ Exclude /sys/leases/renew from registering with expiration manager (#2891) More cleanup Clarify/fix some configuration info. Add a convenience function for copying a client (#2887) Better error messages using ListObjects than using HeadBucket. Might be a bigger request but messages are better than BadRequest, how this changes effect the messages are in the issue (#2892) Add ACL info to Consul configuration page Return error on bad CORS and add Header specification to API request primitive Add Zyborg.Vault PowerShell module to libs list (#2869) changelog++ CouchDB physical backend (#2880) Fix root paths test Add missing datadog vendored lib changelog++ Fix up CORS. Cors headers (#2021) Address review feedback Fix the test error message Added utility on router to fetch mount entry using its ID ...
No description provided.