[#3854] feat(core): Add a cache service to cache the mapping of name identifer to id #3986
Conversation
|
How to handle rename operation? |
Please see the code: 
|
core/src/main/java/org/apache/gravitino/storage/relational/service/NameIdMappingService.java
Show resolved
Hide resolved
| } | ||
|
|
||
| public Long get(EntityIdentifier key) { | ||
| return ident2IdCache.getIfPresent(key); |
There was a problem hiding this comment.
If the cache is not hit, does the upper layer need to retrieve it from the underlying storage by itself?
There was a problem hiding this comment.
Yes, in some scenarios we only need to judge whether the value associated with the key exists.
There was a problem hiding this comment.
I am a bit confused. Shouldn't the cacheService automatically help the caller to query the underlying layer when the cache expires? But I didn't see the logic of querying from the underlying storage in your cache.
There was a problem hiding this comment.
You can use
public Long get(EntityIdentifier key, Function<EntityIdentifier, Long> mappingFunction) {
return ident2IdCache.get(key, mappingFunction);
}
instead.
There was a problem hiding this comment.
Your class name is NameIdMappingService, not a caching service. Caching seems to be just an optimization technique in your service, so I think the cache should be transparent to the caller.
There was a problem hiding this comment.
Your class name is NameIdMappingService, not a caching service. Caching seems to be just an optimization technique in your service, so I think the cache should be transparent to the caller.
I don't understand your words clearly. Could you explain them in more detail?
Since the name of the class is NameIdMappingService for the caller, he only knows the id-name mapping and could not sense the cache mechanism, so what's the problem
There was a problem hiding this comment.
As your current implementation, Long get(EntityIdentifier key) makes the caller feel that they can directly obtain the id through the key, but in fact it only performs a cache query. That's my point.
core/src/main/java/org/apache/gravitino/storage/relational/service/NameIdMappingService.java
Show resolved
Hide resolved
| @@ -123,36 +126,54 @@ public boolean exists(NameIdentifier ident, Entity.EntityType entityType) throws | |||
| @Override | |||
| public <E extends Entity & HasIdentifier> void insert(E e, boolean overwritten) | |||
| throws EntityAlreadyExistsException, IOException { | |||
| EntityType entityType; | |||
There was a problem hiding this comment.
EntityType entityType = e.getType();
| } finally { | ||
| // Remove the entity from the cache again because we may add the cache during the deletion | ||
| // process | ||
| NameIdMappingService.getInstance().invalidateWithPrefix(entityIdentifier); |
There was a problem hiding this comment.
Maybe we should follow the pattern.
We write the slow storage first, then write the fast storage.
There was a problem hiding this comment.
Could you provide more details for the sentence We write the slow storage first, then write the fast storage.?
| public void testGetInstance() throws IOException { | ||
| NameIdMappingService instance = NameIdMappingService.getInstance(); | ||
|
|
||
| EntityIdentifier makeLakeIdent1 = |
I am unable to envision this scenario at the moment. Can you give an example here?
Let me give it a try. It seems that we need to an abstract the class or proxy that delegate
OK |
|
This isn't good enough. Maybe we need a pluggable cache framework to let production users to implement their cache. Maybe Xiaomi wants to implement Redis cache. cc @lw-yang |
This PR is just make it up-to-date and the design is under discussion. |
OK, but you should make this draft first. |
Yes, we need a pluggable caching system that can implement the Redis cache for use in the production environment. |
What changes were proposed in this pull request?
Cache the mapping of name identifier to id.
Why are the changes needed?
To enhance the speed of API.
Fix: #3854
Does this PR introduce any user-facing change?
N/A
How was this patch tested?
N/A