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.

Sign up for GitHub

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

Jump to bottom

Use schema hash in producer cache key #43

Merged
merged 1 commit into from
Jul 28, 2022
Merged

Use schema hash in producer cache key #43

merged 1 commit into from
Jul 28, 2022

Conversation

onobc
Copy link
Collaborator

@onobc onobc commented Jul 28, 2022

This proposal enhances the producer cache key as follows:

  1. Renamed to ProducerCacheKey
  2. Uses SchemaHash to take into account schema bytes and schema type

ℹ️ NOTE: @alpreu @sobychacko we recently talked about doing something more sophisticated for the MessageRouter as part of the cache key. I ended up dropping that because those impls we were looking at (single, roundrobin) are really internal routers and not meant to be set by calling ProducerBuilder.messageRouter. Rather, these routers are custom (aka user-provided). If the custom router does not implement its own EQ/HC then it will fallback to identity EQ/HC. In a Spring based world it is highly likely that the router is a bean and there is only a single instance and this would cache well. If the user is passing in different router instances then its possible we could get cache misses. I am ok w/ documenting this fact and moving forward for now. Thoughts?

@onobc onobc requested a review from sobychacko July 28, 2022 06:15
@onobc onobc force-pushed the cbono-fix-cache-keys branch from 998b96f to ef38bab Compare July 28, 2022 06:22
onobc
onobc commented Jul 28, 2022
View reviewed changes
spring-pulsar/src/main/java/org/springframework/pulsar/core/CachingPulsarProducerFactory.java
static class ProducerCacheKey<T> {

private final Schema<T> schema;
private final SchemaHash schemaHash;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately, having the create a new instance field (schemaHash) is not supported in records. Had to go back to hand-written POJO for now.

@alpreu
Copy link
Contributor

alpreu commented Jul 28, 2022

The changes look good to me 👍

As discussed offline, I asked around and SchemaHash is considered part of the private API. For the moment, it should be fine to rely on it anyway. We can go ahead with creating a PIP for introducing a hashCode for the Schema to the public API. This way we can continue development now and exchange the implementation detail later. I can create a draft for the proposal and send it over for you to review. What do you think?

@onobc
Copy link
Collaborator Author

onobc commented Jul 28, 2022

The changes look good to me 👍

As discussed offline, I asked around and SchemaHash is considered part of the private API. For the moment, it should be fine to rely on it anyway. We can go ahead with creating a PIP for introducing a hashCode for the Schema to the public API. This way we can continue development now and exchange the implementation detail later. I can create a draft for the proposal and send it over for you to review. What do you think?

Thanks @alpreu , that sounds great.

@sobychacko sobychacko merged commit c57f5dd into spring-projects:main Jul 28, 2022
@sobychacko
Copy link
Collaborator

@alpreu Sounds like a plan. Please follow up with the PIP idea.

@onobc onobc deleted the cbono-fix-cache-keys branch September 11, 2022 17:30
@onobc onobc mentioned this pull request Nov 6, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sobychacko sobychacko sobychacko approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@onobc @alpreu @sobychacko