Skip to content
Open
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
1 change: 1 addition & 0 deletions extensions/ingestion/IngestionConfigValidator.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const joiSchema = joi.object({
processor: joi.object({
circuitBreaker: joi.object().optional(),
}).optional(),
producerParams: joi.object().unknown(true).default({}),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why should we accept unknown ? Here we don't really define them neither validate them, why ? We should define what is behind object ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

there are dozens of potentials parameters here : https://docs.confluent.io/platform/current/clients/librdkafka/html/md_CONFIGURATION.html

So we went with this to not have to enumerate all, we are only using one or two for now, but wanna be able to use more without having to do another backbeat pr

Honestly debatable though, an ai could write down instantly the ~50 different params available 🤔

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@francoisferrand Should we enumerate all params, or enumerate only the ones we use now, or keep it that way.

Suggested change
producerParams: joi.object().unknown(true).default({}),
producerParams: joi.object({
'queue.buffering.max.kbytes': joi.number().optional(),
'queue.buffering.max.messages': joi.number().optional(),
}).optional().default({}),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No enumerate all of them. We use only a few, and we can use only a few. So just them should be enough. The goal of Joi is to validate, here we just skip it. Also we can make sure for example that we don't insert critical one (I remember the host later that is skipped for example).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Rechecked with François, we said the purpose of the ticket is to not bother enumerating all kafka possible options. There is also an associated ticket that we talked about for this sprint which is about using env variables in some kind of "override debugging mode" that will make use of this.

What François was also saying is normally, here, on kafka client creation, if a provided option is wrong, it will be rejected, so we are using librdkafka as some kind of validator (gonna add a test for this though)

});

function configValidator(backbeatConfig, extConfig) {
Expand Down
10 changes: 7 additions & 3 deletions lib/BackbeatProducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ class BackbeatProducer extends EventEmitter {
maxRequestSize: joi.number().default(KAFKA_PRODUCER_MESSAGE_MAX_BYTES),
compressionType: joi.string().default(KAFKA_PRODUCER_DEFAULT_COMPRESSION_TYPE),
requiredAcks: joi.number().default(KAFKA_PRODUCER_DEFAULT_REQUIRED_ACKS),
producerParams: joi.object().unknown(true).default({}),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same here, we should define them ? Maybe also factorise them between both (three with kafka params?)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

discussion ongoing in the other topic

}
);
}
Expand All @@ -75,7 +76,8 @@ class BackbeatProducer extends EventEmitter {
}

get producerConfig() {
const producerParams = {
const config = {
...this._producerParams,
Comment thread
DarkIsDude marked this conversation as resolved.
'metadata.broker.list': this._kafkaHosts,
'message.max.bytes': this._maxRequestSize,
'dr_cb': true,
Expand All @@ -84,10 +86,10 @@ class BackbeatProducer extends EventEmitter {
};

if (process.env.RDKAFKA_DEBUG_LOGS) {
producerParams.debug = process.env.RDKAFKA_DEBUG_LOGS;
config.debug = process.env.RDKAFKA_DEBUG_LOGS;
}

return producerParams;
return config;
}

get topicConfig() {
Expand Down Expand Up @@ -125,13 +127,15 @@ class BackbeatProducer extends EventEmitter {
maxRequestSize,
compressionType,
requiredAcks,
producerParams,
} = joiResult;
this._kafkaHosts = kafka.hosts;
this._topic = topic && withTopicPrefix(topic);
this._pollIntervalMs = pollIntervalMs;
this._maxRequestSize = maxRequestSize;
this._compressionType = compressionType;
this._requiredAcks = requiredAcks;
this._producerParams = producerParams;
}

connect() {
Expand Down
1 change: 1 addition & 0 deletions lib/config.joi.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ const joiSchema = joi.object({
site: joi.string(),
compressionType: joi.string().default(KAFKA_PRODUCER_DEFAULT_COMPRESSION_TYPE),
requiredAcks: joi.number().default(KAFKA_PRODUCER_DEFAULT_REQUIRED_ACKS),
producerParams: joi.object().unknown(true).default({}),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

kafka.producerParams is added to the global config, but only IngestionPopulator._setupProducer forwards it when creating a BackbeatProducer. Other producer call sites (LogReader._setupProducer, LifecycleQueuePopulator._setupProducer, BackbeatConsumer, GarbageCollectorProducer, etc.) pass compressionType/requiredAcks/maxRequestSize from kafkaConfig but skip producerParams — so the global setting silently has no effect on them.

Either forward producerParams in the other _setupProducer methods too, or move this config under extensions.ingestion only to avoid the misleading global scope.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is intentional, the objective is to have config that will later be extensible

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

That's a good call from claude, should we define config per producer type or having a global one. What do you mean by extensible ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The idea is we have
kafka.producerParams
extensions.ingestion.producerParams

extensions override the first one when provided. But for now we only have extension for ingestion, and aren't gonna bother doing it for all backbeat extensions.

I think its fine as is, but yeah eventually we should update all other backbeat extension to use this global kafka params, else its a bit weird @francoisferrand

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Agree, can you do a follow up ?

},
transport: transportJoi,
s3: hostPortJoi.optional(),
Expand Down
4 changes: 4 additions & 0 deletions lib/queuePopulator/IngestionPopulator.js
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,10 @@ class IngestionPopulator {
maxRequestSize: this.kafkaConfig.maxRequestSize,
compressionType: this.kafkaConfig.compressionType,
requiredAcks: this.kafkaConfig.requiredAcks,
producerParams: {
...this.kafkaConfig.producerParams,
...this.ingestionConfig.producerParams, // Extension params override global params

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
...this.ingestionConfig.producerParams, // Extension params override global params
...this.ingestionConfig.producerParams,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I wanna keep this one, i know its more obvious for you but not for me 🧐

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤝 let's go then. You just describe the code but why not

},
topic,
pollIntervalMs: POLL_INTERVAL_MS,
});
Expand Down
33 changes: 33 additions & 0 deletions tests/unit/backbeatProducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,39 @@ describe('backbeatProducer', () => {
'custom-topic', [{ key: 'foo', message: 'bar' }], () => {});
});

describe('producerParams', () => {
it('should include extra producerParams in producerConfig', () => {
const producer = new BackbeatProducer({
kafka,
producerParams: {
'queue.buffering.max.kbytes': 1048576,
'queue.buffering.max.messages': 200000,
},
});
const config = producer.producerConfig;
assert.strictEqual(config['queue.buffering.max.kbytes'], 1048576);
assert.strictEqual(config['queue.buffering.max.messages'], 200000);
});

it('should not let producerParams override critical built-in params', () => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should it be rejected by joi directly by disallow some keys ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Let's rediscuss after we've finalized joi config, some ongoing discussion on it

const producer = new BackbeatProducer({
kafka,
producerParams: {
'metadata.broker.list': 'attacker:9092',
'dr_cb': false,
},
});
const config = producer.producerConfig;
assert.strictEqual(config['metadata.broker.list'], kafka.hosts);
assert.strictEqual(config['dr_cb'], true);
});

it('should default to empty producerParams when not provided', () => {
const producer = new BackbeatProducer({ kafka });
assert.deepStrictEqual(producer._producerParams, {});
});
});

afterEach(() => {
process.env.KAFKA_TOPIC_PREFIX = '';
});
Expand Down
91 changes: 91 additions & 0 deletions tests/unit/ingestion/IngestionPopulator.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const config = require('../../../lib/Config');
const IngestionPopulator =
require('../../../lib/queuePopulator/IngestionPopulator');
const IngestionReader = require('../../../lib/queuePopulator/IngestionReader');
const BackbeatProducer = require('../../../lib/BackbeatProducer');
const fakeLogger = require('../../utils/fakeLogger');

const zkConfig = config.zookeeper;
Expand Down Expand Up @@ -354,4 +355,94 @@ describe('Ingestion Populator', () => {
});
});
});

describe('_setupProducer producerParams merge', () => {
let capturedProducerParams;

beforeEach(() => {
sinon.stub(BackbeatProducer.prototype, 'setFromConfig').callsFake(function (cfg) {
capturedProducerParams = cfg.producerParams;
// Minimal instance state so producerConfig getter doesn't throw.
this._kafkaHosts = cfg.kafka.hosts;
this._topic = null;
this._pollIntervalMs = 2000;
this._maxRequestSize = 5000020;
this._compressionType = 'Zstd';
this._requiredAcks = -1;
this._producerParams = cfg.producerParams || {};
});
});

afterEach(() => {
sinon.restore();
capturedProducerParams = undefined;
});

it('should pass merged producerParams : extension overrides global', () => {
const globalParams = {
'queue.buffering.max.kbytes': 1048576,
'queue.buffering.max.ms': 100,
};
const extParams = {
'queue.buffering.max.messages': 200000,
'queue.buffering.max.ms': 500,
};

const populator = new IngestionPopulator(
null,
zkConfig,
{ ...kafkaConfig, producerParams: globalParams },
qpConfig,
mConfig,
rConfig,
{ ...ingestionConfig, producerParams: extParams },
s3Config
);

populator._setupProducer(() => {});

assert.strictEqual(capturedProducerParams['queue.buffering.max.kbytes'], 1048576);
assert.strictEqual(capturedProducerParams['queue.buffering.max.messages'], 200000);
assert.strictEqual(capturedProducerParams['queue.buffering.max.ms'], 500,
'extension producerParams should override global kafka.producerParams');
});

it('should work when only global kafka.producerParams are set', () => {
const globalParams = { 'queue.buffering.max.kbytes': 524288 };

const populator = new IngestionPopulator(
null,
zkConfig,
{ ...kafkaConfig, producerParams: globalParams },
qpConfig,
mConfig,
rConfig,
ingestionConfig,
s3Config
);

populator._setupProducer(() => {});

assert.strictEqual(capturedProducerParams['queue.buffering.max.kbytes'], 524288);
});

it('should work when only extension producerParams are set', () => {
const extParams = { 'queue.buffering.max.messages': 100000 };

const populator = new IngestionPopulator(
null,
zkConfig,
kafkaConfig,
qpConfig,
mConfig,
rConfig,
{ ...ingestionConfig, producerParams: extParams },
s3Config
);

populator._setupProducer(() => {});

assert.strictEqual(capturedProducerParams['queue.buffering.max.messages'], 100000);
});
});
});
Loading