Skip to content
This repository was archived by the owner on Jan 24, 2024. It is now read-only.

Conversation

@eolivelli
Copy link
Contributor

OffsetConfig was still using "public" as tenant

Changes:

  • change "public/default/__consumer_offsets" to "${tenant}/default/__consumer_offsets"
  • pass the 'tenant' when we used that configuration
  • update a test case

@CLAassistant
Copy link

CLAassistant commented Oct 25, 2021

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@Demogorgon314 Demogorgon314 left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Just left few comment. PTAL.

@@ -1154,6 +1158,7 @@ public void removeGroupsForPartition(int offsetsPartition,
TopicPartition topicPartition = new TopicPartition(
GROUP_METADATA_TOPIC_NAME, offsetsPartition
Copy link
Member

Choose a reason for hiding this comment

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

I guess this topic name need change too, it will make log more accurate.

@Default
private String offsetsTopicName = DefaultOffsetsTopicName;

public String getCurrentOffsetsTopicName(String tenant) {
Copy link
Member

@Demogorgon314 Demogorgon314 Oct 26, 2021

Choose a reason for hiding this comment

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

IMO, We can replace placeholder when OffsetConfig object creating, Instead of replacing placeholder every time we get it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it makes sense to me.
But I have to perform some refactors.
I will update the patch in the next days

@eolivelli
Copy link
Contributor Author

After rebasing to current master this patch is useless.
I am closing this PR

@eolivelli eolivelli closed this Dec 1, 2021
@eolivelli eolivelli deleted the fix/multi-tenant-group-management branch December 1, 2021 07:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants