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

Fabric8 leader election (CAN ONLY GO IN THE NEXT MAJOR RELEASE) #1658

Draft
wants to merge 232 commits into
base: main
Choose a base branch
from

Conversation

wind57
Copy link
Contributor

@wind57 wind57 commented May 28, 2024

No description provided.

wind57 and others added 30 commits December 4, 2021 07:59
@wind57
Copy link
Contributor Author

wind57 commented Sep 18, 2024

I am a bit confused by the PR title. If this is disabled by default I think it can go in main and then make the old implementation as deprecated. Then we enable it by default in the major then remove the old implementation.

It can go in main or 3.1.x? Sorry, I'm confused a bit

Also do you plan on an equivalent implementation for the K8S Java client?

100%. I started to look into that implementation also, but as with fabric-8 I want to be sure I understand all the details, first, so it will take a bit.

Would there need to be specific RBAC configuration to allow the app to make this API request?

not sure, I'll dig more into it.

Also, even if it is supported would it make sense to have a configuration property to force using config maps?

valid point, it might be actually. I'll work on it.

@ryanjbaxter
Copy link
Contributor

It should go into main IMO

@wind57
Copy link
Contributor Author

wind57 commented Sep 21, 2024

if you say it can go into main and I treat main as the next major release, it means this one should still wait for when that major is coming. We can not merge it now. This is regarding your comment:

I am a bit confused by the PR title. If this is disabled by default I think it can go in main and then make the old implementation as deprecated. Then we enable it by default in the major then remove the old implementation.

@ryanjbaxter
Copy link
Contributor

I thought everything was wrapped in a feature flag and it wasn't introducing any breaking changes. If that is not the case then it will need to wait and cannot go into main right now.

@wind57
Copy link
Contributor Author

wind57 commented Sep 23, 2024

it is protected by a feature flag, I got your point now.

@wind57
Copy link
Contributor Author

wind57 commented Sep 26, 2024

Would there need to be specific RBAC configuration to allow the app to make this API request?

good point! Added it in the documentation

Also, even if it is supported would it make sense to have a configuration property to force using config maps?

good point again, added such an option

@wind57
Copy link
Contributor Author

wind57 commented Sep 26, 2024

This seems like a useful feature. I wonder if it makes sense for it to live in the commons package?

I can't really do that because of the very specific fabric8 APIs... but may be I will revisit this idea once I get a better understanding of the native client implementation.

I've addressed all of the comments here I think, you can take another look now.

Users have the option to switch back to the old implementation at any point in time, so the more the new one is used, the more we will polish it. As usual, I'll be there for that work...

@ryanjbaxter
Copy link
Contributor

LGTM once you are satisfied with it we can merge it into main.

@wind57
Copy link
Contributor Author

wind57 commented Sep 28, 2024

I'm not there yet, I want to work on something else related indirectly with this PR for some time, and then get back to it. I would like to try and refactor some integration tests. The idea is that some integration tests do make sense, but some really need to be moved to plain tests. For example, here in this leader implementation some ITs are really needed, but because we already consume so much time with them, we can't add more. So I want to reduce their number and then add one more here. I'm not sure where that will take me, but I have to try this path first

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

Successfully merging this pull request may close these issues.

Spring Cloud Kubernetes - Use Informers instead of Watchers
4 participants