-
Notifications
You must be signed in to change notification settings - Fork 466
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
Add server type blacklist and concurrent server get on etcd to improv… #119
Conversation
} | ||
|
||
// We sleep to guarantee that the servers were added to etcd | ||
time.Sleep(time.Millisecond * 50) |
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 may make the test flaky, don't you think?
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.
The server is in memory, so this should be a very large margin. Do you think something else is required?
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.
Oh, the cli.Put method is synchronous, so the sleep is probably unnecessary.
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.
there are better ways of waiting in a test, something like that:
https://github.com/topfreegames/pitaya/blob/master/cluster/nats_rpc_client_test.go#L158
let me know what you think
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.
other example
pitaya/session/session_test.go
Line 612 in 931f288
helpers.ShouldEventuallyReturn(t, s.NumSubscriptions, uint32(1)) |
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.
there is no simple way of doing that because for some reason sync.Map
does not have a Len method. But I'll see a way of doing it.
…fter inserting keys on etcd.
…iscovery-startup-time
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.
Good work @leohahn. I left some comments.
Please, include the information about the configs you are adding in the documentation:
https://github.com/topfreegames/pitaya/blob/master/docs/configuration.rst
Next time please split the PR, e.g. PR1 - Server blacklist === EDIT also add a proper description to them |
} | ||
|
||
// We sleep to guarantee that the servers were added to etcd | ||
time.Sleep(time.Millisecond * 50) |
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.
there are better ways of waiting in a test, something like that:
https://github.com/topfreegames/pitaya/blob/master/cluster/nats_rpc_client_test.go#L158
let me know what you think
cluster/etcd_service_discovery.go
Outdated
sd.serverTypesBlacklist = sd.config.GetStringSlice("pitaya.cluster.sd.etcd.servertypeblacklist") | ||
sd.syncServersParallelism = sd.config.GetInt("pitaya.cluster.sd.etcd.syncserversparallelism") | ||
|
||
if sd.syncServersParallelism == 0 { |
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.
Why do we still have this if? Aren't you specifying a default for that already?
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.
good job, lgtm
…e startup time.