gjohnson / consul-node

A node.js client library for consul.
MIT License
67 stars 16 forks source link

Issues running example #1

Closed 19h closed 10 years ago

19h commented 10 years ago

Hi,

we're giving Consul a spin because it sounds fruitful but stumbled over an issue.

We're running a node in datacenter (1) and another in datacenter (2), where node (2) is joining dc1, the cluster of node (1). So far so good, both give positive signals.

When doing this:

var Consul = require('consul-node');

var consul = new Consul({
    host: "localhost",
    port: 8500,
});

consul.kv.put('hello', 'world', function(err, ok) {
    if (err) throw err;
    consul.kv.get('hello', function(err, items) {
        if (err) throw err;
        console.log(items);
    });
});

=>

Error: ruh oh
    at Requestor.<anonymous> (/private/tmp/node_modules/consul-node/lib/requestor.js:87:15)
    at Request.self.callback (/private/tmp/node_modules/consul-node/node_modules/request/request.js:121:22)
    at Request.EventEmitter.emit (events.js:98:17)
    at Request.<anonymous> (/private/tmp/node_modules/consul-node/node_modules/request/request.js:978:14)
    at Request.EventEmitter.emit (events.js:117:20)
    at IncomingMessage.<anonymous> (/private/tmp/node_modules/consul-node/node_modules/request/request.js:929:12)
    at IncomingMessage.EventEmitter.emit (events.js:117:20)
    at _stream_readable.js:920:16
    at process._tickCallback (node.js:415:13)

... is thrown

_Update_

When upgrading to latest master, this is thrown instead:

ReferenceError: callback is not defined
    at Requestor.<anonymous> (/private/tmp/node_modules/consul-node/lib/requestor.js:86:16)
    at Request.self.callback (/private/tmp/node_modules/consul-node/node_modules/request/request.js:121:22)
    at Request.EventEmitter.emit (events.js:98:17)
    at Request.<anonymous> (/private/tmp/node_modules/consul-node/node_modules/request/request.js:978:14)
    at Request.EventEmitter.emit (events.js:117:20)
    at IncomingMessage.<anonymous> (/private/tmp/node_modules/consul-node/node_modules/request/request.js:929:12)
    at IncomingMessage.EventEmitter.emit (events.js:117:20)
    at _stream_readable.js:920:16
    at process._tickCallback (node.js:415:13)

aha, this is pointing to return callback(new Error('not implemented'));.. fun that the error fails to error :facepunch:

19h commented 10 years ago

Strange, works now w/o intervention:

[ { createIndex: 72,
    modifyIndex: 72,
    key: 'hello',
    flags: 0,
    value: 'world' } ]
gjohnson commented 10 years ago

Ha, yeah sorry... I accidentally committed that throw to remind my self to figure out all the response code's it uses. I would point to master for the next week as some things flesh out.

gjohnson commented 10 years ago

@KenanSulayman fixed in 010a2784bc30829e82467adc690d0294f3e834ed

gjohnson commented 10 years ago

@KenanSulayman also, if you run your app via DEBUG=* node your-app.js you will get the response codes and other goodies. Please paste in here so I can fix or submit a PR with a fix :-)

19h commented 10 years ago

Thanks! :+1: for DEBUG=*, I usually never use env variables.

Btw, wouldn't it be faster to use plain tcp sockets? I'll look into it

gjohnson commented 10 years ago

@KenanSulayman conul's RPC API does not support all the features of the HTTP API. However, I think it would make sense to support both.

Maybe something like:

var consul = require('consul-node');

// http client
var api = consul.http({ host: 'localhost' });

// tcp client
var rpc = consul.rpc({ host: 'localhost' });

// dns utility
var dns = consul.dns({ host: 'localhost' });
19h commented 10 years ago

Nice. Actually I mean the communication to the HTTP interface, though.

I just rewrote the Requestor.prototype.get to connect over a TCP socket and send a plain buffer ("GET /path HTTP/1.1") and was able to reduce the first-byte-latency by almost 50% on my Mac. I guess the abstractions of request are quite expensive for this particular "atomic" use-case.

Let's see, everything is possible.

:walking:

gjohnson commented 10 years ago

Ah, I see... I think once everything is working, I would just drop down to the native http|https modules if needed. Despite the speed benefits, there is a lot that would have to be done to correctly support everything over raw TCP and I'd rather spend that time implementing and maintaining the endpoints.

19h commented 10 years ago

Drop this on your λmaster lib/requestor.js and check the differences: http://data.sly.mn/code/1n2r1T3k091e/requestor.patch

:walking: