MANTA-3071 mls reports no results, no error

[trentm]

This PR was migrated-from-gerrit, https://cr.joyent.us/#/c/1207/. The raw archive of this CR is here. See MANTA-4594 for info on Joyent Eng's migration from Gerrit.

CR discussion

@trentm commented at 2017-01-04T23:20:49

Patch Set 1:

New commits:
commit 2ba0b479844b0720c36052f281a663f772b3d2de
use the lserror if that is all we have

@trentm commented at 2017-01-04T23:21:03

Patch Set 1:

The test suite passes:

OK: 273 assertions (57393ms)

@davepacheco commented at 2017-01-04T23:51:22

Patch Set 1:

(1 comment)

bin/mls#222

Does this potentially fetch an entire object? What happens to that?

@davepacheco commented at 2017-01-04T23:52:50

Patch Set 1:

Also, what's the output now when the HEAD fails but the GET doesn't?

@trentm commented at 2017-01-05T00:11:14

Patch Set 1:

Dave,

It looks like this:

[16:09:46 trentm@danger0:~/joy/node-manta (grr-MANTA-3071)]
$ gd
diff --git a/bin/mls b/bin/mls
index 7e78ed8..21c7434 100755
--- a/bin/mls
+++ b/bin/mls
@@ -205,6 +205,10 @@ function printEntry(opts, obj) {

     options.paths.forEach(function (p) {
         client.ls(p, options, function (lsErr, res) {
+            // XXX
+            var restifyErrors = require('restify-clients/node_modules/restify-errors');
+            lsErr = new restifyErrors.ForbiddenError();
+
             if (lsErr) {
                 if (lsErr.name === 'InvalidDirectoryError') {
                     printEntry(options, lsErr.info);
[16:09:48 trentm@danger0:~/joy/node-manta (grr-MANTA-3071)]
$ mls
mls: ForbiddenError
[16:09:53 trentm@danger0:~/joy/node-manta (grr-MANTA-3071)]
$ echo $?
1

@trentm commented at 2017-01-05T00:15:45

Patch Set 1:

(1 comment)

bin/mls#222

That's somewhat true. A successful client.get results in returning a stream... and I think client.get will start processing the stream right away in the nextTick. So perhaps it is better to explicitly end that.

@davepacheco commented at 2017-01-05T00:50:05

Patch Set 1:

Other notes from chat:

• I wonder if we should skip the get altogether and provide hardcoded strings based on the response code. Or retry only errors < 500, which are less likely to be transient (but still could be).
• As you mention, if we keep doing the get, we probably ought to abort the stream if possible. But I'm not sure there's a way to do that.
• It would be nice to fix up mrm and mrmdir, which were also affected by the change that introduced this.

Thanks for digging into all this.

@trentm commented at 2017-01-05T16:39:40

Patch Set 1:

(1 comment)

I want to look at an option to client.get for this use case: "I just want an error, if any. If it isn't an error, then just close the stream and return.

bin/mls#222

Currently mls will exit soon after with process.exit(1), which will mean we won't hang on a streaming file with the current code. However, we might eventually want to move to not calling process.exit directly because of stdout/stderr flushing behaviour in node v4+... so we should consider that.

@trentm commented at 2017-01-05T17:51:18

Patch Set 2:

New commits:
commit 24eef3d6afcac0da9a6293d138efebf3cf43e6d8
add returnErrorOnly option to client.get and use that

@trentm commented at 2017-02-08T00:20:17

Patch Set 3:

New commits:
commit 497c8ba2aca9f55e02a222bbfdc0aee22888bb4d
add returnErrorOnly option to client.get and use that

commit 370534fff3f39a16431c733545d8257e0c834fca  
use the lserror if that is all we have

@trentm commented at 2017-02-11T00:45:14

Patch Set 4:

New commits:
commit b76d86d278bde597f3d4ad485ee24de7ff4b5dd0
add returnErrorOnly option to client.get and use that

commit 3ae4738eb3ea7591a5ff7a4a6a2dbb7deb6052c8  
use the lserror if that is all we have

@davepacheco commented at 2017-02-13T20:04:25

Patch Set 4:

(1 comment)

bin/mls#214

The "returnErrorOnly" flag feels like a band-aid. Why would a client ever really do that? That's what a HEAD is supposed to be. I get why it isn't, but I'm not sure it makes sense to expose this complexity (and put extra load on the server) in hopes that we get a better message, particularly given that we may still end up showing the same error we would have anyway, or even a different error than we really saw with the HEAD. I'd rather we just reported what we know: "the server reported status code X [and we don't have more information than that]".

That's just my take. Maybe others feel differently?