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.

Sign up for GitHub

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

Jump to bottom

Update the Kafka recovery procedure with KRaft notes #10728

Merged
merged 7 commits into from
Oct 21, 2024
Merged

Update the Kafka recovery procedure with KRaft notes #10728

merged 7 commits into from
Oct 21, 2024

Conversation

fvaleri
Copy link
Contributor

@fvaleri fvaleri commented Oct 17, 2024

Type of change

  • Documentation

Description

This patch should close #10722.

The challenging task here is to retrieve the clusterId from existing volumes.
I thought to spin up a temporary pod for that, ugly but works.
Let me know if you have a better idea.

Checklist

  • Update documentation

@fvaleri
Update the Kafka recovery procedure with KRaft notes
f652517 
This patch should close strimzi#10722.

Signed-off-by: Federico Valeri <fedevaleri@gmail.com>
@fvaleri fvaleri added this to the 0.44.0 milestone Oct 17, 2024
scholzj
scholzj reviewed Oct 17, 2024
View reviewed changes
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

I added a nit. But LGTM otherwise. Thanks for looking into it.

I wonder if it would be easier to have just a second procedure for KRaft only instead of mixing it. It would be easier to just delete one in few months when Zoo is removed.

documentation/modules/managing/proc-cluster-recovery-volume.adoc Outdated Show resolved Hide resolved
@fvaleri
Address feedback from Jakub
33fc240 
Signed-off-by: Federico Valeri <fedevaleri@gmail.com>
@fvaleri
Copy link
Contributor Author

fvaleri commented Oct 18, 2024

I wonder if it would be easier to have just a second procedure for KRaft only instead of mixing it. It would be easier to just delete one in few months when Zoo is removed.

At the moment Kraft commands are just limited to this point, but if you prefer to have a second procedure I can do that.

@scholzj
Copy link
Member

scholzj commented Oct 18, 2024

I wonder if it would be easier to have just a second procedure for KRaft only instead of mixing it. It would be easier to just delete one in few months when Zoo is removed.

At the moment Kraft commands are just limited to this point, but if you prefer to have a second procedure I can do that.

Let's leave it to @PaulRMellor I guess. The advantage of separate procedures would be that we just ned to drop one once ZooKeeper is removed. That is it.

scholzj
scholzj approved these changes Oct 18, 2024
View reviewed changes
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

Thanks,

PaulRMellor
PaulRMellor reviewed Oct 18, 2024
View reviewed changes
Copy link
Contributor

@PaulRMellor PaulRMellor left a comment

Choose a reason for hiding this comment

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

Thanks @fvaleri
I left a few comments, mainly about the level of detail we might want to include for someone following the steps.
I agree with Jakub that it might be easier to have two procedures

documentation/modules/managing/proc-cluster-recovery-volume.adoc Outdated Show resolved Hide resolved
documentation/modules/managing/proc-cluster-recovery-volume.adoc Outdated Show resolved Hide resolved
documentation/modules/managing/proc-cluster-recovery-volume.adoc Outdated Show resolved Hide resolved
documentation/modules/managing/proc-cluster-recovery-volume.adoc Outdated Show resolved Hide resolved
documentation/modules/managing/proc-cluster-recovery-volume.adoc Outdated Show resolved Hide resolved
@fvaleri
Split KRaft and ZK procedures
929072d 
Signed-off-by: Federico Valeri <fedevaleri@gmail.com>
@fvaleri
Copy link
Contributor Author

fvaleri commented Oct 18, 2024 •
edited
Loading

@PaulRMellor @scholzj better?

I also need to update the pod listing in a separate commit.

@fvaleri
Update pod listing
1fb9c50 
Signed-off-by: Federico Valeri <fedevaleri@gmail.com>
@fvaleri fvaleri requested a review from PaulRMellor October 18, 2024 13:31
@fvaleri
Use myproject
a6ff5ef 
Signed-off-by: Federico Valeri <fedevaleri@gmail.com>
@fvaleri
Use edit command
32d8d25 
Signed-off-by: Federico Valeri <fedevaleri@gmail.com>
PaulRMellor
PaulRMellor reviewed Oct 21, 2024
View reviewed changes
Copy link
Contributor

@PaulRMellor PaulRMellor left a comment

Choose a reason for hiding this comment

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

New procedure looks good.
I think we don't need the sub-steps now.

documentation/modules/managing/proc-cluster-recovery-volume-zk.adoc Outdated Show resolved Hide resolved
documentation/modules/managing/proc-cluster-recovery-volume.adoc Outdated Show resolved Hide resolved
documentation/modules/managing/proc-cluster-recovery-volume.adoc Outdated Show resolved Hide resolved
documentation/modules/managing/proc-cluster-recovery-volume.adoc Outdated Show resolved Hide resolved
@fvaleri
Address feedback from Paul
87160f3 
Signed-off-by: Federico Valeri <fedevaleri@gmail.com>
@fvaleri fvaleri requested a review from PaulRMellor October 21, 2024 12:13
@scholzj scholzj modified the milestones: 0.44.0, 0.45.0 Oct 21, 2024
PaulRMellor
PaulRMellor approved these changes Oct 21, 2024
View reviewed changes
Copy link
Contributor

@PaulRMellor PaulRMellor left a comment

Choose a reason for hiding this comment

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

Thanks Fede 👍

@scholzj scholzj modified the milestones: 0.45.0, 0.44.0 Oct 21, 2024
@scholzj scholzj merged commit f2fa81b into strimzi:main Oct 21, 2024
13 checks passed
@fvaleri fvaleri deleted the strimzi-operator-volume-rec branch October 22, 2024 07:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scholzj scholzj scholzj approved these changes

@PaulRMellor PaulRMellor PaulRMellor approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.44.0
Development

Successfully merging this pull request may close these issues.

[DOC]: Invalid cluster.id error when initializing a new KRaft cluster with existing volumes
3 participants
@fvaleri @scholzj @PaulRMellor