add support for DescribeConfigs protocol #1081
Conversation
test/test.admin.js
Outdated
| const groupId = 'test-group-id'; | ||
|
|
||
| before(function (done) { | ||
| if (process.env.KAFKA_VERSION === '0.8' || process.env.KAFKA_VERSION === '0.9' || process.env.KAFKA_VERSION === '0.10') { |
There was a problem hiding this comment.
how about ['0.8', ...].includes(process.env.KAFKA_VERSION) ?
test/test.admin.js
Outdated
| } | ||
|
|
||
| createTopic(topic, 1, 1).then(function () { | ||
| consumer = new ConsumerGroup({ |
There was a problem hiding this comment.
why do we need the consumer group in this test ?
There was a problem hiding this comment.
We don't, good catch! I've updated the test. Also, do you want me to add a close function onto Admin for convenience? It'd just delegate to the kafkaClient. I can do that in another PR.
README.md
Outdated
|
|
||
| ```js | ||
| const resource = { | ||
| resourceType: '2', // '2' for topic, '4' for broker |
There was a problem hiding this comment.
should export a constant map instead, and do like resourceType: RESOURCE_TYPES.TOPIC
magic numbers as an api is bad
There was a problem hiding this comment.
Make sense and i agree, i just did not see any other examples within the library already that forced library consumers to import definitions of a request variable.
"Magic numbers" are already being used in the producer:
// Partitioner type (default = 0, random = 1, cyclic = 2, keyed = 3, custom = 4), default 0
partitionerType: 2
which I'm assuming are just the kafka defined values (which I have done the same in this PR).
As an alternative to a new constant map, we could allow users to pass in the string names of the resource type e.g. broker, topic. I have no preferences.
@aikar @hyperlink thoughts?
There was a problem hiding this comment.
just because bad practice was used else where, doesn't mean it should continue :)
strings are more readable, but also vulnerable to typos.
but it could be a
const typeMap = {
topic: 2,
broker: 4
}
export const TYPES = {
TOPIC: "topic",
BROKER: "broker",
};
const type = typeMap[userOption] || userOption;then the enum map can be provided, but string versions would still work (and even numbers for anyone relying on implementation details)
There was a problem hiding this comment.
Good suggestions @aikar. 👍
We can make it a string to number map and yield an error if it's not in the map.
There was a problem hiding this comment.
Yes I love this approach, thanks @aikar ! I'll push up the changes for this soon.
There was a problem hiding this comment.
Pushed up the changes for this. I added RESOURCE_TYPES directly onto Admin so user's don't have to separately import the resource types if that's the only spot that they're going to use it. Also updated the README as I had some pieces wrong, Let me know if there are any changes you'd like to see (any nitpicks as well).
I'll also probably refactor the validation logic in kafkaClient soon when I implement the AlterConfigs protocol.
|
Accidentally did this in #1142 :-) Any change this would get merged soon? |
|
@hyperlink any concerns? I've been using this functionality from my commit for a few months now. |
|
Thanks for your contribution @ahmedlafta ! |
This PR adds support for DescribeConfigs, which allows a user to get the config entries for either a broker or topic (aka resource).
@hyperlink let me know if you have any opinions on data format input/output.
https://kafka.apache.org/protocol#The_Messages_DescribeConfigs