Add fail after configuration#155
Add fail after configuration#155ubnt-michals wants to merge 2 commits intorapid7:masterfrom ubnt-michals:fail_after_configuration
Conversation
|
Any chance to get this merged? |
stopal-r7
left a comment
stopal-r7
left a comment
There was a problem hiding this comment.
There are a few questions I have raised. I like the intention, we should definitely have this feature.
| "babel": "6.5.2", | ||
| "babel-plugin-add-module-exports": "0.1.2", | ||
| "babel-plugin-syntax-decorators": "6.5.0", | ||
| "babel-plugin-transform-builtin-extend": "1.1.2", |
There was a problem hiding this comment.
why is this change for along with the new entry on gobblefile.js?
There was a problem hiding this comment.
No, it is related to: src/error.js
here: ReconnectFailedError extends LogentriesError
There was a problem hiding this comment.
Is there no better way doing this? without introducing a new dependency?
There was a problem hiding this comment.
We have babel-preset-es2015 which includes babel-plugin-transform-es2015-classes.
This last one supports partially and it recommends to use babel-plugin-transform-builtin-extend which also have limitations.
I am not sure.
| this.drained = false; | ||
|
|
||
| if (this.reconnectFailed) { | ||
| this.ringBuffer.read(); |
There was a problem hiding this comment.
we are losing an event here, aren't we?
| return this.records.shift(); | ||
| } | ||
|
|
||
| empty() { |
There was a problem hiding this comment.
I can't see the place we use this function.
| _write(ch, enc, cb) { | ||
| this.drained = false; | ||
|
|
||
| if (this.reconnectFailed) { |
There was a problem hiding this comment.
What are we trying to accomplish with this check?
| this.emit(errorEvent, err); | ||
| this.debugLogger.log(`Error: ${err}`); | ||
| if (err instanceof ReconnectFailedError) { | ||
| this.ringBuffer.read(); // this connection failed, shift the buffer |
There was a problem hiding this comment.
we are losing an event here. read() will return the ext event in the buffer by shifting it.
danilosterrapid7
left a comment
danilosterrapid7
left a comment
There was a problem hiding this comment.
There are a loose method and some shifts in the buffer without a clear reason.
| "babel": "6.5.2", | ||
| "babel-plugin-add-module-exports": "0.1.2", | ||
| "babel-plugin-syntax-decorators": "6.5.0", | ||
| "babel-plugin-transform-builtin-extend": "1.1.2", |
There was a problem hiding this comment.
No, it is related to: src/error.js
here: ReconnectFailedError extends LogentriesError
3 participants
In case of network outage or DNS failure... with current setup logentries will try to reconnect indefinitely. In our case that's not a viable approach. We would like it to fail after a few attempts. I hope this PR correctly enables that functionality.