couchbaselabs / cbfs

Distributed Blobstore using Couchbase Server
http://dustin.github.com/2012/09/27/cbfs.html
Apache License 2.0
188 stars 40 forks source link

Listing a directory with subdirectories fails #137

Closed floridoo closed 9 years ago

floridoo commented 9 years ago

Here couchbase.GetBulk() returns an error if one of the keys is not found (which happens if the key is a directory).

A temporary fix is to remove the error check following it, but that would ignore all errors.

The proper fix would be to ignore just the "not found" errors, but afaik that is not possible, because the type couchbase.multiError is unexported in go-couchbase (here).

mschoch commented 9 years ago

Can you maybe provide an example request/response corresponding to this behavior?

I've tried to reproduce the behavior, but it seems OK.

Here is a list request of a directory which contains both files and directories:

http://cbfs-ext.hq.couchbase.com/.cbfs/list/mschoch/

mschoch commented 9 years ago

I've reviewed the underlying code a bit. Generally keys not found are simply omitted from the bulk get response. However, upon my reading its possible there is a corner case when the last key requested is not found (as this uses GET, not GETQ). This might mean that in some cases you get an error and others you don't. I'm still looking into this.

floridoo commented 9 years ago

Listing files on /:

> curl -i http://localhost:8484/.cbfs/list/
HTTP/1.1 500 Internal Server Error
Date: Thu, 11 Dec 2014 15:31:47 GMT
Content-Length: 120
Content-Type: text/plain; charset=utf-8

Error generating file list: {1 errors, starting with MCResponse status=KEY_ENOENT, opcode=GET, opaque=0, msg: Not found}%

With depth set to include all subdirectories:

> curl -i http://localhost:8484/.cbfs/list/\?depth\=2
HTTP/1.1 200 OK
Content-Type: application/json; charset=utf-8
Date: Thu, 11 Dec 2014 15:33:28 GMT
Content-Length: 180

{"files":{"monitor/d3.v2.min.js":{},"monitor/files.html":{},"monitor/files.js":{},"monitor/index.html":{},"monitor/jquery.min.js":{},"monitor/monitor.js":{}},"dirs":{},"path":"/"}

Used the current versions of all dependencies (go get in empty GOPATH).

mschoch commented 9 years ago

Interesting, I've confirmed there is a bug in gomemcached's handling of bulk requests. But, it seems to yield different behavior than you observe. I get an empty result, even when some keys do exist.

I think theres an optimization to just use a single get when there is only one key. Maybe that path handles things differently. I'll keep digging.

mschoch commented 9 years ago

Yes, a more recent commit to go-couchbase changed the behavior of GetBulk...

mschoch commented 9 years ago

Working on a fix, trying to push it through the gerrit review machinery.

floridoo commented 9 years ago

Great, thanks a lot!

mschoch commented 9 years ago

I pushed a change to gomemcached. Can you get the latest gomemcached, rebuild and try again.

floridoo commented 9 years ago

I will try it as soon as I get to the office in the morning (European time).

floridoo commented 9 years ago

It works fine now. Thank you for the fast fix!