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

Automatically refreshing the KafkaRebalance proposal should be properly plugged into the async-flow #10416

Closed
scholzj opened this issue Aug 4, 2024 · 2 comments · Fixed by #10486

Comments

@scholzj
Copy link
Member

scholzj commented Aug 4, 2024

When the KafkaRebalance spec is updated, the KafkaRebalanceAssemblyOperator automatically adds a refresh annotation to it:

if (kafkaRebalance.getStatus() != null
&& kafkaRebalance.getStatus().getObservedGeneration() != kafkaRebalance.getMetadata().getGeneration()) {
KafkaRebalanceBuilder patchedKafkaRebalance = new KafkaRebalanceBuilder(kafkaRebalance);
patchedKafkaRebalance
.editMetadata()
.addToAnnotations(Map.of(ANNO_STRIMZI_IO_REBALANCE, KafkaRebalanceAnnotation.refresh.toString()))
.endMetadata()
.editStatus()
.withObservedGeneration(kafkaRebalance.getMetadata().getGeneration())
.endStatus();
kafkaRebalanceOperator.patchAsync(reconciliation, patchedKafkaRebalance.build()).onComplete(
r -> LOGGER.debugCr(reconciliation, "The KafkaRebalance resource is updated with refresh annotation"));
}

However, this code seems to be a bit lazily implemented and should be improved:

  • As it is done asynchronously, it should be plugged into the asynchronous flow of the Vert.x futures
  • It should be done only after the checks for the Kafka cluster, to make sure that:
    • The refresh annotation actually makes sense because the the Kafka cluster exists and is ready
    • Make sure the operator instance annotating it owns the rebalance resource

This should help to avoid race conditions.

@ppatierno
Copy link
Member

Not able to join today's call but I think that this should be done.

@scholzj
Copy link
Member Author

scholzj commented Aug 8, 2024

Triaged on the Community call on 8.8.2024: This should be fixed.

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

Successfully merging a pull request may close this issue.

3 participants