Copilot commented on code in PR #9162:
URL: https://github.com/apache/gravitino/pull/9162#discussion_r2554899134
##########
core/src/main/java/org/apache/gravitino/cache/ReverseIndexCache.java:
##########
@@ -45,6 +47,25 @@ public class ReverseIndexCache {
/** Registers a reverse index processor for a specific entity class. */
private final Map<Class<? extends Entity>, ReverseIndexRule>
reverseIndexRules = new HashMap<>();
+ /**
+ * Map from data entity key to a list of entity cache relation keys. This is
used for reverse
+ * indexing.
+ *
+ * <p>For example, a role entity may be related to multiple securable
objects, so we need to
+ * maintain a mapping from the role entity key to the list of securable
object keys. that is
+ * dataToReverseIndexMap: roleEntityKey -> [securableObjectKey1,
securableObjectKey2, ...]
+ *
+ * <p>This map is used to quickly find all the related entity cache keys
when we need to
+ * invalidate in the reverse index if a role entity is updated. The
following is an example: a
+ * Role a has securable objects s1 and s2, so we have the following mapping:
<br>
+ * cacheData: role1 -> role entity reserveIndex: s1 -> [role1], s2 ->
[role1] </br>
Review Comment:
Spelling error: "reserveIndex" should be "reverseIndex" to match the actual
variable name and concept being described.
```suggestion
* cacheData: role1 -> role entity reverseIndex: s1 -> [role1], s2 ->
[role1] </br>
```
##########
core/src/main/java/org/apache/gravitino/cache/ReverseIndexCache.java:
##########
@@ -45,6 +47,25 @@ public class ReverseIndexCache {
/** Registers a reverse index processor for a specific entity class. */
private final Map<Class<? extends Entity>, ReverseIndexRule>
reverseIndexRules = new HashMap<>();
+ /**
+ * Map from data entity key to a list of entity cache relation keys. This is
used for reverse
+ * indexing.
+ *
+ * <p>For example, a role entity may be related to multiple securable
objects, so we need to
+ * maintain a mapping from the role entity key to the list of securable
object keys. that is
+ * dataToReverseIndexMap: roleEntityKey -> [securableObjectKey1,
securableObjectKey2, ...]
Review Comment:
Incorrect documentation: The description states "dataToReverseIndexMap:
roleEntityKey -> [securableObjectKey1, securableObjectKey2, ...]" but based on
the actual implementation in the `put` method (line 112), the mapping is
reversed. The key is the `EntityCacheRelationKey key` parameter (which
represents the securable object or data entity), and the value contains
`entityCacheKey` (the role entity).
The documentation should be corrected to reflect the actual mapping
direction. For example: "dataToReverseIndexMap: securableObjectKey ->
[roleEntityKey1, roleEntityKey2, ...]"
```suggestion
* Map from securable object (or data entity) key to a list of related
entity cache keys (such as role entity keys).
* This is used for reverse indexing.
*
* <p>For example, a role entity may be related to multiple securable
objects, so we need to
* maintain a mapping from the securable object key to the list of role
entity keys. That is,
* dataToReverseIndexMap: securableObjectKey -> [roleEntityKey1,
roleEntityKey2, ...]
```
##########
core/src/main/java/org/apache/gravitino/cache/ReverseIndexCache.java:
##########
@@ -45,6 +47,25 @@ public class ReverseIndexCache {
/** Registers a reverse index processor for a specific entity class. */
private final Map<Class<? extends Entity>, ReverseIndexRule>
reverseIndexRules = new HashMap<>();
+ /**
+ * Map from data entity key to a list of entity cache relation keys. This is
used for reverse
+ * indexing.
+ *
+ * <p>For example, a role entity may be related to multiple securable
objects, so we need to
+ * maintain a mapping from the role entity key to the list of securable
object keys. that is
+ * dataToReverseIndexMap: roleEntityKey -> [securableObjectKey1,
securableObjectKey2, ...]
+ *
+ * <p>This map is used to quickly find all the related entity cache keys
when we need to
+ * invalidate in the reverse index if a role entity is updated. The
following is an example: a
+ * Role a has securable objects s1 and s2, so we have the following mapping:
<br>
+ * cacheData: role1 -> role entity reserveIndex: s1 -> [role1], s2 ->
[role1] </br>
+ *
+ * <p>When we update role1, we need to invalidate s1 and s2 from the reverse
index, or the data
+ * will be in the memory forever. However, the current implementation of
ReverseIndexCache does
+ * not support this operation directly as we do not maintain such a map.
+ */
+ private Map<EntityCacheKey, List<EntityCacheKey>> dataToReverseIndexMap =
Maps.newHashMap();
Review Comment:
Thread safety issue: `dataToReverseIndexMap` is declared as a regular
`HashMap` using `Maps.newHashMap()`, but `ReverseIndexCache` is used in a
concurrent environment as evidenced by the use of `ConcurrentRadixTree` for
`reverseIndex`. The `remove()` and `put()` methods in this class access and
modify `dataToReverseIndexMap` without synchronization, which can lead to race
conditions, data corruption, or ConcurrentModificationException when accessed
from multiple threads.
Consider using `ConcurrentHashMap` instead:
```java
private Map<EntityCacheKey, List<EntityCacheKey>> dataToReverseIndexMap =
new ConcurrentHashMap<>();
```
Additionally, when adding to the list in the `put()` method (line 112),
you'll need to ensure thread-safe list operations since `computeIfAbsent` can
still have concurrent modifications to the list. Consider using
`CopyOnWriteArrayList` or synchronizing list modifications.
--
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]
