lhotari commented on PR #23808:
URL: https://github.com/apache/pulsar/pull/23808#issuecomment-2569689969
> ok,i test all and could you review it and give me some suggestions
Instead of adding more code to test everything, please reduce to a minimal
implementation. This means to remove features to track cache metrics. That's
not something that is needed. For the cache implementation, I'd suggest using a
`ConcurrentMap` created with Guava's MapMaker. Instead of adding yet another
abstraction, I'd suggest modifying the `PulsarClientImplementationBinding`
interface and adding a new interface method `<T extends
com.google.protobuf.GeneratedMessageV3> Schema<T> newProtobufSchema(Class<T>
clazz)`. Then we could keep the cache as an implementation level detail.
example of minimal implementation for `newProtobufSchema` using Guava's
MapMaker with weak keys:
```java
private static final ConcurrentMap<Class<?>, Schema<?>> PROTOBUF_CACHE =
new MapMaker().weakKeys().makeMap();
public <T extends com.google.protobuf.GeneratedMessageV3> Schema<T>
newProtobufSchema(Class<T> clazz) {
return (Schema<T>) PROTOBUF_CACHE.computeIfAbsent(clazz,
k ->
ProtobufSchema.of(SchemaDefinition.builder().withPojo(clazz).build())).clone();
}
```
There shouldn't be a need to ever clear the cache since it's bounded by the
number of classes with strong references. It won't consume a significant amount
of memory in the first place.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
