You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "DomGarguilo (via GitHub)" <gi...@apache.org> on 2024/02/28 15:27:56 UTC
[I] Consider adding caching to more ID objects. [accumulo]
DomGarguilo opened a new issue, #4316:
URL: https://github.com/apache/accumulo/issues/4316
**Is your feature request related to a problem? Please describe.**
#4313 added caching to CompactorGroupId like this:
```java
static final Cache<String,CompactorGroupId> cache = Caches.getInstance()
.createNewBuilder(Caches.CacheName.COMPACTOR_GROUP_ID, false).weakValues().build();
```
There are several other ID objects that already have caching too like `TableId`, `NamespaceId` and `InstanceId`.
**Describe the solution you'd like**
Consider adding similar caching to the following ID objects:
* ExternalCompactionId
* CompactionExecutorId
* CompactionServiceId
* FateId
* TabletOperationId
There may be others but these are the objects that `extends AbstractId`.
--
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: notifications-unsubscribe@accumulo.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Consider adding caching to more ID objects. [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on issue #4316:
URL: https://github.com/apache/accumulo/issues/4316#issuecomment-1976928850
> Sorry, I wasn't clear. I was suggesting that if we are going to cache Ids, that we may want to have them expire from the cache after some period of time.
The reason behind the caching is that it is expected that some ids are low cardinality and used very frequently. With this in mind, wondering if we should have size limited and time limited caches. The point of the caches is that we expect it to be small, size limiting it would guard against exceptions to that expectation. Time limiting clears out junk that is not being used any more (like a table id for a table that was deleted).
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Consider adding caching to more ID objects. [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion closed issue #4316: Consider adding caching to more ID objects.
URL: https://github.com/apache/accumulo/issues/4316
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Consider adding caching to more ID objects. [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on issue #4316:
URL: https://github.com/apache/accumulo/issues/4316#issuecomment-1971344540
> We likely also want to have Id expire after some period of time (a day?)
Are you suggesting this for the existing objects that are cached? Or if new ones are added?
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Consider adding caching to more ID objects. [accumulo]
Posted by "ArbaazKhan1 (via GitHub)" <gi...@apache.org>.
ArbaazKhan1 commented on issue #4316:
URL: https://github.com/apache/accumulo/issues/4316#issuecomment-1969684204
Mind if I take a look at this
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Consider adding caching to more ID objects. [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on issue #4316:
URL: https://github.com/apache/accumulo/issues/4316#issuecomment-1970029925
I wonder if we really need to cache the Id object. Each Id is a wrapper around a String. I wonder if the following would be sufficient:
```
protected AbstractId(final String canonical) {
- this.canonical = Objects.requireNonNull(canonical, "canonical cannot be null");
+ this.canonical = Objects.requireNonNull(canonical, "canonical cannot be null").intern();
}
```
I realize that by Caching the Id object we are saving the time it takes to create it, but we are also incurring the overhead of having a Cache for each Id type.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Consider adding caching to more ID objects. [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on issue #4316:
URL: https://github.com/apache/accumulo/issues/4316#issuecomment-1973102524
> > We likely also want to have Id expire after some period of time (a day?)
>
> Are you suggesting this for the existing objects that are cached? Or if new ones are added?
Sorry, I wasn't clear. I was suggesting that if we are going to cache Ids, that we may want to have them expire from the cache after some period of time. Adding a cache in the AbstractId implementations will affect the server and client. I think we want to be mindful of consuming client side memory for long periods of time for something that might not be used.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Consider adding caching to more ID objects. [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on issue #4316:
URL: https://github.com/apache/accumulo/issues/4316#issuecomment-1986075875
The purpose of the existing caches is de-duplication, not rapid on-demand access, like we think of most caches. When these IDs were just String types, we could use something similar to String.intern, but as a concrete type, we need the weak values cache to de-duplicate the wrapper object around the String canonical form. Expiring due to size or time would only cause duplicates to start appearing, which defeats the purpose of the cache. Sure, it would keep the cache smaller, but the objects already get removed from the cache when they are garbage collected, and removing the reference in the cache isn't going to remove the memory used by the object that's still in the JVM. I believe we already have unit tests to make sure the weak values caching mechanism is working correctly with garbage collection.
So, for the existing caches, I don't think any change needs to be made.
That said, this ticket is about caching more types, not necessarily changing the existing caching. In that context, it's important to note that the de-duplication caches really only benefit objects that are frequently duplicated (regardless of cardinality, though types with low-cardinality relative to the frequency of object creation will benefit the most from de-duplication). These typically come from instantiation of the wrapper concrete type when deserializing the canonical representation from user input, ZooKeeper, the metadata table, or similar. Things like TableId and NamespaceId clearly benefit from these. However, not all of these ID types are prone to duplication due to deserialization of their canonical forms. Each one would have to be evaluated to determine if they are subject to frequent duplication, and would benefit from a de-duplication cache, on a case-by-case basis.
It's also possible that some of these would benefit from a different kind of cache that might have something like time or size limits, but again, that would require each type to be evaluated on a case-by-case basis in the context of that cache's purpose.
If a cache is added, it should be done because it's clear that it would benefit from a the cache being added, whether it's a de-duplication cache to limit memory usage for frequently deserialized IDs with low cardinality, or if it's a cache for some other purpose.
Right now, I'm not sure we have any reason to believe any of these other ID types would benefit from additional caching. So, we could probably just close this issue, or somebody could dive in and do some experimentation and analysis to see if the other types would benefit from a cache. I suspect that it's unlikely they will benefit, but have not done the analysis myself, so it's only a suspicion.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Consider adding caching to more ID objects. [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on issue #4316:
URL: https://github.com/apache/accumulo/issues/4316#issuecomment-1970034095
Also, I'm not sure that we want to cache things like FateId which could be short-lived. We likely also want to have Id expire after some period of time (a day?)
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Consider adding caching to more ID objects. [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on issue #4316:
URL: https://github.com/apache/accumulo/issues/4316#issuecomment-1971311193
Probably do not want to cache high cardinality short lived ids like FateId and ExternalCompactionId.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org