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

Topics validation can be done via Kafka Admin Client. #79

Merged
merged 6 commits into from
Dec 21, 2023

Conversation

onukristo
Copy link
Contributor

@onukristo onukristo commented Dec 20, 2023

Context

Changed

  • Topic validation can now be done via Kafka Admin, instead of Kafka Producer.
    The new logic is under a feature flag, until the logic gets more battle tested.

Checklist

@onukristo onukristo added the change:standard Not an emergency or impactful change label Dec 20, 2023
@onukristo onukristo requested a review from a team as a code owner December 20, 2023 07:26
.refreshAfterWrite(Duration.ofSeconds(30))
.build(this::fetchTopicDescription);

CaffeineCacheMetrics.monitor(meterRegistry, topicDescriptionsCache, "tkmsTopicDescriptions");
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the Javadoc we need to call .recordStats() in the Caffeine builder above in order to gather non-zero stats.


### Changed

- Topic validation can now be done via Kafka Admin, instead of Kafka Producer.
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the upside of doing these topic validations before publishing messages instead of just failing with failed publishing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When publishing a message on shard-partition fails, then that shard-partition stops.
I.e. we have a poison-pill situtation.
The reason for doing different validation on message registering is to lower the probability of that situation happening.

tkmsMetricsTemplate.registerNoAclOperationsFetched(shardPartition, topic);
} else if (!aclOperations.contains(AclOperation.ALL)
&& !aclOperations.contains(AclOperation.WRITE)
&& !aclOperations.contains(AclOperation.IDEMPOTENT_WRITE)
Copy link
Contributor

Choose a reason for hiding this comment

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

IDEMPOTENT_WRITE is not enough by itself, we need WRITE or ALL too. That is IDEMPOTENT_WRITE does not include WRITE implicitly. We have a catch all rule for all clusters to allow everyone to do I_W: https://github.com/transferwise/kafka-topic-configurations/blob/2392c8d3289af69254bcd5cecd0c8e8770b70428/main/production/acls/cluster/cluster-acls.yml#L25-L27

/**
* Uses AdminClient to validate topics.
*
* <p>AdminClient would allow us to also check if topics have suitable ACLs.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* <p>AdminClient would allow us to also check if topics have suitable ACLs.
* <p>AdminClient allows us to also check if topics have suitable ACLs.

@onukristo onukristo merged commit 23c474e into master Dec 21, 2023
18 checks passed
@onukristo onukristo deleted the topic_validation_via_admin branch December 21, 2023 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change:standard Not an emergency or impactful change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants