Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow describe/alter config for broker configs #649

Closed
jlandersen opened this issue Mar 3, 2020 · 1 comment · Fixed by #898
Closed

Allow describe/alter config for broker configs #649

jlandersen opened this issue Mar 3, 2020 · 1 comment · Fixed by #898

Comments

@jlandersen
Copy link

Currently the describeConfigsand alterConfigs methods on admin client fails for broker configs when the cluster has more than one broker. Config requests for topic resources can be served by any broker, but broker config requests must be served by the specific broker.

Example test that will break (unless you are lucky and the request actually goes to the broker being requested):

test('describe broker config', async () => {
  const cluster = createCluster()
  admin = createAdmin({ cluster: cluster, logger: newLogger() })
  await admin.connect()
  const metadata = await cluster.brokerPool.seedBroker.metadata()
  const brokers = metadata.brokers
  const brokerToDescribeConfig = brokers[1].nodeId.toString()

  await admin.describeConfigs({
    resources: [
      {
        type: RESOURCE_TYPES.CLUSTER,
        name: brokerToDescribeConfig,
      },
    ],
  })
})

Possible solutions would be to either rework the current resource type object to match that of ConfigResource, or create a new ConfigResourceType module and use that for all config related methods. This is potentially a breaking change in both cases, especially for Typescript users, as a different type is introduced. Alternatively, keep RESOURCE_TYPE and accept the deviation from the Java client.

I am happy to make the necessary changes for this to work, especially the first part to get the feature working, but input on handling the second point would be good. My preference is to introduce the new ConfigResourceType module, and keep the existing resource type for any future functionality where it is needed.

[0] https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java#L1891

[1] https://github.com/tulios/kafkajs/blob/master/src/protocol/resourceTypes.js

@Nevon
Copy link
Collaborator

Nevon commented Mar 10, 2020

I suggest splitting this into two efforts. The first one seems like a straight up bug that I would be happy to review a PR for.

The second one I'm not sure what the best solution for is. Given that it's a bug, perhaps a technically breaking change is okay.

My priority would be to reduce the impact on existing Typescript users. My suggestion would be to:

  1. Introduce a new CONFIG_RESOURCE_TYPES
  2. Alias the old RESOURCE_TYPES to CONFIG_RESOURCE_TYPES and make describeConfigs etc. take a union of both.
  3. Remove all references to RESOURCE_TYPES from the documentation and use the new CONFIG_RESOURCE_TYPES instead.

When the time comes that we need the equivalent of RESOURCE_TYPES from the Java client, we can introduce a new ACL_RESOURCE_TYPES or something and accept the deviation. Eventually we can remove RESOURCE_TYPES in a breaking change.

Nevon added a commit that referenced this issue May 2, 2022
Replaced by AclResourceTypes and ConfigResourceTypes. See #649
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 a pull request may close this issue.

2 participants