implement task handling for master server's service #2245
Conversation
helinwang
requested review from
wangkuiyi,
jacquesqiao,
dzhwinter,
gongweibao and
typhoonzero
paddle/go/cmd/master/master.go
Outdated
| port := flag.Int("p", 0, "port of the master server") | ||
| dataset := flag.String("d", "", "dataset: comma separated path to RecordIO files") | ||
| faultTolerant := flag.Bool("fault-tolerance", false, "enable fault tolerance (requires etcd).") | ||
| flag.Parse() |
There was a problem hiding this comment.
Flags should be able to load from environment variables, so that we can run on cloud.
There was a problem hiding this comment.
Understood. I did a search, it seems that this https://github.com/spf13/viper repo has many stars. Not sure if it might be helpful.
There was a problem hiding this comment.
I will add reading flag being load from env variables. @typhoonzero just curious, how good is k8s' support for pod command line arguments? And what are the best practices for building cloud-native applications? (I think you mentioned pass arguments by env variable, and print log to stdout before. Do you have any good article in mind from which I could read more detail?)
There was a problem hiding this comment.
@typhoonzero @wangkuiyi Done. Using https://github.com/namsral/flag, it's a drop-in replacement for flag, adding support for environment variable and plain config file. I think https://github.com/spf13/viper probably is an overkill for the current situation (viper supports loading and monitoring config etcd, ...) And it have different syntax than the offical flag package, harder to learn / maintain.
There was a problem hiding this comment.
the best practices for building cloud-native applications
The most famous one is "12-factor" app: https://12factor.net
For apps on kubernetes, there are generally 3 ways to pass arguments to program:
- Via environment variables, kubernetes can also pass pod dynamic status info through environment variables using downwardAPI
- Via command-line arguments, specify arguments manually
- Via configuration files through mounting a "config map" to pod
paddle/go/cmd/master/master.go
Outdated
|
|
||
| func main() { | ||
| port := flag.Int("p", 0, "port of the master server") | ||
| dataset := flag.String("d", "", "dataset: comma separated path to RecordIO files") |
There was a problem hiding this comment.
Should dataset be a list of dirs, for file lists may be very large. It's even better to user wildcards to specify dataset, like /path/to/my/files/myfile-00*.recordio
There was a problem hiding this comment.
-d ==> -training_dataset?
There was a problem hiding this comment.
@typhoonzero Done. Added support for glob pattern. But not supporting list of dirs, I think directory is too coarse, it's hard to specify what should happen when there is a non-recordIO format under that directory.
@wangkuiyi Done.
paddle/go/master/service.go
Outdated
| ) | ||
|
|
||
| const ( | ||
| targetTaskCount = 300 |
There was a problem hiding this comment.
targetTaskCount should be configurable. Or config chunkPerTask
There was a problem hiding this comment.
Done. Changed to chunksPerTask.
paddle/go/cmd/master/master.go
Outdated
| ) | ||
|
|
||
| func main() { | ||
| port := flag.Int("p", 0, "port of the master server") |
There was a problem hiding this comment.
Maybe we can specify a default port instead of a random port, such as 8080.
There was a problem hiding this comment.
Should we use long names for command line parameters? For example, -p to -addr?
There was a problem hiding this comment.
@Yancey1989 Good point! Done.
@wangkuiyi Agree with long names, changed to port, maybe port is more precise than addr?
No description provided.