Skip to content

Conversation

patrykwegrzyn
Copy link
Contributor

Redpanda compatibility

@mcollina
Copy link
Member

mcollina commented Aug 7, 2025

There seems to be a CI problem?

@patrykwegrzyn
Copy link
Contributor Author

Yeah its a weird one, i'll have a look i tested locally against 3.7

@patrykwegrzyn
Copy link
Contributor Author

I can see some issues with base tests when force v8.
v8 don't have topic id , any ways to get negotiated api versions from the client ? We could make this version specific.
Open to suggestions

@mcollina mcollina requested a review from ShogunPanda August 7, 2025 17:13
@mcollina
Copy link
Member

mcollina commented Aug 7, 2025

Actually it seems it was just a flake.

@ShogunPanda can you take a look?

Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

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

@patrykwegrzyn Thanks for your contribution!
The code looks good to me, but I just have a little issue: so far I tried not to leave any "hole" in the supported API versions. Could you please implement Metadata v9, v10, v11 as well? I guess it will be mostly copying files over and make little modifications.
Thanks!

topics: reader.readArray(
(r, i) => {
const ec = r.readInt16()
if (ec !== 0) errors.push([`/topics/${i}`, ec])
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: can you please add { and } here?

@patrykwegrzyn
Copy link
Contributor Author

I can see some issues with base tests when force v8. v8 don't have topic id , any ways to get negotiated api versions from the client ? We could make this version specific. Open to suggestions

Any thoughts on this? this test will break on lower versions , can we move it to version specific test with something like
metadataV8.async(connection, null, false, true) ?

@ShogunPanda
Copy link
Contributor

We already negotiate the version from the broker, see https://github.com/platformatic/kafka/blob/main/src/clients/base/base.ts#L362 and https://github.com/platformatic/kafka/blob/main/src/clients/base/base.ts#L552
If it poses an issue when testing Redpanda, we can just skip it.

@patrykwegrzyn
Copy link
Contributor Author

patrykwegrzyn commented Aug 9, 2025

We already negotiate the version from the broker, see https://github.com/platformatic/kafka/blob/main/src/clients/base/base.ts#L362 and https://github.com/platformatic/kafka/blob/main/src/clients/base/base.ts#L552 If it poses an issue when testing Redpanda, we can just skip it.

Cheers for this!!
Exactly what I needed.
My usual flow is Kafka first, then Redpanda, forcing the client up to max API version im working on (v8 here). Totally missed the base-case issue without forcing it, cause the client was happily locking onto v12.
Hope this make sense.

Will you be ok with

  const getApi = promisify(client[kGetApi].bind(client))
  const api = await getApi('Metadata')
  const testTopic = `test-topic-${randomUUID()}`

  // Fetch metadata with autocreate
  const metadata = await client.metadata({
    topics: [testTopic],
    autocreateTopics: true
  })
  const topicMetadata = metadata.topics.get(testTopic)!
  if (api.version >= 10) {
    strictEqual(typeof topicMetadata.id, 'string')
  }

and make strictEqual(typeof topicMetadata.id, 'string') check only on v10+ versions ?

@ShogunPanda
Copy link
Contributor

Yup, go ahead!

@patrykwegrzyn
Copy link
Contributor Author

sweet ill submit Metadata v9, v10, v11 after holiday

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants