Implement Redis client configuration logic #104
Conversation
aaron-congo
changed the title
There was a problem hiding this comment.
consider using linter for makefile too (later)
.github/workflows/go.yml
Outdated
| @@ -65,7 +65,13 @@ jobs: | |||
| with: | |||
| redis-version: ${{ matrix.redis }} | |||
|
|
|||
| - name: Install client dependencies | |||
| - name: Install tools for Go 1.18 | |||
There was a problem hiding this comment.
Having one set of CI tool versions was not working for me because go 1.18 and go 1.21 require different versions of CI tools. Old tools did not work on go 1.21 and new tools did not work on go 1.18. I've looked around and have not been able to find recommendations on how to handle this scenario. The solution I've landed on so far is to have different make targets for installing tools on go 1.18 vs go 1.20
|
|
||
| import "github.com/aws/glide-for-redis/go/glide/protobuf" | ||
|
|
||
| // NodeAddress represents the host address and port of a node in the cluster. |
There was a problem hiding this comment.
Apparently go doc comments are supposed to include the name of the type/func in the first sentence, so the docs are slightly different than the other clients to accommodate that. I've tried to keep them as close possible still though
There was a problem hiding this comment.
@Yury-Fridlyand present tense will be forced for Go it seems ;)
| ) | ||
|
|
||
| type baseClientConfiguration struct { | ||
| addresses []NodeAddress |
There was a problem hiding this comment.
I've made these fields private (indicated by the lower-case first letter) so that the user has to use the With* builder methods instead of accessing them directly
There was a problem hiding this comment.
Instead of putting docs for these private fields here, I've put the docs above the associated With* builder methods below
go/api/config_test.go
Outdated
| @@ -0,0 +1,160 @@ | |||
| /** | |||
There was a problem hiding this comment.
Note: I had to get rid of the tests folder and put this test file beside the implementation file. Unfortunately this seems to be the way to do things in Go. If the test file resides in a separate tests directory, it is not in the same package as the implementation, which means tests do not have access to any of the internal (private) symbols in the implementation. Looking at other Go projects they also have their test files beside their implementation files
aaron-congo
changed the title
| } | ||
|
|
||
| // NewRedisCredentials returns a new RedisCredentials struct with the given username and password. | ||
| func NewRedisCredentials(username string, password string) *RedisCredentials { |
There was a problem hiding this comment.
since the zero-values for the struct are all suitable for defaults, if we wanted we could get rid of the whole builder and just have the user use the struct directly. But that solution may be slightly more awkward in some ways, eg forming the address list
aaron-congo
requested review from
jonathanl-bq,
SanHalacogluImproving and
acarbonetto
There was a problem hiding this comment.
One minor comment about iota, but otherwise looks pretty good. We should make sure we confirm with the rest of the team that we want to remove the builder, as per our discussion. Also, we need someone who hasn't already installed prerequisites for working on this wrapper to test the DEVELOPER.md steps.
|
|
||
| import "github.com/aws/glide-for-redis/go/glide/protobuf" | ||
|
|
||
| // NodeAddress represents the host address and port of a node in the cluster. |
There was a problem hiding this comment.
@Yury-Fridlyand present tense will be forced for Go it seems ;)
|
|
||
| // NodeAddress represents the host address and port of a node in the cluster. | ||
| type NodeAddress struct { | ||
| Host string // Optional: if not supplied, "localhost" will be used. |
There was a problem hiding this comment.
move the Optional comment above the field, rather than inline
There was a problem hiding this comment.
The Go docs have examples with both inline and above, but if we prefer them above then I can change this
| // | ||
| // Once the maximum value is reached, that will remain the time between retry attempts until a reconnect attempt is successful. | ||
| // The client will attempt to reconnect indefinitely. | ||
| type BackoffStrategy struct { |
There was a problem hiding this comment.
@aaron-congo can you confirm that the formatting for these comments is preserved when generating documentation?
There was a problem hiding this comment.
The line length is not preserved, but blank lines and indents are. So docs for the above look like this (you can get them by running go doc -all github.com/aws/glide-for-redis/go/glide/api
BackoffStrategy represents the strategy used to determine how and when to
reconnect, in case of connection failures. The time between attempts grows
exponentially, to the formula:
rand(0 ... factor * (exponentBase ^ N))
where N is the number of failed attempts.
Once the maximum value is reached, that will remain the time between retry
attempts until a reconnect attempt is successful. The client will attempt to
reconnect indefinitely.
There was a problem hiding this comment.
What about font for the coded section and "N". Can we use backticks (like Python) or add html tags (like <code> and <pre>)?
There was a problem hiding this comment.
With Go docs you create code blocks by indenting the relevant code, like I've done here. There is no inline code highlighting like you would get with backticks. When you upload the package the site will automatically render the Go docs into html, which includes rendering the indented code blocks as <pre> blocks
No description provided.