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 first version of Kafka operator #3

Merged
merged 19 commits into from Feb 18, 2021
Merged

Add first version of Kafka operator #3

merged 19 commits into from Feb 18, 2021

Conversation

lfrancke
Copy link
Member

@lfrancke lfrancke commented Feb 4, 2021

No description provided.

@lfrancke lfrancke added this to the Milestone #1 milestone Feb 4, 2021
@lfrancke lfrancke self-assigned this Feb 4, 2021
@project-bot project-bot bot added this to In progress in Stackable Feb 4, 2021
This was linked to issues Feb 10, 2021
@lfrancke lfrancke requested a review from a team February 15, 2021 11:54
@lfrancke lfrancke marked this pull request as ready for review February 15, 2021 11:54
Copy link
Member

@maltesander maltesander left a comment

Choose a reason for hiding this comment

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

Very clean. I just commented the change from .de to .tech in some cases (hope i got them all).

crd/src/lib.rs Outdated Show resolved Hide resolved
crd/src/lib.rs Outdated Show resolved Hide resolved
crd/kafkaclusters.crd.yaml Outdated Show resolved Hide resolved
crd/kafkaclusters.crd.yaml Outdated Show resolved Hide resolved
examples/simple-kafkacluster.yaml Outdated Show resolved Hide resolved
operator/src/lib.rs Outdated Show resolved Hide resolved
operator/src/lib.rs Outdated Show resolved Hide resolved
server/src/main.rs Outdated Show resolved Hide resolved
Stackable automation moved this from In progress to Review in progress Feb 17, 2021
@lfrancke
Copy link
Member Author

I'd be happy with just your review @maltesander but I've added @soenkeliebau as you requested.

soenkeliebau
soenkeliebau previously approved these changes Feb 18, 2021
Copy link
Member

@soenkeliebau soenkeliebau left a comment

Choose a reason for hiding this comment

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

I've looked through the code and overall am happy for it to go in. Left just a few minor comments.

One thing though, I get
" Reconciliation finished with an error, this should not happen, please file an issue err=ObjectNotFound { obj_ref: ObjectRef { kind: ErasedResourceState..." when running this. I think I remember you having that with the ZK operator as well and mentioning that it is not critical - but can't remember what it was.

target/
**/*.rs.bk

.idea/
Copy link
Member

Choose a reason for hiding this comment

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

Super-Nit: this shouldn't be in the .gitignore that is checked in

Copy link
Member Author

Choose a reason for hiding this comment

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

Why not?

Copy link
Member

Choose a reason for hiding this comment

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

Because that is specific to your development environment and should be contained either in your global .gitignore file or in your local repositories .git/info/exclude file.
At least that's the way I was taught :)

But as I said, super-nit, feel free to leave as is.

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason I always include this is because I don't want anyone checking in their .idea even if they don't have it in their global .gitignore.

Copy link
Member

Choose a reason for hiding this comment

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

Following that argument we should also include .eclipse, ~ files for vim, whatever vscode does ..

I totally understand where you are coming from and please, absolutely do merge like this!! This is really more of a philosophical discussion :)

operator/src/lib.rs Show resolved Hide resolved
operator/src/lib.rs Outdated Show resolved Hide resolved
self.context.client.delete(pod).await?;
}
}
Ok(ReconcileFunctionAction::Continue)
Copy link
Member

Choose a reason for hiding this comment

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

If there were excess pods which we deleted, this was probably a cluster shrink. General question, should we requeue to enable performing checks whether the cluster has rebalanced after the shrink before taking any more actions?
Irrelevant in this case, as it is the last action anyway and there are no checks to perform..

Copy link
Member Author

Choose a reason for hiding this comment

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

Not irrelevant. It's a good question.
Should we delete a single pod and then requeue or should we delete all of them?

In Kafka we probably want to take steps to rebalance before removing too many?
This is where you are the expert.

But before we have a full solution you're right and it might make sense to requeue after every deletion.

operator/src/lib.rs Outdated Show resolved Hide resolved
@lfrancke
Copy link
Member Author

The reconciliation error you mentioned has been fixed in operator-rs today so that should hopefully not happen anymore

Copy link
Member

@soenkeliebau soenkeliebau left a comment

Choose a reason for hiding this comment

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

There are unused imports that clippy complains about after you removed the affinity elements from the pod.

@lfrancke lfrancke dismissed maltesander’s stale review February 18, 2021 13:30

Addressed all requests

Stackable automation moved this from Review in progress to To be merged Feb 18, 2021
@lfrancke lfrancke merged commit 97cc774 into main Feb 18, 2021
Stackable automation moved this from To be merged to Done Feb 18, 2021
@lfrancke lfrancke deleted the start branch February 18, 2021 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Basic implementation Implement process lifecycle
3 participants