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

Add topic properties to Migrator #3207

Merged
merged 4 commits into from
Feb 27, 2025
Merged

Conversation

mihaitodor
Copy link
Collaborator

Also make sure ACLs are always migrated even if the destination topic already exists.

c.Key == "replication.factor" ||
c.Key == "write.caching" ||
c.Key == "redpanda.iceberg.mode" ||
strings.HasPrefix(c.Key, "redpanda.remote.") {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is this correct? Do we have to first query the destination server settings to ensure that we're not overwriting a custom setting?

Comment on lines -779 to -780
replication_factor_override: true
replication_factor: -1
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The template linter is a bit annoying, since it doesn't check if there are any unrecognised fields... Something to improve in Benthos.


continue
} else {
mgr.Logger().Infof("Created topic %q", topic)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's a debug message which gets printed if the topic already exists...

@mihaitodor mihaitodor force-pushed the mihaitodor-migrator-topic-properties branch 2 times, most recently from b28c503 to e3efdbd Compare February 25, 2025 21:32
Copy link
Collaborator

@rockwotj rockwotj left a comment

Choose a reason for hiding this comment

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

LGTM, the only thing is that I think we don't want to mirror redpanda.remote.* properties

c.Key == "replication.factor" ||
c.Key == "write.caching" ||
c.Key == "redpanda.iceberg.mode" ||
strings.HasPrefix(c.Key, "redpanda.remote.") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure we want to mirror redpanda.remote.* but just want to take defaults in the destination cluster. The thing is in Redpanda Cloud we never want this to be off, and the default is always on. Maybe we should just not mirror it...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good, I removed redpanda.remote. from that list.

Also make sure ACLs are always migrated even if the destination
topic already exists.

Signed-off-by: Mihai Todor <[email protected]>
@mihaitodor mihaitodor force-pushed the mihaitodor-migrator-topic-properties branch from e3efdbd to 840ed26 Compare February 26, 2025 17:05
c.Key == "max.message.bytes" ||
c.Key == "replication.factor" ||
c.Key == "write.caching" ||
c.Key == "redpanda.iceberg.mode" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe simpler as a switch or a slices.Contain?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I went for a map since that's what Copilot suggested when I asked it to make the code look nicer 😅

@mihaitodor mihaitodor requested a review from rockwotj February 26, 2025 21:07
Copy link
Collaborator

@rockwotj rockwotj left a comment

Choose a reason for hiding this comment

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

lgtm

@mihaitodor mihaitodor merged commit e0a528d into main Feb 27, 2025
4 checks passed
@mihaitodor mihaitodor deleted the mihaitodor-migrator-topic-properties branch February 27, 2025 13:15
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.

4 participants