Tests and foundations for new cluster peer discovery #1311
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
thampiotr
force-pushed
the
thampiotr/cluster-dns-queries-v2
branch
from
5b78f1d
to
15e8037
Compare
thampiotr
force-pushed
the
thampiotr/cluster-dns-queries-v2
branch
from
15e8037
to
6c31bc4
Compare
thampiotr
changed the title
thampiotr
commented
Jul 30, 2024
Comment on lines
+87
to
+105
| switch { | ||
| case len(opts.JoinPeers) > 0 && opts.DiscoverPeers != "": | ||
| return nil, fmt.Errorf("at most one of join peers and discover peers may be set") | ||
|
|
||
| case len(opts.JoinPeers) > 0: | ||
| config.DiscoverPeers = newStaticDiscovery(opts.JoinPeers, listenPort, opts.Log) | ||
|
|
||
| case opts.DiscoverPeers != "": | ||
| discoverFunc, err := newDynamicDiscovery(config.Log, opts.DiscoverPeers, listenPort) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| config.DiscoverPeers = discoverFunc | ||
|
|
||
| default: | ||
| // Here, both JoinPeers and DiscoverPeers are empty. This is desirable when | ||
| // starting a seed node that other nodes connect to, so we don't require | ||
| // one of the fields to be set. | ||
| default: | ||
| // Here, both JoinPeers and DiscoverPeers are empty. This is desirable when | ||
| // starting a seed node that other nodes connect to, so we don't require | ||
| // one of the fields to be set. | ||
| } |
There was a problem hiding this comment.
The diff doesn't make it easy to see that this else is basically the original cluster peer discovery code.
| "github.com/go-kit/log" | ||
| ) | ||
|
|
||
| func TestPeerDiscovery(t *testing.T) { |
There was a problem hiding this comment.
These tests help to highlight some of the issues that I want to address on top of using A records in the follow-up PR.
thampiotr
commented
Jul 31, 2024
Comment on lines
+141
to
+142
| The `--cluster.use-discovery-v1` flag can be used to switch back to the older, v1 version of the cluster peer discovery mechanism | ||
| in case of any issues with the newer version. This flag will be deprecated in the future and eventually removed. |
There was a problem hiding this comment.
As I add support for A records, I will add here a note about what differences are between these mechanisms in the follow-up PR.
I will also add something to CHANGELOG.md once we actually have functional changes - this PR doesn't make any behaviour changes.
wildum
approved these changes
Jul 31, 2024
thampiotr
force-pushed
the
thampiotr/cluster-dns-queries-v2
branch
from
5a0900c
to
920382e
Compare
thampiotr
force-pushed
the
thampiotr/cluster-dns-queries-v2
branch
from
920382e
to
d722910
Compare
thampiotr
added a commit
that referenced
this pull request
Jul 31, 2024
This reverts commit 020713c.
thampiotr
added a commit
that referenced
this pull request
Jul 31, 2024
clayton-cornell
added
the
type/docs
thampiotr
added a commit
that referenced
this pull request
Aug 2, 2024
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Description
As part of this issue investigation: #1208 we found that there are missing tests and a few inconsistencies in the way that cluster peer discovery currently works.
Since this is a critical feature used by large scale users, we want to ensure that these are well-tested paths and bugs and inconsistencies are eliminated.
In order to isolate these changes and future refactoring, this PR:
This sets a foundation for further changes:
PR Checklist