-
Notifications
You must be signed in to change notification settings - Fork 0
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
WIP: Productionises distributed scheduler #22
Changes from all commits
cf104b8
4281ba2
e958d8b
cdba733
6915aaa
7c26a78
8c01b69
abcfd81
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,7 @@ ports: | |
protocol: TCP | ||
apiPort: 50006 | ||
etcdRPCClientPort: 2379 | ||
etcdHttpClientPort: 2330 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I considered the same, and it's open for discussion, but I thought a second scheduler instance would be using 2381 for the main (gRPC) port. So let's say if we had 5 instances, we'd have 2379, 2381, 2383, 2385 and 2387 for the client ports. That's why I chose a random port that's reasonably far away for http. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fair enough |
||
etcdRPCPeerPort: 2380 | ||
|
||
replicaCount: 1 | ||
|
@@ -31,6 +32,10 @@ cluster: | |
EtcdDataDirPath: /var/run/data/dapr-scheduler/etcd-data-dir | ||
EtcdDataDirWinPath: C:\\etcd-data-dir | ||
|
||
etcdSpaceQuota: 2147483648 | ||
etcdCompactionMode: periodic | ||
etcdCompactionRetention: 24h | ||
|
||
volumeclaims: | ||
storageSize: 1Gi | ||
storageClassName: | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -37,10 +37,14 @@ type Options struct { | |||||
PlacementAddress string | ||||||
Mode string | ||||||
|
||||||
EtcdID string | ||||||
EtcdInitialPeers []string | ||||||
EtcdDataDir string | ||||||
EtcdClientPorts []string | ||||||
Id string | ||||||
EtcdInitialPeers []string | ||||||
EtcdDataDir string | ||||||
EtcdClientPorts []string | ||||||
EtcdClientHttpPorts []string | ||||||
EtcdSpaceQuota int64 | ||||||
EtcdCompactionMode string | ||||||
EtcdCompactionRetention string | ||||||
|
||||||
Logger logger.Options | ||||||
Metrics *metrics.Options | ||||||
|
@@ -79,10 +83,14 @@ func New(origArgs []string) *Options { | |||||
fs.StringVar(&opts.PlacementAddress, "placement-address", "", "Addresses for Dapr Actor Placement service") | ||||||
fs.StringVar(&opts.Mode, "mode", string(modes.StandaloneMode), "Runtime mode for Dapr Scheduler") | ||||||
|
||||||
fs.StringVar(&opts.EtcdID, "id", "dapr-scheduler-server-0", "Scheduler server ID") | ||||||
fs.StringVar(&opts.Id, "id", "dapr-scheduler-server-0", "Scheduler server ID") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed this because we can have scheduler instances without a running etcd; it would feel weird to have an "etcdID" for them. |
||||||
fs.StringSliceVar(&opts.EtcdInitialPeers, "initial-cluster", []string{"dapr-scheduler-server-0=http://localhost:2380"}, "Initial etcd cluster peers") | ||||||
fs.StringVar(&opts.EtcdDataDir, "etcd-data-dir", "./data", "Directory to store scheduler etcd data") | ||||||
fs.StringSliceVar(&opts.EtcdClientPorts, "etcd-client-ports", []string{"dapr-scheduler-server-0=2379"}, "Ports for etcd client communication") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
if we are being explicit on http, it might be worth putting grpc here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is related to decision we're gonna make above (re: to having a default http port or not). If we do it like etcd does it and serve http on the same port as grpc by default, it wouldn't make sense to add |
||||||
fs.StringSliceVar(&opts.EtcdClientHttpPorts, "etcd-client-http-ports", []string{""}, "Ports for etcd client http communication") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we put a sane default here? or no and default is non production setup There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the review Cassie 🙏 |
||||||
fs.Int64Var(&opts.EtcdSpaceQuota, "etcd-space-quota", 2*1024*1024*1024, "Space quota for etcd") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For consistency, should we put the computed val here of
Suggested change
|
||||||
fs.StringVar(&opts.EtcdCompactionMode, "etcd-compaction-mode", "periodic", "Compaction mode for etcd. Can be 'periodic' or 'revision'") | ||||||
fs.StringVar(&opts.EtcdCompactionRetention, "etcd-compaction-retention", "24h", "Compaction retention for etcd. Can express time or number of revisions, depending on the value of 'etcd-compaction-mode'") | ||||||
|
||||||
opts.Logger = logger.DefaultOptions() | ||||||
opts.Logger.AttachCmdFlags(fs.StringVar, fs.BoolVar) | ||||||
|
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 looks good since its just changing the port var from the logic above I've thoroughly tested. I'm not sure if there is a way to pass in a var in the helm template helper logic.