Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions lib/BackbeatConsumer.js
Original file line number Diff line number Diff line change
Expand Up @@ -438,6 +438,14 @@ class BackbeatConsumer extends EventEmitter {
this._tryConsumedTimeout = null;
}

// the shutdown drains what is already in flight, so fetching more
// only delays the departure and strands the extra work. This also
// ends the self-rescheduling loop, which would otherwise keep
// consuming against a closed client for the life of the process.
if (this._shuttingDown) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the assumption is that consume() should have no effect during drain (and shutdown, by extension), as we are already unsubscribed.

So this should not be needed, as far as we know.

  • c.f. previous PR: if we need to do it for shutdown, probably also needed during "regular" rebalance.
  • Should not be needed I think, but maybe there could be race conditions (if our timeout hits after kafka rebalance timeout), so it may be a good defensive measure to avoid any race condition
  • On the other hand, this call is only way to poll kafka, i.e. let it know the consumer is alive (even if not consuming). Not sure if this has any effect on rebalance, but we should make sure that skipping these calls does not actually degrade the situation by getting kafka to kick the consumer out of the group for aggressively...

return undefined;
}

// use non-flowing mode of consumption to add some flow
// control: explicit consumption of messages is required,
// needs explicit polling to get new messages
Expand Down
31 changes: 31 additions & 0 deletions tests/unit/backbeatConsumer.js
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,37 @@ describe('backbeatConsumer', () => {
});
});

describe('_tryConsume', () => {
let consumer;

beforeEach(() => {
consumer = new BackbeatConsumerMock({
kafka,
groupId: 'unittest-group',
topic: 'my-test-topic',
});
consumer._processingQueue = {
length: () => 0,
running: () => 0,
};
consumer._consumer = { consume: sinon.stub() };
});

it('should fetch while the consumer is running', () => {
consumer._tryConsume();

assert.strictEqual(consumer._consumer.consume.calledOnce, true);
});

it('should not fetch once the shutdown has started', () => {
consumer._shuttingDown = true;

consumer._tryConsume();

assert.strictEqual(consumer._consumer.consume.called, false);
});
});

describe('_getAvailableSlotsInPipeline', () => {
let consumer;

Expand Down
Loading