Skip to content
This repository has been archived by the owner on Sep 25, 2020. It is now read-only.

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

Open
lopter opened this issue Nov 17, 2015 · 1 comment
Open

Comments

@lopter
Copy link

lopter commented Nov 17, 2015

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
Copy link
Contributor

@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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants