Small HA tracker cleanup #3808
Small HA tracker cleanup #3808
Conversation
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
pkg/distributor/ha_tracker_test.go
Outdated
| ctx, cancel = context.WithTimeout(ctxUser2, time.Second) | ||
| err = checkReplicaTimestamp(ctx, c, user, cluster, replica, start) | ||
| cancel() | ||
| err = c.checkReplica(context.Background(), user, cluster, replica, now) |
There was a problem hiding this comment.
We're not passing a different user here. The test is supposed to test the HA tracker with different users. I'm a bit puzzled.
There was a problem hiding this comment.
You're right, good catch. I'm bit puzzled as well... I don't think old test was really doing what the test name says. I'll update it to do so.
| require.NoError(t, err) | ||
|
|
||
| // Stop the watcher | ||
| cancel() |
There was a problem hiding this comment.
If we cancel immediately there's no guarantee the WatchPrefix() has actually had the time to receive the update. I think we should wait "a bit".
There was a problem hiding this comment.
You're right, and it's fixed in #3809, but let's add a wait here.
| require.NoError(t, err, "object could not be created") | ||
|
|
||
| // Now delete it | ||
| err = client.Delete(context.Background(), "key-to-delete") |
There was a problem hiding this comment.
Should we add a sleep between creating the key-to-delete and deleting it? I'm wondering what happens if the two operations happens very close each other: the watcher may not get any notification (depending on how the backend is implemented).
There was a problem hiding this comment.
OK, let's do that.
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
|
Thanks for reviews, I've addressed your feedbacks, please take a look again. |
There was a problem hiding this comment.
LGTM (modulo the comments about the nooplogger in tests).
I haven't checked, but if the pkg/distributor is not using the global logger anymore than please add it to the failling "rule" in Makefile:
faillint -paths "github.com/cortexproject/cortex/pkg/util/log.{Logger}" \
./pkg/ingester/... \
./pkg/flusher/... \
./pkg/distributor/...
We're not quite there yet due to logger in |
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
What this PR does: This PR does some small code "cleanup" in HA tracker (metrics as fields, logger), and tests (removes usage of
mtime, usestest.Poll). In addition to that, there is new integration test which doesn't verify behaviour, only logs it -- it shows differences in WatchPrefix and Delete behaviour between Consul (existing keys are returned via WatchPrefix, Delete doesn't send "notification" to WatchPrefix) and etcd (existing keys are not returned via WatchPrefix, but Delete "sends" notification).Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]