consulutil: Don't propagate a 0-length result #751
Conversation
petertseng
force-pushed
the
watch-no-empty
branch
2 times, most recently
from
5cbd5d7
to
534e397
Compare
Otherwise, failsafe triggers
We assume that there will never be a case where there are no results, so we shield our clients from potentially deleting all of their stored results.
petertseng
force-pushed
the
watch-no-empty
branch
from
534e397
to
2f6e285
Compare
| @@ -911,6 +911,92 @@ func TestMultipleFarms(t *testing.T) { | |||
| Assert(t).IsNil(err, "Expected pod not to have a dsID label") | |||
| } | |||
|
|
|||
| // Honestly, this test is a bit bogus. | |||
| // dsstoretest.NewFake doesn't use consulutil's WatchDiff, | |||
There was a problem hiding this comment.
my confidence in this test would be much increased if dsstoretest's WatchDiff and consulutil's WatchDiff would both use a common function, under which condition I would assume that they behave the same.
|
Given the concern I raised in #750 (comment) , this is my alternative. |
pkg/ds/farm.go
Outdated
| @@ -263,7 +263,7 @@ func (dsf *Farm) handleDSChanges(changes dsstore.WatchedDaemonSets, quitCh <-cha | |||
| } | |||
|
|
|||
| if len(changes.Created) > 0 { | |||
| dsf.logger.Infof("The following daemon sets have been created:") | |||
| dsf.logger.Infof("The following %d daemon sets have been created:", len(changes.Created)) | |||
There was a problem hiding this comment.
it might be nice to just list their IDs in this message instead of it getting interspersed with everything else. You could just %s a slice of ids
There was a problem hiding this comment.
I should move that commit to a separate PR, since it's gotten into both #750 and this one now.
petertseng
force-pushed
the
watch-no-empty
branch
from
2f6e285
to
5bf3425
Compare
| // We will not be sending this to the watcher. | ||
| // Assume it means something happened with Consul, and try again. | ||
| timer.Reset(2 * time.Second) // backoff | ||
| continue | ||
| } |
There was a problem hiding this comment.
while you're in here, maybe we should do a sanity check that queryMeta.LastIndex isn't less than currentIndex. Can save for a later PR if you desire
|
Let's revisit when we want to re-enable, and choose between this and #750 |
We assume that there will never be a case where there are no results, so
we shield our clients from potentially deleting all of their stored
results.
Test included, but is bogus.
Also, tests are failing for maybe-related reasons probably because there are other tests in which we delete the only daemon set. That is unfortunate.Dealt with that.