opened
crhino
username_0
Doing some performance testing on the new CF Routing API, we found that under high load (~200 concurrent requests), the Watch function was missing a large amount of events. This translated into a long wait time for propagating routes from the API to the GoRouter. We spiked on adding a channel to the call and it improved performance dramatically.

Is there a reason why the current implementation does not use a channel to receive events from the etcd client?


@username_0 && @ebabani
Etcd Watch does not pass in a channel to etcd.Client.Watch()
issue
created
crhino
username_0
https://gist.github.com/username_0/96f2b76bc275b7726671

That is a diff of our changes for the spike. We tried to preserve the same behavior, although two tests were failing.
```
It("skips the missing event history and eventually catches up"
It("should write to the error channel"
```

The error channel  failure was because the etcd.client.Watch() closes the recvChan as well as returning an error, so there was a race condition about which case of the select statement would be triggered. The missing events history seems more relevant.
comment
created
ematpl
username_1
@onsi Do you know if there was a specific rationale behind always doing a one-off watch instead of passing a channel into the go-etcd client's Watch method? @vito and I looked through the commit history and we didn't see anything obvious. The only things I can think of are that it avoids creating another goroutine to handle receiving the etcd Response instances on a channel and transforming them storeadapter WatchEvent instances and may simplify the error case handling.
comment
created
ematpl
username_1
@username_0 Might also be related to https://github.com/coreos/go-etcd/issues/83, which was fixed in https://github.com/coreos/go-etcd/pull/108, and motivated https://github.com/cloudfoundry/storeadapter/commit/e6225ebbfbd3e6f60b2a974da6052b81fa4d819a.

@username_2 might have additional historical context?
comment
created
fraenkel
username_2
I fear you are trying to have a guarantee that can't ever be met with Watch. While you are seeing more of the events, eventually you will hit the next threshold that starts to miss the actual events.

The changes are fine. I don't know why someone didn't switch over to this call form. My guess is that it got lost waiting for the PR. There are some improvements that could be made like using the 2 form case on the select to determine the channel is closed rather than a nil value.

The reason why the first test fails is because you are receiving watch events from etcd when the events channel is blocked whereas the current code doesn't bother to do any watches if the events channel is all backed up. You have probably one extra event before it all backs up. My guess is by making events buffered, you would have seen a similar improvement.
comment
created
ematpl
username_1
@username_0 && @ebabani, if you're interested in submitting a PR to improve watch performance, we'd likely pull it in.

Thanks,
Eric
comment
closed
ematpl
username_1
issue
created
ematpl
username_1
Closing due to inactivity.
comment