Addresses comments on original PR #1
Conversation
| @@ -72,6 +75,39 @@ func newCassandraBackend(conf map[string]string, logger log.Logger) (Backend, er | |||
| connectStart := time.Now() | |||
| cluster := gocql.NewCluster(hosts...) | |||
| cluster.Keyspace = keyspace | |||
|
|
|||
| cluster.ProtoVersion = 2 | |||
There was a problem hiding this comment.
Why set this to 2? I agree with letting users specify the version explicitly in the config, but in the case they do not, leaving it as 0 which causes gocql to try to auto-detect the version seems better.
There was a problem hiding this comment.
Oh I see… is it for consistency with this?
There was a problem hiding this comment.
Yes. I'm all up for 0 but their comment also suggested 2.
There was a problem hiding this comment.
🤔 Fair enough, given their goal is to unify the Cassandra connection implementations.
|
|
||
| tlsOn, err := strconv.Atoi(tlsOnStr) | ||
| if err != nil { | ||
| return fmt.Errorf("'tls' must be an integer (0 or 1)") |
There was a problem hiding this comment.
Ill double check. Their config examples use ints and it seems unquoted strings for representing bools trigger parsing errors inconsistently.
There was a problem hiding this comment.
There was a problem hiding this comment.
Hmm, interesting. In the same example there is disable_hostname = true further down 🤔
There was a problem hiding this comment.
It's a bit confusing. I'll dig deeper but while trying unquoted literals in both config formats I had this same issue: hashicorp#1559
There was a problem hiding this comment.
Interesting. Let's leave it as an integer ¯_(ツ)_/¯
I think if you can add the documentation for the new config params then we should be good to submit upstream 🚀
|
These parameters should probably also be documented. |
physical/cassandra.go
Outdated
| if err != nil { | ||
| return nil, fmt.Errorf("'connection_timeout' must be an integer") | ||
| } | ||
| cluster.Timeout = time.Duration(connectionTimeout) * time.Millisecond |
There was a problem hiding this comment.
Just noticed that in this file, this parameter is specified in seconds rather than milliseconds. Should we align the two for consistency? If they aren't the same unit then unifying the two implementations (as they would like) is going to be impossible.
|
@obeattie I'm fixing an issue with pem parsing, please don't merge just yet 👎 |
|
👍 good to go and tested |
created structure for permissions and modified parsePaths in policy.g…
I'm splitting these simple changes from the TLS setup. Working on that next.