New features and enhancements #92
Conversation
- implementatio of channel pool - implementation of universal message format - Add benchmark
ykhatal
force-pushed
the
feature/enhancements
branch
from
fb05974 to
e15f0f5
Compare
|
|
||
| # To do | ||
| TO-DO.md | ||
| >>>>>>> New features and enehancement |
There was a problem hiding this comment.
There was an unsuccessful merge
| const suite = new Benchmark.Suite; | ||
|
|
||
| process.on('unhandledRejection', error => { | ||
| // Will print "unhandledRejection err is not dewfined" |
| timeout: 1000, | ||
| // default timeout for RPC calls. If set to '0' there will be none. | ||
| rpcTimeout: 15000, | ||
| consumerSuffix: '', |
There was a problem hiding this comment.
shouldn't it be taken from LOCAL_QUEUE?
| return this.connection.createChannel() | ||
| .then((channel) => { | ||
| ++this.channelCount; | ||
| this.pool.push(channel); |
There was a problem hiding this comment.
you can do
this.channelCount = this.pool.push(channel)
Take a look at a return value description
There was a problem hiding this comment.
Maybe also check for poolSize and current amount of channel?
|
|
||
| getChannel() { | ||
| return mutex.synchronize(() => { | ||
| if (this.pool.length) return Promise.resolve(this.pool.shift()); |
There was a problem hiding this comment.
Out of optimization perspective, can you use pop() instead? The difference is that shift will force the array re-indexing, which is a costly operation if you take into consideration the possible amount of channels and frequency of the function calls, while pop() simply cuts one element from the end.
| static serialize(message, enableCompression) { | ||
| console.log('message:', message); | ||
| return new Promise((resolve, reject) => { | ||
| const msg = Buffer.from(JSON.stringify(message), 'utf-8'); |
There was a problem hiding this comment.
If you want to use encoding, you need to remove the dash, because utf-8 does not exist, but just for your info, utf8 is a default encoding for buffer, so no need to do it explicitly
| function serializeMsg(msg, options) { | ||
| const falsie = [undefined, null]; | ||
|
|
||
| if (typeof msg === 'string') return Buffer.from(msg, 'utf-8'); |
There was a problem hiding this comment.
same as above (about utf8)
| msg = JSON.stringify(msg); | ||
| options.contentType = 'application/json'; | ||
|
|
||
| return Buffer.from(msg, 'utf-8'); |
| // Par défaut, Object.assign copie également | ||
| // les symboles énumérables | ||
| Object.getOwnPropertySymbols(source).forEach(sym => { | ||
| let descriptor = Object.getOwnPropertyDescriptor(source, sym); |
There was a problem hiding this comment.
It's good, but it's an es8 feature while we use LTS node version, which supports ES6 + some ES7. It runs locally for you without issues because you have a modern version of node, but can you check it with the LTS?
| correlationId: options.correlationId, | ||
| replyTo: options.replyTo, | ||
| // requeue: options.requeue, | ||
| // requeueCount: options.requeueCount |
There was a problem hiding this comment.
if not used, then please remove these
|
such an awesome pull... why it's not merged yet? best |
|
@RafaelAmorim thanks for your message.
So feel free to fork it and provide some work there, we always appreciate contributions. |
Early opened PR, below you can find a list of what was done or still to be: