search

uber-node / tcurl

A command line utility to talk to tchannel servers
MIT License
36 stars 7 forks source link

The `-t` option shouldn't crash tcurl when the service can't be found #96

Open lopter opened 9 years ago

lopter commented 9 years ago

If you pass a directory to the -t option but your service name doesn't match the name of your thrift IDL file then tcurl crashes:

/usr/lib/node_modules/tcurl/index.js:292
        delegate.error('Spec for service "' +
                 ^
TypeError: Cannot call method 'error' of undefined
    at TCurl.readThriftDir (/usr/lib/node_modules/tcurl/index.js:292:18)
    at TCurl.readThrift (/usr/lib/node_modules/tcurl/index.js:275:17)
    at TCurl.asThrift (/usr/lib/node_modules/tcurl/index.js:385:23)
    at TCurl.send (/usr/lib/node_modules/tcurl/index.js:353:14)
    at onReady (/usr/lib/node_modules/tcurl/index.js:375:14)
    at onIdentified (/usr/lib/node_modules/tcurl/index.js:334:9)
    at finish (/usr/lib/node_modules/tcurl/node_modules/tchannel/peer.js:463:9)
    at Array.onIdentified [as 1] (/usr/lib/node_modules/tcurl/node_modules/tchannel/peer.js:454:9)
    at DefinedEvent.emit (/usr/lib/node_modules/tcurl/node_modules/tchannel/lib/event_emitter.js:90:25)
    at TChannelConnection.onOutIdentified (/usr/lib/node_modules/tcurl/node_modules/tchannel/connection.js:460:26)

Moreover, it feels like tcurl is relying on a convention here (your thrift definition has to be in a specific place and format that matches your service name) but I haven't seen this convention documented anywhere. Maybe the -t option should be implemented differently? or maybe it should extract the service name from the endpoint argument (since, as far as I understand, its format is ServiceName::endPoint).

kriskowal commented 9 years ago

@lopter Thanks. We have gone through multiple phases with -t, and have to revisit it now that we have the idl tool and our conventions have evolved. In the near term, the -t argument should become unnecessary since thrift methods e.g., tcurl service Service::method will automatically hit the Meta::thriftIDL endpoint.

-t does accept the full path to a specific file. We will keep this ticket open to track providing a better error message when you don’t provide a path to a specific file.

We may also evolve the convention, such that -t can point to an idl directory and infer the rest of the path using the git origin remote and the service name…and document that convention.

cc @malandrew