-
Notifications
You must be signed in to change notification settings - Fork 781
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[NET-9467, NET-9468] Add partitions + exportedServices funcs #1940
Conversation
c8a1fab
to
4a33247
Compare
Co-authored-by: Blake Covarrubias <[email protected]>
for exportedServices
@@ -121,7 +121,6 @@ func TestNewNVGetQuery(t *testing.T) { | |||
assert.Equal(t, tc.exp, act) | |||
}) | |||
} | |||
fmt.Println("done") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was unnecessary
exitCh <- m.Run() | ||
}() | ||
|
||
exit := <-exitCh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this channel shenanigans doesn't actually do anything
@@ -43,6 +43,20 @@ func TestMain(m *testing.M) { | |||
tb := &test.TestingTB{} | |||
runTestConsul(tb) | |||
clients := NewClientSet() | |||
|
|||
defer func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved the defer here so we actually catch panics during setup, this won't catch panics during test runs, I spent some time trying to get to a working solution but it would require checking for running instances of consul/nomad/vault that did not previously get cleaned up before running
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving, nothing blocking from my additional comments, but think you do need to address RB's comment about exported services implementing blocking queries. You'll want to remove that logic that does this select based on the sleep time.
dependency/consul_partitions.go
Outdated
|
||
// ListPartitionsQuerySleepTime is the amount of time to sleep between | ||
// queries, since the endpoint does not support blocking queries. | ||
ListPartitionsQuerySleepTime = 15 * time.Second |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[NIT] wonder if we want to create just one LongPollingSleepTime
set to 15 seconds for use across all of the non blocking query endpoints?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so we kinda can, we do some re-assigning of these within tests and I'm not 100% on how that will play out if these tests run in parallel (since we could get a concurrent read and write) what I did though was move this to a DefaultNonBlockingQuerySleepTime
constant and assign the individual sleep times to that const
default sleep time
Overview
This pull request adds two new functions to consul-template:
partitions
, which lists all partitions in the local datacenter using the existing/v1/partitions
APIexportedServices
, which lists all exported services in a given partition using the existing/v1/exported-services
APIExample
By combining these two functions, you can list the services imported into the local partition from all other partitions within the local datacenter.
Note
The following should probably be optimized with a
{{ break }}
to end therange
early once$localPartition
has been identified as a consumer of a given service; however, I haven't had a chance to test with that change in place yet, so I'm noting it here.