You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "kgyrtkirk (via GitHub)" <gi...@apache.org> on 2024/06/06 13:52:08 UTC
[PR] Remove incorrect utf8 conversion of ResultCache keys (druid)
kgyrtkirk opened a new pull request, #16569:
URL: https://github.com/apache/druid/pull/16569
* remove the incorrect call to read the byte[] as utf8
* not sure about its origin - but this will push the contract for `Cache` implementations to handle the `byte[]` in `NamedKey` - its already a `byte[]` - so they shouldn't expect any better than that...
* use `Cache.NamedKey` instead of passing `byte[]` and creating the key at multiple places
* added some test to ensure that cache implementations are able to accept such keys
Fixes #16552
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Remove incorrect utf8 conversion of ResultCache keys (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16569:
URL: https://github.com/apache/druid/pull/16569#discussion_r1635959372
##########
server/src/main/java/org/apache/druid/client/CacheUtil.java:
##########
@@ -34,6 +34,8 @@
public class CacheUtil
{
+ private static final String RESULT_CACHE_NS = "ResultCacheNS";
Review Comment:
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
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Remove incorrect utf8 conversion of ResultCache keys (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk commented on code in PR #16569:
URL: https://github.com/apache/druid/pull/16569#discussion_r1635945757
##########
server/src/main/java/org/apache/druid/client/CacheUtil.java:
##########
@@ -34,6 +34,8 @@
public class CacheUtil
{
+ private static final String RESULT_CACHE_NS = "ResultCacheNS";
Review Comment:
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 the `NamedKey` and change the namespace to use `byte[]`;
* however: the `MapCache` directly accesses the [namespace](https://github.com/apache/druid/blob/fee509df2ebade0d6a17f96ee0637a27acedb45d/server/src/main/java/org/apache/druid/client/cache/MapCache.java#L86) field of the `NamedKey` class
* since its being used as a key in a `Map` it must provide a valid `equals` - this could be addressed with a small refactor around there to use `ByteBuffer`
* the `namespace` also appears as an argument to [Cache#close](https://github.com/apache/druid/blob/fee509df2ebade0d6a17f96ee0637a27acedb45d/server/src/main/java/org/apache/druid/client/cache/Cache.java#L48) - 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)
* I could store the `byte[]` next to the `String` 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 the `toUtf8` 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
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Remove incorrect utf8 conversion of ResultCache keys (druid)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #16569:
URL: https://github.com/apache/druid/pull/16569#discussion_r1629654479
##########
server/src/test/java/org/apache/druid/client/cache/CaffeineCacheTest.java:
##########
@@ -65,6 +69,16 @@
cache = CaffeineCache.create(cacheConfig);
}
+ @Test
+ public void testKeyContainingNegativeBytest()
Review Comment:
## Missing Override annotation
This method overrides [CacheTestBase<CaffeineCache>.testKeyContainingNegativeBytest](1); it is advisable to add an Override annotation.
[Show more details](https://github.com/apache/druid/security/code-scanning/7440)
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Remove incorrect utf8 conversion of ResultCache keys (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #16569:
URL: https://github.com/apache/druid/pull/16569#discussion_r1635560935
##########
server/src/main/java/org/apache/druid/client/CacheUtil.java:
##########
@@ -34,6 +34,8 @@
public class CacheUtil
{
+ private static final String RESULT_CACHE_NS = "ResultCacheNS";
Review Comment:
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.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Remove incorrect utf8 conversion of ResultCache keys (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis merged PR #16569:
URL: https://github.com/apache/druid/pull/16569
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org