FileManager #112

gongweibao · 2017-05-30T03:49:12Z

No description provided.

helinwang · 2017-05-30T18:38:05Z

go/cmd/pfsserver/main.go

+package main
+
+import (
+	"flag"


Please use "github.com/namsral/flag" (a drop in replacement of "flag") instead of "flag", since the former can parse env variable as input.
Please see this discussion: PaddlePaddle/Paddle#2245 (comment)

Usage of "github.com/namsral/flag" see: https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/go/cmd/pserver/pserver.go#L9

@helinwang @typhoonzero github.com/golang/glog里边判断了

679 if !flag.Parsed() { 680 os.Stderr.Write([]byte("ERROR: logging before flag.Parse: ")) 681 os.Stderr.Write(data) 682 }

而它用的是go原生的flag，所以用了github.com/namsral/flag，每一行打印中都会出现类似
ERROR: logging before flag.Parse: I0531 11:34:47.868818 3294 main.go:21]

鉴于需要使用glog的level日志的功能，同时flag的功能已经足够用了。先用flag。

Ok. It's fine as long as we don't need to parse environment variable for file server.

helinwang · 2017-05-30T18:42:18Z

go/cmd/pfsserver/main.go

+
+	router := pfsserver.NewRouter()
+
+	portPtr := flag.Int("port", 8080, "listen port")


port would suffice. Go prefer short variable names. See: https://github.com/golang/go/wiki/CodeReviewComments#variable-names

helinwang · 2017-05-30T18:44:55Z

go/filemanager/pfsmod/chunk.go

+	"strconv"
+)
+
+//ChunkCmd structure


The code next line already shows it's a struct, I think we need a more descriptive comment.
Also, please leave a space after "//": // comment.
Please see: https://github.com/golang/go/wiki/CodeReviewComments#comment-sentences

helinwang · 2017-05-30T18:57:34Z

go/filemanager/pfsmod/chunk.go

+
+//ToJSON encodes chunkcmd to json string
+func (p *ChunkCmd) ToJSON() ([]byte, error) {
+	return nil, nil


Why it's returning nil, nil, is it not implemented? If so, do you plan to implement it later? If so, maybe panic("not implemented") is better.

helinwang · 2017-05-30T18:59:37Z

go/filemanager/pfsmod/command.go

+
+const (
+	//DefaultMultiPartBoundary is the default multipart form boudary
+	DefaultMultiPartBoundary = "8d7b0e5709d756e21e971ff4d9ac3b20"


It's only used by package pfsserver, why not put it there?

这个是paddlecloud package和pfsserver package都用到了，所以放到了pfsmod package中

明白了。

helinwang · 2017-05-30T19:01:28Z

go/filemanager/pfsmod/chunk.go

+//NewChunkCmdFromURLParam get a ChunkCmd structure
+// path example:
+// 	  path=/pfs/datacenter1/1.txt&offset=4096&chunksize=4096
+func NewChunkCmdFromURLParam(path string) (*ChunkCmd, error) {


NewChunkCmdFromURLParam is a very long name, maybe ParseChunkCmd is a better name?

helinwang · 2017-05-30T19:05:58Z

go/filemanager/pfsmod/chunk.go

+
+//Run function runs a ChunkCmd
+func (p *ChunkCmd) Run() (interface{}, error) {
+	return nil, nil


Why it's returning nil, nil, is it not implemented? If so, do you plan to implement it later? If so, maybe panic("not implemented") is better.

helinwang · 2017-05-30T19:07:16Z

go/filemanager/pfsmod/chunk.go

+)
+
+//ChunkCmd structure
+type ChunkCmd struct {


I think we need a comment that describe what is ChunkCmd, I am confused why there is "cmd", from the definition, I can not see what "cmd" does?

helinwang · 2017-05-30T19:08:45Z

go/filemanager/pfsmod/ls.go

+	cmd := LsCmd{}
+
+	m, err := url.ParseQuery(path)
+	if err != nil ||


if err != nil just indicate the parsing failed, it's not "not enough arguments", maybe we should use a different StatusText for it?

helinwang · 2017-05-30T19:10:13Z

go/filemanager/pfsmod/chunk.go

+		return nil, errors.New(StatusText(StatusJSONErr))
+	}
+
+	//var err error


This line should be removed.

helinwang · 2017-05-30T19:13:27Z

go/filemanager/pfsmod/status.go

+// need a custome error type?
+
+const (
+	StatusFileNotFound          = 520


好像只有在提取statusText的时候采用到了error code，还有其他时候需要用error code吗？
如果是这样的话，需要把error code暴露出package之外吗，以及，有必要有error code这个东西吗？

这个我也在犹豫，是否应该定义一个自己的error类型，errno用来判定，errtext用来提示。所以上边写了一个注释:need a custom error type?

感觉直接用HTTP的状态码也是可以的。

Done.
这一版先用text。等我找到更好的办法的时候再重构一下。

helinwang · 2017-05-30T19:14:27Z

go/filemanager/pfsmod/chunkmeta.go

+			break
+		}
+
+		//log.Println(n)


Please no commented out code.

helinwang · 2017-05-30T19:15:23Z

go/filemanager/pfsmod/chunkmeta.go

+	defaultMaxChunkSize = 4 * 1024 * 1024
+	defaultMinChunkSize = 4 * 1024
+	//DefaultChunkSize is the default chunk's size
+	DefaultChunkSize = 2 * 1024 * 1024


DefaultChunkSize只被另一个package用到，是不是该放在另一个package里面？

哦。有道理。我本来是想把关于同一个主题的都放到一个文件中。

Yancey1989 · 2017-05-31T02:22:12Z

go/filemanager/pfsmod/resp.go

+package pfsmod
+
+//JSONResponse is the common response from server
+type JSONResponse struct {


The result field type of all response struct is interface{} in Go, so maybe we can only need one struct named:

type Response struct { Err string `json:"err"`, Result interface{} `json:"result"` }

有道理。只是这个parse的时候会出错。可以把resp.go的内容重新分配了一下，删除了这个文件。

Yancey1989 · 2017-05-31T02:25:36Z

go/filemanager/pfsmod/status.go

+// need a custome error type?
+
+const (
+	StatusFileNotFound          = 520


感觉直接用HTTP的状态码也是可以的。

typhoonzero · 2017-05-31T01:42:49Z

go/filemanager/pfsmod/chunk.go

+type ChunkCmd struct {
+	Path      string
+	Offset    int64
+	ChunkSize int64


ChunkSize may be a global const or read from some command-line arguments or config file.

Else if you intend to save the size of current chunk, it should be named size

typhoonzero · 2017-05-31T01:51:16Z

go/filemanager/pfsmod/chunk.go

@@ -0,0 +1,118 @@
+package pfsmod


This package name filemanager/pfsmod seems confusing. You should name it like pfs/pfscommands or pfs/pfsmodules.

typhoonzero · 2017-05-31T01:51:58Z

go/filemanager/pfsmod/chunk.go

+	return err
+}
+
+//SaveChunkData save data from r


SaveChunkData save data from r => SaveChunkData save data from io.Reader

typhoonzero · 2017-05-31T01:54:24Z

go/filemanager/pfsmod/chunk.go

+	return &cmd, nil
+}
+
+//LoadChunkData loads a specified chunk to w


LoadChunkData loads a specified chunk to w => LoadChunkData loads a specified chunk to io.Writer

typhoonzero · 2017-05-31T02:30:47Z

go/filemanager/pfsmod/chunkmeta.go

@@ -0,0 +1,221 @@
+package pfsmod


chunkmeta definations should also add to design doc: https://github.com/PaddlePaddle/Paddle/tree/develop/doc/design/file_manager

这个后续补上。

helinwang · 2017-05-31T19:47:43Z

go/cmd/pfsserver/main.go

+)
+
+func main() {
+


Nitpicking here, I know new line can separate regions of code of different functionalities, but a new line at the beginning of a function does not seem necessary?

Done
习惯于大括号在第二行居左了。

helinwang · 2017-05-31T19:48:40Z

go/cmd/pfsserver/main.go

+func main() {
+
+	port := flag.Int("port", 8080, "port of server")
+	ip := flag.String("ip", "0.0.0.0", "ip of server")


Just curious, when will the ip of server necessary? I usually do ":8080" rather than "0.0.0.0:8080" so that I don't need to specify the ip.

ip port都可以不用指定。中间的是他们的默认值。

Listening on an IP address is always useful, ":8080" is the same as "0.0.0.0:8080". When we need to bind TCP server to the specified network(in a LAN or VLAN or VPC), we need to specify the IP of the network card.

helinwang · 2017-05-31T19:50:49Z

go/filemanager/pfsmodules/chunk.go

+	log "github.com/golang/glog"
+)
+
+// Chunk respresents a chunk info


Please see https://github.com/golang/go/wiki/CodeReviewComments#comment-sentences , comments should be a full sentence. (end with a period ".")

helinwang · 2017-05-31T19:54:13Z

go/filemanager/pfsmodules/status.go

+// need a custom error type?
+
+const (
+	StatusFileNotFound          = "no such file or directory"


Does these errors needs to be public (visible outside of the package)?
Typically errors are used:

var ErrFileNotFound = errors.New("no such file or directory")

See: https://golang.org/src/io/io.go

But most of the time error do not need to be exported, since people who use the library does not need to differentiate between difference errors.

error类型没法序列化到JSON以及从JSON中反序列化。

确实需要区分错误：如文件不存在。

@gongweibao 我查了一下，貌似现在的code并没有把这些status序列化到json？如果需要序列化的话，err.Error()是不是更符合Go idiom?

helinwang · 2017-05-31T20:00:05Z

go/filemanager/pfsmodules/chunk.go

+	}
+
+	writen, err := io.CopyN(w, f, p.Size)
+	log.V(2).Infof("writen:%d\n", writen)


I thought this function loads data, why here says "written"? Maybe "loaded" is more appropriate?

helinwang · 2017-05-31T20:15:14Z

go/filemanager/pfsmodules/command.go

+
+const (
+	// DefaultMultiPartBoundary is the default multipart form boudary
+	DefaultMultiPartBoundary = "8d7b0e5709d756e21e971ff4d9ac3b20"


I think in general const block can be put together:

const ( DefaultMultiPartBoundary = "8d7b0e5709d756e21e971ff4d9ac3b20" MaxJSONRequestSize = 2048

If you want to write them separately, you can do:

const DefaultMultiPartBoundary = "8d7b0e5709d756e21e971ff4d9ac3b20" const MaxJSONRequestSize = 2048

helinwang · 2017-05-31T20:22:28Z

go/filemanager/pfsserver/handler.go

+func writeJSONResponse(w http.ResponseWriter,
+	req string,
+	httpStatus int,
+	resp *response) {


Maybe pass by value is better resp response. Please see https://github.com/golang/go/wiki/CodeReviewComments#pass-values

helinwang · 2017-05-31T20:24:34Z

go/filemanager/pfsserver/handler.go

+
+// PostFilesHandler processes files' POST request
+func PostFilesHandler(w http.ResponseWriter, r *http.Request) {
+


Maybe this empty line is not necessary.

helinwang · 2017-05-31T20:26:22Z

go/filemanager/pfsserver/handler.go

+	result, err := cmd.Run()
+	if err != nil {
+		resp.Err = err.Error()
+		resp.Results = result


Isn't result invalid when err != nil?

helinwang · 2017-05-31T20:32:18Z

go/filemanager/pfsmodules/ls.go

+
+// Run functions runs LsCmd and return LsResult and error if any happened
+func (p *LsCmd) Run() (interface{}, error) {
+	results := make([]LsResult, 0, 100)


Why the capacity is set to 100? Please do not optimize at the cost of code clarity if it's not identified as a bottleneck.
var results []LsResult is probably more appropriate. (calling append / len using a nil slice is fine)

typhoonzero · 2017-06-01T05:30:39Z

go/paddlecloud/pfssubmit.go

+		return nil, err
+	}
+
+	url := fmt.Sprintf("%s:%d/api/v1/files", s.config.ActiveConfig.Endpoint, s.port)


No need to add s.port. Endpoint already contains it.

typhoonzero · 2017-06-01T05:31:16Z

go/paddlecloud/pfssubmit.go

+	return &pfsSubmitter{
+		config: config,
+		client: client,
+		port:   8080,


Port is hard coded

typhoonzero · 2017-06-01T05:40:40Z

go/paddlecloud/pfssubmit.go

@@ -0,0 +1,246 @@
+package paddlecloud


PFS operations are not "submits", so we don't need a pfsSubmitter

submitter其实是提交命令的意思。接口是面向Command interface的。这个接口部分我觉得可以保留。

还是有点牵强。一般会说submit a job, submit a task，run a command. 如果都是HTTP请求，直接调用Get, Post函数即可，可以省掉这个struct

typhoonzero · 2017-06-01T05:41:09Z

go/paddlecloud/pfssubmit.go

+}
+
+// PostFiles implements files' POST method.
+func (s *pfsSubmitter) PostFiles(cmd pfsmod.Command) ([]byte, error) {


You can put HTTP client operations in restclient.go, I'll do some refactor to restclient.go so it is more flexible.

OK。pfssubmitter里边可以调用这几个GetCall和PostCall等几个函数。

typhoonzero · 2017-06-01T05:50:18Z

go/filemanager/pfsserver/handler.go

+)
+
+type response struct {
+	Err     string      `json:"err"`


We need to use a same form of general response, like:

{ "code": 0, "msg": "OK or Error", "items": {...}}

这个response就是一个general response。Results是一个interface，不赋值的时候就是nil。JSON解析没有问题，而且这个时候客户端是不需要知道具体的Results是什么。
code这个思路不太可行。golang里边都是error对象，是没有error code的。如果要传error code就要在所有return err的地方做一个转换

是说需要一个规范性质的，描述所有的REST response json的格式。包括pfs和submit等API。如果没有code 也可以，是需要一个统一的返回格式。

可以考虑：

type responseStatus struct { Err string `json:"msg"` } type response { responseStatus Results interface{} `json:"results"` } // ... type chunkMetaResponse struct { responseStatus Results []pfsmod.ChunkMeta `json:"results"` }

这样的好处是，可以确保所有的HTTP Response都有固定的status格式。方便client实现统一的parsing logic（client需要parse了status再决定下一步干什么，是报错还是继续执行）。

helin，这个没有太理解。Err放到一个结构体中和单独出来的区别是？

typhoonzero · 2017-06-01T06:20:36Z

go/filemanager/pfsmodules/ls.go

+}
+
+// CloudCheck checks the conditions when running on cloud.
+func (p *LsCmd) CloudCheck() error {


CloudCheck() and LocalCheck() seems the same at mkdir.go, rm.go etc. with slitly differences, can we make it more simpler?

By the way, I don't quite understand what does "checks the conditions when running on cloud." means.

命令面向用户的基本设定都是在Cloud端运行，所以按照道理是不需要LocalCheck的

不过，像Ls这样的命令，cp用到了本地其本地运行的结果，所以把命令抽象为interface以后，加入了LocalCheck和CloudCheck，以便

在Local只需要检查参数的个数等，而在Cloud上需要检查用户的对目录的权限。其实是不一样的。

我的理解：

检查参数一般都叫作：validate，如果只是检查参数合法性，是要实现validate函数，比如检查dest是否是/pfs开始

服务端检查用户对目录的权限，只需要比较认证后的username == path[3]即可

可以缩短。

helinwang · 2017-06-01T22:11:23Z

go/filemanager/pfsmodules/command.go

+}
+
+// Close closes c and log it.
+func Close(c io.Closer) {


Close should not be visible from outside of the package.

我的思路是这样的：有两个package中用到这个函数，为了防止两个包相互调用，所以定义了一个pfsmodules作为中间的公共package，然后把公共函数放到了它里边。

这种简单的函数，就直接放在package内吧。另外，防止相互调用往往会用interface（好像挺少因为这个再去加一个新的package，创建新package的目的可能是为了模块化，而不是解决循环依赖），比如：

package a import "b" var c = b.Foo() type T int func (t T) Bar() { }

package b type interface Bar { Bar() } // can't import "a" and use a.T as an argument, using a interface. func Baz(b Bar) { } func Foo() { }

package main func main() { b.Baz(a.T(0)) }

往往必须有循环依赖的时候还是比较少的。

helinwang · 2017-06-01T22:14:35Z

go/filemanager/pfsmodules/chunkmeta.go

+// CloudCheck checks the conditions when running on cloud.
+func (p *ChunkMetaCmd) CloudCheck() error {
+	if !IsCloudPath(p.FilePath) {
+		return errors.New(StatusShouldBePfsPath + ": p.FilePath")


maybe should be return errors.New(StatusShouldBePfsPath + ":" + p.FilePath)

惭愧，搞错了。
Done

helinwang · 2017-06-01T22:26:08Z

go/filemanager/pfsmodules/chunkmeta.go

+		return nil, err
+	}
+
+	metas := make([]ChunkMeta, 0, fi.Size()/len+1)


Sorry I don't understand why there is a +1 in fi.Size()/len+1, I thought (fi.Size() + len - 1)/len is the ceil(fi.Size()/len).
Anyway, I think var metas []ChunkMeta is more fool proof, and easy to read. The optimization here will not have any visible impact to the performance. We should choose code clarity rather than performance, unless performance is a bottleneck.

helinwang · 2017-06-01T22:32:10Z

go/filemanager/pfsmodules/cp.go

+		if flag.Name == "v" {
+			cmd.V, err = strconv.ParseBool(flag.Value.String())
+			if err != nil {
+				panic(err)


https://github.com/golang/go/wiki/CodeReviewComments#dont-panic

I think in general no code should panic besides maybe it's ok if code in package main.

Visit这个函数没有返回值可以供使用。
用goto跳出来？

Maybe:

var err error f.Visit(func(flag *flag.Flag) { if err != nil { return } if flag.Name == "v" { cmd.V, err = strconv.ParseBool(flag.Value.String()) } }) if err != nil { return nil, err }

多谢helin。Done。

helinwang · 2017-06-01T22:33:18Z

go/filemanager/pfsmodules/ls.go

+}
+
+// NewLsCmdFromFlag returen a new LsCmd.
+func NewLsCmdFromFlag(f *flag.FlagSet) (*LsCmd, error) {


Need test case, it's fine to have it in another PR.

OK，我记录了。

helinwang · 2017-06-01T22:37:10Z

go/filemanager/pfsmodules/stat.go

+
+	return &LsResult{
+		Path:    p.Path,
+		ModTime: fi.ModTime().Format("2006-01-02 15:04:05"),


Why not use info.ModTime().UnixNano(), which is more precise. We only need string when print out the data.

helinwang · 2017-06-01T22:42:22Z

go/filemanager/pfsserver/handler.go

+)
+
+type response struct {
+	Err     string      `json:"err"`


可以考虑：

type responseStatus struct { Err string `json:"msg"` } type response { responseStatus Results interface{} `json:"results"` } // ... type chunkMetaResponse struct { responseStatus Results []pfsmod.ChunkMeta `json:"results"` }

这样的好处是，可以确保所有的HTTP Response都有固定的status格式。方便client实现统一的parsing logic（client需要parse了status再决定下一步干什么，是报错还是继续执行）。

helinwang · 2017-06-01T22:45:06Z

go/paddlecloud/pfsmkdir.go

+}
+
+// Execute runs a MkdirCommand.
+func (p *MkdirCommand) Execute(_ context.Context, f *flag.FlagSet, _ ...interface{}) subcommands.ExitStatus {


What is the purpose for context.Context, is it for cancellation, if so, why it's not implemented?

这个是subcommands package里边Command interface要求实现的。

helinwang · 2017-06-01T22:45:46Z

go/paddlecloud/pfssubmit.go

+func newPfsCmdSubmitter(configFile string) *pfsSubmitter {
+	// TODO
+	/*https
+	pair, e := tls.LoadX509KeyPair(config.ActiveConfig.Usercert,


Please no commented out code. You can put the code on Evernote :)

helinwang · 2017-06-01T22:46:00Z

go/paddlecloud/pfssubmit.go

+
+	log.V(1).Infof("%#v\n", config)
+
+	//http


This comment does not provide any information.

Yancey1989 · 2017-06-02T02:06:16Z

With this PR, the user should configure two kinds of authentication: basic auth means username && password and tls key files at the same time, It look so confusing.
Because of the cloud server has already use basic auth as the authentication mechanism, so the PFS can also use the it, the main steps is as following:

call the cloud server auth API /api-token-auth/ with username and password, and get the token string.
call the cmd API with the token string.

helinwang · 2017-06-07T01:11:03Z

go/filemanager/pfsmodules/status.go

+// need a custom error type?
+
+const (
+	StatusFileNotFound          = "no such file or directory"


I am still not convinced that we need these string constants, and these string constant need to be visible outside of the package.
Can you take a look at : #112 (comment)

After offline discussion with @gongweibao , it's used by the client to compare these string with the error message returned from the server over HTTP.

多谢helin.

helinwang · 2017-06-07T01:19:11Z

go/filemanager/pfsmodules/chunkmeta.go

+}
+
+// ToURLParam encodes ChunkMetaCmd to URL encoding string.
+func (p *ChunkMetaCmd) ToURLParam() url.Values {


These code are repetitive, a good candidate for automatically generating them. See: https://blog.golang.org/generate

Not required, just want to let you know :)

棒。我正要模仿JSON的Marshal函数自己搞一个那。多谢helin。
后续改进它。

helinwang · 2017-06-07T01:20:56Z

go/paddlecloud/download.go

+}
+
+// DownloadFile downloads src to dst. If dst does not exists, create it with srcFileSize.
+func DownloadFile(src string, srcFileSize int64, dst string) error {


Is this necessary to be visible outside of the package?

helinwang · 2017-06-07T01:21:12Z

go/paddlecloud/download.go

+}
+
+// Download function downloads src to dst.
+func Download(src, dst string) error {


Is this necessary to be visible outside of the package?

helinwang

LGTM after the two function being visible outside the package comment.

typhoonzero · 2017-06-07T03:16:09Z

go/filemanager/pfsserver/handler.go

+	Results interface{} `json:"results"`
+}
+
+func makeRequest(uri string, method string, body io.Reader,


You can call this function from package paddlecloud instead of copy again.

会引入很多不必要的用来，比如全局变量config。
或者把restclient放到公共的地方去。

这个merge进去以后，下个版本继续改正。不然改动太大，容易出问题。

typhoonzero · 2017-06-07T03:18:53Z

go/filemanager/pfsserver/handler.go

+	return req, nil
+}
+
+func getResponse(req *http.Request) ([]byte, error) {


You can call this function from package paddlecloud instead of copy again.

会引入很多不必要的用来，比如全局变量config。
或者把restclient放到公共的地方去

这个merge进去以后，下个版本继续改正。不然改动太大，容易出问题。

typhoonzero · 2017-06-07T03:32:14Z

go/filemanager/pfsserver/handler.go

+func getUserName(uri string, token string) (string, error) {
+	authHeader := make(map[string]string)
+	authHeader["Authorization"] = "Token " + token
+	req, err := makeRequest(uri, "GET", nil, "", nil, authHeader)


Can call makeTokenRequest directly.

会引入很多不必要的用来，比如全局变量config。
或者把restclient放到公共的地方去

这个merge进去以后，下个版本继续改正。不然改动太大，容易出问题。

typhoonzero · 2017-06-07T03:32:50Z

go/filemanager/pfsserver/handler.go

+		return "", err
+	}
+
+	log.V(4).Infoln("before getResponse")


This debug log maybe not needed?

typhoonzero · 2017-06-07T05:49:09Z

go/filemanager/pfsserver/handler.go

+func GetChunkHandler(w http.ResponseWriter, r *http.Request) {
+	log.V(1).Infof("begin proc GetChunkHandler")
+
+	cmd, err := pfsmod.ParseChunk(r.URL.RawQuery)


ParseChunk returns a *Chunk type but the variable is cmd. Previous review I commented ChunkCmd is confusing, so we changed ChunkCmd to Chunk. But when using it we still treat it as a Cmd.

Don't know how to do.

typhoonzero · 2017-06-07T05:56:04Z

go/filemanager/pfsserver/handler.go

+	}
+
+	switch method {
+	case "rm":


Why "rm", "touch", "mkdir", shares the same URL route and use the same handler?

In REST, resource removal methods should use DELETE HTTP method.

typhoonzero · 2017-06-07T05:58:58Z

go/filemanager/pfsserver/handler.go

+	switch method {
+	case "rm":
+		rmHandler(w, body, r.Header)
+	case "touch":


touchHandler can reuse PostChunkHandler to create an empty file?

语义不一样。一个是创建文件，一个是上传Chunk。
touch创建的是sized的文件。

typhoonzero · 2017-06-07T06:18:25Z

go/filemanager/pfsmodules/command.go

+
+// Command is a interface of all commands.
+type Command interface {
+	ToURLParam() url.Values


Better to add comments for each interface usages? like
// ToURLParam convert the current pfs command to a url.Values struct which will be used by pfs server and client.

typhoonzero · 2017-06-07T06:21:46Z

go/filemanager/pfsmodules/command.go

+
+// CheckUser checks if a user has authority to access a path.
+func checkUser(path string, user string) error {
+	// TODO


Is this function still a TODO?

typhoonzero · 2017-06-07T06:27:08Z

go/paddlecloud/pfsls.go

@@ -0,0 +1,131 @@
+package paddlecloud


Can we merge content of these files(pfscp.go, pfsls.go, pfsmkdir.go, pfsrm.go) to filemanager/pfsmodules/*.go?

For example, in ls.go:

// LsCmd means LsCommand structure. type LsCmd struct { Method string R bool Args []string } // for subcommand func (*LsCmd) Name() string { return "ls" } func (*LsCommand) Synopsis() string { return "List files on PaddlePaddle Cloud" } ... // original command implementations func (p *LsCmd) ToURLParam() url.Values { ...

In cmd/paddlecloud/paddlecloud.go:

subcommands.Register(&pfsmod.LsCmd{}, "")

这个merge进去以后，下个版本继续改正。不然改动太大，容易出问题。

typhoonzero

LGTM

gongweibao added 20 commits May 26, 2017 10:56

init add

a53328f

add some

e589e33

add some

4b8572b

add touch

24c303f

fix some bugs

2081766

fix some bugs

c09e0cf

compile ok

23c1177

rm not need

380b942

fix ls bugs

bac9cc8

ls ok

b96c270

prepare add stat

c3dbe34

fix bugs

f756307

upload ok

4ab2a7f

download ok

463df28

rm ok

ff95f05

mkdir ok

b2d919f

rename files

419a678

fix by gometalinter

cb75cac

del not need code

737f7a3

rm not need codes

3636d04

gongweibao requested review from Yancey1989, helinwang and typhoonzero May 30, 2017 03:58

helinwang reviewed May 30, 2017

View reviewed changes

Yancey1989 reviewed May 31, 2017

View reviewed changes

typhoonzero requested changes May 31, 2017

View reviewed changes

fix by comments

9c41efc

helinwang requested changes May 31, 2017

View reviewed changes

fix by helin's comments

42b0890

typhoonzero reviewed Jun 1, 2017

View reviewed changes

helinwang reviewed Jun 1, 2017

View reviewed changes

gongweibao added 12 commits June 2, 2017 11:34

Merge remote-tracking branch 'upstream/develop' into filemanager2

026669a

fix by comments

7e5f9df

prepar use restclient

db395cd

prepare use restclient

6f219e9

Merge remote-tracking branch 'upstream/develop' into filemanager2

d6b2bd0

fix restclient and compile ok

bbee71e

test commands ok

20d13a4

fix by comments and add check user

cfd5767

fix by gometa

2750df9

rm no need code

1b3aa33

fix visit to return error

7a32638

Errorf to Errorln

a17e0b1

helinwang reviewed Jun 7, 2017

View reviewed changes

helinwang previously approved these changes Jun 7, 2017

View reviewed changes

rename some funtions that not need export out package

8966691

gongweibao dismissed helinwang’s stale review via 8966691 June 7, 2017 01:54

rm not need code

2485891

typhoonzero reviewed Jun 7, 2017

View reviewed changes

gongweibao added 3 commits June 7, 2017 16:20

fix by wuyi comments

4c7cf4e

add comments of Command interface

930b8cd

Merge branch 'develop' into filemanager2

00880ae

typhoonzero approved these changes Jun 7, 2017

View reviewed changes

gongweibao merged commit f647640 into PaddlePaddle:develop Jun 7, 2017

gongweibao mentioned this pull request Jun 20, 2017

Add test cases of FileManager #156

Closed


		router := pfsserver.NewRouter()

		portPtr := flag.Int("port", 8080, "listen port")


		// PostFilesHandler processes files' POST request
		func PostFilesHandler(w http.ResponseWriter, r *http.Request) {

FileManager #112

FileManager #112

Conversation

gongweibao commented May 30, 2017

Choose a reason for hiding this comment

gongweibao May 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helinwang May 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helinwang May 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gongweibao May 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gongweibao Jun 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helinwang Jun 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gongweibao May 31, 2017 •

edited

Loading

helinwang May 31, 2017 •

edited

Loading

helinwang May 30, 2017 •

edited

Loading

gongweibao May 31, 2017 •

edited

Loading

gongweibao Jun 1, 2017 •

edited

Loading

helinwang Jun 1, 2017 •

edited

Loading