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 KafkaPartition and KafkaPartitionKey annotations #387

Merged

Conversation

bugflux
Copy link

@bugflux bugflux commented Jun 19, 2021

Hi all,

Although I haven't faced this need in micronaut projects yet, I have found it potentially useful to control partitioning separately from record keys.

This is useful when Keys have specific functional requirement (deduplication, log compaction, etc), but where partitioning of a topic needs to be made on a separate field (often having a one to many relationship with record keys, although that's not a requirement).

This PR brings in the ability to do just that with two new additional annotations: @KafkaPartition and @KafkaPartitionKey.

@CLAassistant
Copy link

CLAassistant commented Jun 19, 2021

CLA assistant check
All committers have signed the CLA.

if (partitionKey != null) {
byte[] partitionKeyBytes = Optional.ofNullable(serdeRegistry.pickSerializer(argument))
.orElseGet(ByteArraySerializer::new)
.serialize(finalTopic, parameterValues[i]);
Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure how expensive it might be to pick a serializer from the registry on every single call.

If it's deemed expensive then we can/should keep it in a map, but I'm not entirely sure what it should be keyed by. I suppose for the current implementation it would make the most sense to key by Class, but in the end that's more or less what I'd expect the registry implementations to do.

* <p>When used in consumers, it is populated with the partition that the record was received from.</p>
*
* <p>Note that while using {@link KafkaPartition} in the same method as {@link KafkaPartitionKey}
* will not throw an exception, the outcome of doing so is left unspecified.</p>
Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure if this is good behaviour. I'm happy to implement a different one with some advice/guidance

@graemerocher graemerocher self-assigned this Jun 22, 2021
Micronaut Developers Work Coordination automation moved this from Review in progress to Reviewer approved Jun 30, 2021
@graemerocher
Copy link
Contributor

Thanks for the contribution, this looks good

@graemerocher
Copy link
Contributor

Could you address the compilation errors?

Copy link
Contributor

@graemerocher graemerocher left a comment

Choose a reason for hiding this comment

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

Resolve compilation errors please.

Micronaut Developers Work Coordination automation moved this from Reviewer approved to Review in progress Jun 30, 2021
@bugflux
Copy link
Author

bugflux commented Jul 1, 2021

I believe I fixed the Java 8 compilation issue. I'm not entirely sure whether GraalVM's build will be fixed though, I'm not sure why it failed

@bugflux
Copy link
Author

bugflux commented Jul 1, 2021

Looks like one test is flaky, perhaps some concurrency issue and/or misuse of index-based access to queues, but I'm not sure since I don't have a strong grasp of the language. I'll look more into it but any input would be appreciated.

@graemerocher
Copy link
Contributor

yeah getting async tests right can be challenging it might be best to wait first until the size of the collection is what you expect and then do any index based queries

Obviously the messages aren't necessarily received in the same order that they are sent, since in these tests they travel in different partitions
@bugflux
Copy link
Author

bugflux commented Jul 2, 2021

It was fairly obvious, actually. Should be good now. I appreciate the patience :-)

@graemerocher graemerocher merged commit 571b663 into micronaut-projects:master Jul 5, 2021
Micronaut Developers Work Coordination automation moved this from Review in progress to Done Jul 5, 2021
@graemerocher
Copy link
Contributor

Thanks for the contribution!

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

Successfully merging this pull request may close these issues.

None yet

3 participants