Add protocol produce v6 #869
Conversation
|
Yes, I noticed that. I made the change locally but we have to change the client to wait for the pending requests when disconnecting otherwise we could lose data with the producer. I am on it. |
…ar connection status
|
There is one more detail, the request queue has to wait for the current inflight requests. I am making the change. |
|
I spot a bug in the way the request queue process pending requests kafkajs/src/network/requestQueue/index.js Line 232 in c025e05 it is processing the most recent request in the queue rather than the oldest one, I am switching from this.pending.pop() to this.pending.shift() since we want FIFO behavior.
|
|
I ran many tests, and I am now happy with the solution. Please take a look. |
| @@ -317,8 +324,13 @@ module.exports = class Connection { | |||
|
|
|||
| try { | |||
| const payloadDecoded = await response.decode(payload) | |||
| // KIP-219: If the response indicates that the client-side needs to throttle, do that. | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
Somewhat nit-picky, but this line wouldn't be a JSDoc-y comment, but just a multiline comment, no?
There was a problem hiding this comment.
I like the doc block because it highlights the comment. I made the change to give the importance this comment deserve; I don't think we should argue about the comment block style; my intention was just to highlight a good comment 😄
| await sendSampleMessages() | ||
| }) | ||
|
|
||
| afterEach(async () => { | ||
| await producer.disconnect() |
There was a problem hiding this comment.
This should use the same protection against producer not being assigned, to avoid noisy errors when tests fail due to problems with the cluster (see #795)
|
LGTM! |
No description provided.