-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Remove incorrect utf8 conversion of ResultCache keys #16569
Conversation
server/src/test/java/org/apache/druid/client/cache/CaffeineCacheTest.java
Fixed
Show fixed
Hide fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm 👍
@@ -34,6 +34,8 @@ | |||
|
|||
public class CacheUtil | |||
{ | |||
private static final String RESULT_CACHE_NS = "ResultCacheNS"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe make a version of NamedKey
for result cache where the nsBytes
is already computed so we don't have to call StringUtils.toUtf8
on the constant everytime. Also could probably use a smaller string since the segment level cache uses segment identifiers for the namespace, so unlikely to have any collisions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed the resultcache namespace to RES
the toUtf8
stuff is a bit more complicated:
- I wanted to push out the
String/byte[]
conversion from theNamedKey
and change the namespace to usebyte[]
;- however: the
MapCache
directly accesses the namespace field of theNamedKey
class- since its being used as a key in a
Map
it must provide a validequals
- this could be addressed with a small refactor around there to useByteBuffer
- since its being used as a key in a
- the
namespace
also appears as an argument to Cache#close - so this will kinda force that to change as well- this could force changes in the segment cache key generation stuff
- ...so this path leads to refactor(s)
- however: the
- I could store the
byte[]
next to theString
by passing that as well in the constructor - but I would rather not do that unless there are measurable benefits of doing so - status quo also have some pros: the
toByteArray
is not used in all cache implementations - so thetoUtf8
might not be even called
I think for now it would be best to just leave this alone; not passing the full cachekey as the namespace have kinda reduced the key sizes to around half
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool, re complicated stuff i guess i was imagining just subclassing it or something so parts could be constant, but if its complicated then seems fine to skip
Cache
implementations to handle thebyte[]
inNamedKey
- its already abyte[]
- so they shouldn't expect any better than that...Cache.NamedKey
instead of passingbyte[]
and creating the key at multiple placesFixes #16552