You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by GitBox <gi...@apache.org> on 2021/08/13 04:34:14 UTC

[GitHub] [shardingsphere] strongduanmu opened a new pull request #11796: solve the OrderedServicesCache concurrency exception

strongduanmu opened a new pull request #11796:
URL: https://github.com/apache/shardingsphere/pull/11796


   Fixes #11307.
   
   Changes proposed in this pull request:
   - solve the OrderedServicesCache concurrency exception
   - use guava cache to replace ConcurrentHashMap
   


-- 
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@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] codecov-commenter commented on pull request #11796: solve the OrderedServicesCache concurrency exception

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #11796:
URL: https://github.com/apache/shardingsphere/pull/11796#issuecomment-898215620


   # [Codecov](https://codecov.io/gh/apache/shardingsphere/pull/11796?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#11796](https://codecov.io/gh/apache/shardingsphere/pull/11796?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4416f7b) into [master](https://codecov.io/gh/apache/shardingsphere/commit/b9dc6ca5592e406d53a21591ef4e9b2d30ab3754?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b9dc6ca) will **increase** coverage by `0.19%`.
   > The diff coverage is `60.81%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/shardingsphere/pull/11796/graphs/tree.svg?width=650&height=150&src=pr&token=ZvlXpWa7so&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/shardingsphere/pull/11796?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master   #11796      +/-   ##
   ============================================
   + Coverage     63.51%   63.71%   +0.19%     
     Complexity     1196     1196              
   ============================================
     Files          2306     2306              
     Lines         34884    34894      +10     
     Branches       6047     6049       +2     
   ============================================
   + Hits          22155    22231      +76     
   + Misses        10957    10887      -70     
   - Partials       1772     1776       +4     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/shardingsphere/pull/11796?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...stsql/parser/api/DistSQLStatementParserEngine.java](https://codecov.io/gh/apache/shardingsphere/pull/11796/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtZGlzdHNxbC1wYXJzZXIvc2hhcmRpbmdzcGhlcmUtZGlzdHNxbC1wYXJzZXItZW5naW5lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9zaGFyZGluZ3NwaGVyZS9kaXN0c3FsL3BhcnNlci9hcGkvRGlzdFNRTFN0YXRlbWVudFBhcnNlckVuZ2luZS5qYXZh) | `0.00% <0.00%> (ø)` | |
   | [...ore/rule/FeaturedDistSQLStatementParserEngine.java](https://codecov.io/gh/apache/shardingsphere/pull/11796/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtZGlzdHNxbC1wYXJzZXIvc2hhcmRpbmdzcGhlcmUtZGlzdHNxbC1wYXJzZXItZW5naW5lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9zaGFyZGluZ3NwaGVyZS9kaXN0c3FsL3BhcnNlci9jb3JlL3J1bGUvRmVhdHVyZWREaXN0U1FMU3RhdGVtZW50UGFyc2VyRW5naW5lLmphdmE=) | `0.00% <0.00%> (ø)` | |
   | [...ql/parser/core/DatabaseDiscoveryDistSQLParser.java](https://codecov.io/gh/apache/shardingsphere/pull/11796/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtZmVhdHVyZXMvc2hhcmRpbmdzcGhlcmUtZGItZGlzY292ZXJ5L3NoYXJkaW5nc3BoZXJlLWRiLWRpc2NvdmVyeS1kaXN0c3FsL3NoYXJkaW5nc3BoZXJlLWRiLWRpc2NvdmVyeS1kaXN0c3FsLXBhcnNlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2hhcmRpbmdzcGhlcmUvZGJkaXNjb3ZlcnkvZGlzdHNxbC9wYXJzZXIvY29yZS9EYXRhYmFzZURpc2NvdmVyeURpc3RTUUxQYXJzZXIuamF2YQ==) | `0.00% <ø> (ø)` | |
   | [...core/DatabaseDiscoveryDistSQLStatementVisitor.java](https://codecov.io/gh/apache/shardingsphere/pull/11796/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtZmVhdHVyZXMvc2hhcmRpbmdzcGhlcmUtZGItZGlzY292ZXJ5L3NoYXJkaW5nc3BoZXJlLWRiLWRpc2NvdmVyeS1kaXN0c3FsL3NoYXJkaW5nc3BoZXJlLWRiLWRpc2NvdmVyeS1kaXN0c3FsLXBhcnNlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2hhcmRpbmdzcGhlcmUvZGJkaXNjb3ZlcnkvZGlzdHNxbC9wYXJzZXIvY29yZS9EYXRhYmFzZURpc2NvdmVyeURpc3RTUUxTdGF0ZW1lbnRWaXNpdG9yLmphdmE=) | `0.00% <0.00%> (ø)` | |
   | [...DatabaseDiscoveryDistSQLStatementParserFacade.java](https://codecov.io/gh/apache/shardingsphere/pull/11796/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtZmVhdHVyZXMvc2hhcmRpbmdzcGhlcmUtZGItZGlzY292ZXJ5L3NoYXJkaW5nc3BoZXJlLWRiLWRpc2NvdmVyeS1kaXN0c3FsL3NoYXJkaW5nc3BoZXJlLWRiLWRpc2NvdmVyeS1kaXN0c3FsLXBhcnNlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2hhcmRpbmdzcGhlcmUvZGJkaXNjb3ZlcnkvZGlzdHNxbC9wYXJzZXIvZmFjYWRlL0RhdGFiYXNlRGlzY292ZXJ5RGlzdFNRTFN0YXRlbWVudFBhcnNlckZhY2FkZS5qYXZh) | `0.00% <0.00%> (ø)` | |
   | [...rypt/distsql/parser/core/EncryptDistSQLParser.java](https://codecov.io/gh/apache/shardingsphere/pull/11796/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtZmVhdHVyZXMvc2hhcmRpbmdzcGhlcmUtZW5jcnlwdC9zaGFyZGluZ3NwaGVyZS1lbmNyeXB0LWRpc3RzcWwvc2hhcmRpbmdzcGhlcmUtZW5jcnlwdC1kaXN0c3FsLXBhcnNlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2hhcmRpbmdzcGhlcmUvZW5jcnlwdC9kaXN0c3FsL3BhcnNlci9jb3JlL0VuY3J5cHREaXN0U1FMUGFyc2VyLmphdmE=) | `0.00% <ø> (ø)` | |
   | [...ql/parser/core/EncryptDistSQLStatementVisitor.java](https://codecov.io/gh/apache/shardingsphere/pull/11796/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtZmVhdHVyZXMvc2hhcmRpbmdzcGhlcmUtZW5jcnlwdC9zaGFyZGluZ3NwaGVyZS1lbmNyeXB0LWRpc3RzcWwvc2hhcmRpbmdzcGhlcmUtZW5jcnlwdC1kaXN0c3FsLXBhcnNlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2hhcmRpbmdzcGhlcmUvZW5jcnlwdC9kaXN0c3FsL3BhcnNlci9jb3JlL0VuY3J5cHREaXN0U1FMU3RhdGVtZW50VmlzaXRvci5qYXZh) | `0.00% <0.00%> (ø)` | |
   | [...er/facade/EncryptDistSQLStatementParserFacade.java](https://codecov.io/gh/apache/shardingsphere/pull/11796/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtZmVhdHVyZXMvc2hhcmRpbmdzcGhlcmUtZW5jcnlwdC9zaGFyZGluZ3NwaGVyZS1lbmNyeXB0LWRpc3RzcWwvc2hhcmRpbmdzcGhlcmUtZW5jcnlwdC1kaXN0c3FsLXBhcnNlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2hhcmRpbmdzcGhlcmUvZW5jcnlwdC9kaXN0c3FsL3BhcnNlci9mYWNhZGUvRW5jcnlwdERpc3RTUUxTdGF0ZW1lbnRQYXJzZXJGYWNhZGUuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...l/parser/core/ReadwriteSplittingDistSQLParser.java](https://codecov.io/gh/apache/shardingsphere/pull/11796/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtZmVhdHVyZXMvc2hhcmRpbmdzcGhlcmUtcmVhZHdyaXRlLXNwbGl0dGluZy9zaGFyZGluZ3NwaGVyZS1yZWFkd3JpdGUtc3BsaXR0aW5nLWRpc3RzcWwvc2hhcmRpbmdzcGhlcmUtcmVhZHdyaXRlLXNwbGl0dGluZy1kaXN0c3FsLXBhcnNlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2hhcmRpbmdzcGhlcmUvcmVhZHdyaXRlc3BsaXR0aW5nL2Rpc3RzcWwvcGFyc2VyL2NvcmUvUmVhZHdyaXRlU3BsaXR0aW5nRGlzdFNRTFBhcnNlci5qYXZh) | `0.00% <ø> (ø)` | |
   | [...ore/ReadwriteSplittingDistSQLStatementVisitor.java](https://codecov.io/gh/apache/shardingsphere/pull/11796/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtZmVhdHVyZXMvc2hhcmRpbmdzcGhlcmUtcmVhZHdyaXRlLXNwbGl0dGluZy9zaGFyZGluZ3NwaGVyZS1yZWFkd3JpdGUtc3BsaXR0aW5nLWRpc3RzcWwvc2hhcmRpbmdzcGhlcmUtcmVhZHdyaXRlLXNwbGl0dGluZy1kaXN0c3FsLXBhcnNlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2hhcmRpbmdzcGhlcmUvcmVhZHdyaXRlc3BsaXR0aW5nL2Rpc3RzcWwvcGFyc2VyL2NvcmUvUmVhZHdyaXRlU3BsaXR0aW5nRGlzdFNRTFN0YXRlbWVudFZpc2l0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | ... and [22 more](https://codecov.io/gh/apache/shardingsphere/pull/11796/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/shardingsphere/pull/11796?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/shardingsphere/pull/11796?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [56acbff...4416f7b](https://codecov.io/gh/apache/shardingsphere/pull/11796?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] terrymanu merged pull request #11796: solve the OrderedServicesCache concurrency exception

Posted by GitBox <gi...@apache.org>.
terrymanu merged pull request #11796:
URL: https://github.com/apache/shardingsphere/pull/11796


   


-- 
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@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] strongduanmu commented on a change in pull request #11796: solve the OrderedServicesCache concurrency exception

Posted by GitBox <gi...@apache.org>.
strongduanmu commented on a change in pull request #11796:
URL: https://github.com/apache/shardingsphere/pull/11796#discussion_r688271566



##########
File path: shardingsphere-infra/shardingsphere-infra-common/src/main/java/org/apache/shardingsphere/infra/spi/ordered/cache/OrderedServicesCache.java
##########
@@ -17,50 +17,45 @@
 
 package org.apache.shardingsphere.infra.spi.ordered.cache;
 
+import com.google.common.cache.Cache;
+import com.google.common.cache.CacheBuilder;
 import lombok.AccessLevel;
 import lombok.NoArgsConstructor;
 import org.apache.shardingsphere.infra.spi.ordered.OrderedSPI;
 
-import java.util.Collection;
 import java.util.Map;
 import java.util.Optional;
-import java.util.concurrent.ConcurrentHashMap;
 
 /**
  * Ordered services cached.
  */
 @NoArgsConstructor(access = AccessLevel.PRIVATE)
 public final class OrderedServicesCache {
     
-    private static final Map<Class<?>, CachedOrderedServices> CACHED_SERVICES = new ConcurrentHashMap<>();
+    private static final Cache<OrderedServicesCacheKey, Map<?, ?>> CACHED_SERVICES = CacheBuilder.newBuilder().softValues().build();
     
     /**
      * Find cached services.
      * 
-     * @param types types
-     * @param orderedSPIClass class of ordered SPI
+     * @param cacheKey cache key
      * @param <K> type of key
      * @param <V> type of ordered SPI class
      * @return cached ordered services
      */
-    public static <K, V extends OrderedSPI<?>> Optional<CachedOrderedServices> findCachedServices(final Collection<K> types, final Class<V> orderedSPIClass) {
-        return isHitCache(types, orderedSPIClass) ? Optional.of(CACHED_SERVICES.get(orderedSPIClass)) : Optional.empty();
-    }
-    
-    private static <K, V extends OrderedSPI<?>> boolean isHitCache(final Collection<K> types, final Class<V> orderedSPIClass) {
-        return CACHED_SERVICES.containsKey(orderedSPIClass) && CACHED_SERVICES.get(orderedSPIClass).getTypes().equals(types);
+    @SuppressWarnings("unchecked")
+    public static <K, V extends OrderedSPI<?>> Optional<Map<K, V>> findCachedServices(final OrderedServicesCacheKey cacheKey) {
+        return Optional.ofNullable(CACHED_SERVICES.getIfPresent(cacheKey)).map(optional -> (Map<K, V>) optional);
     }
     
     /**
      * Cache services.
      * 
-     * @param types types
-     * @param orderedSPIClass class of ordered SPI
+     * @param cacheKey cache key
      * @param services ordered services
      * @param <K> type of key
      * @param <V> type of ordered SPI class
      */
-    public static <K, V extends OrderedSPI<?>> void cacheServices(final Collection<K> types, final Class<V> orderedSPIClass, final Map<K, V> services) {
-        CACHED_SERVICES.put(orderedSPIClass, new CachedOrderedServices(types, services));
+    public static <K, V extends OrderedSPI<?>> void cacheServices(final OrderedServicesCacheKey cacheKey, final Map<K, V> services) {

Review comment:
       > Is it necessary to add generic types `K` and `V`?
   
   @terrymanu I will try to remove them.




-- 
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@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] terrymanu commented on a change in pull request #11796: solve the OrderedServicesCache concurrency exception

Posted by GitBox <gi...@apache.org>.
terrymanu commented on a change in pull request #11796:
URL: https://github.com/apache/shardingsphere/pull/11796#discussion_r688257227



##########
File path: shardingsphere-infra/shardingsphere-infra-common/src/main/java/org/apache/shardingsphere/infra/spi/ordered/OrderedSPIRegistry.java
##########
@@ -77,18 +77,18 @@
      * @param comparator comparator
      * @return registered services
      */
-    @SuppressWarnings("unchecked")
     public static <K, V extends OrderedSPI<?>> Map<K, V> getRegisteredServices(final Collection<K> types, final Class<V> orderedSPIClass, final Comparator<Integer> comparator) {

Review comment:
       Is it better to adjust the param sequence too?
   `orderedSPIClass` maybe the first param, `types` is second one.

##########
File path: shardingsphere-infra/shardingsphere-infra-common/src/main/java/org/apache/shardingsphere/infra/spi/ordered/cache/OrderedServicesCacheKey.java
##########
@@ -17,20 +17,19 @@
 
 package org.apache.shardingsphere.infra.spi.ordered.cache;
 
-import lombok.Getter;
+import lombok.EqualsAndHashCode;
 import lombok.RequiredArgsConstructor;
 
 import java.util.Collection;
-import java.util.Map;
 
 /**
- * Cached ordered services.
+ * Ordered services cache key.
  */
+@EqualsAndHashCode

Review comment:
       Please adjust order, constructor should the first of annotation

##########
File path: shardingsphere-infra/shardingsphere-infra-common/src/main/java/org/apache/shardingsphere/infra/spi/ordered/cache/OrderedServicesCacheKey.java
##########
@@ -17,20 +17,19 @@
 
 package org.apache.shardingsphere.infra.spi.ordered.cache;
 
-import lombok.Getter;
+import lombok.EqualsAndHashCode;
 import lombok.RequiredArgsConstructor;
 
 import java.util.Collection;
-import java.util.Map;
 
 /**
- * Cached ordered services.
+ * Ordered services cache key.
  */
+@EqualsAndHashCode
 @RequiredArgsConstructor
-@Getter
-public final class CachedOrderedServices {
+public class OrderedServicesCacheKey {

Review comment:
       may add final with class




-- 
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@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] strongduanmu commented on a change in pull request #11796: solve the OrderedServicesCache concurrency exception

Posted by GitBox <gi...@apache.org>.
strongduanmu commented on a change in pull request #11796:
URL: https://github.com/apache/shardingsphere/pull/11796#discussion_r688271265



##########
File path: shardingsphere-infra/shardingsphere-infra-common/src/main/java/org/apache/shardingsphere/infra/spi/ordered/OrderedSPIRegistry.java
##########
@@ -77,18 +77,18 @@
      * @param comparator comparator
      * @return registered services
      */
-    @SuppressWarnings("unchecked")
     public static <K, V extends OrderedSPI<?>> Map<K, V> getRegisteredServices(final Collection<K> types, final Class<V> orderedSPIClass, final Comparator<Integer> comparator) {

Review comment:
       > Is it better to adjust the param sequence too?
   > `orderedSPIClass` maybe the first param, `types` is second one.
   
   @terrymanu Yes, it is better to understand. I will optimize it.




-- 
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@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] terrymanu commented on a change in pull request #11796: solve the OrderedServicesCache concurrency exception

Posted by GitBox <gi...@apache.org>.
terrymanu commented on a change in pull request #11796:
URL: https://github.com/apache/shardingsphere/pull/11796#discussion_r688259018



##########
File path: shardingsphere-infra/shardingsphere-infra-common/src/main/java/org/apache/shardingsphere/infra/spi/ordered/cache/OrderedServicesCache.java
##########
@@ -17,50 +17,45 @@
 
 package org.apache.shardingsphere.infra.spi.ordered.cache;
 
+import com.google.common.cache.Cache;
+import com.google.common.cache.CacheBuilder;
 import lombok.AccessLevel;
 import lombok.NoArgsConstructor;
 import org.apache.shardingsphere.infra.spi.ordered.OrderedSPI;
 
-import java.util.Collection;
 import java.util.Map;
 import java.util.Optional;
-import java.util.concurrent.ConcurrentHashMap;
 
 /**
  * Ordered services cached.
  */
 @NoArgsConstructor(access = AccessLevel.PRIVATE)
 public final class OrderedServicesCache {
     
-    private static final Map<Class<?>, CachedOrderedServices> CACHED_SERVICES = new ConcurrentHashMap<>();
+    private static final Cache<OrderedServicesCacheKey, Map<?, ?>> CACHED_SERVICES = CacheBuilder.newBuilder().softValues().build();
     
     /**
      * Find cached services.
      * 
-     * @param types types
-     * @param orderedSPIClass class of ordered SPI
+     * @param cacheKey cache key
      * @param <K> type of key
      * @param <V> type of ordered SPI class
      * @return cached ordered services
      */
-    public static <K, V extends OrderedSPI<?>> Optional<CachedOrderedServices> findCachedServices(final Collection<K> types, final Class<V> orderedSPIClass) {
-        return isHitCache(types, orderedSPIClass) ? Optional.of(CACHED_SERVICES.get(orderedSPIClass)) : Optional.empty();
-    }
-    
-    private static <K, V extends OrderedSPI<?>> boolean isHitCache(final Collection<K> types, final Class<V> orderedSPIClass) {
-        return CACHED_SERVICES.containsKey(orderedSPIClass) && CACHED_SERVICES.get(orderedSPIClass).getTypes().equals(types);
+    @SuppressWarnings("unchecked")
+    public static <K, V extends OrderedSPI<?>> Optional<Map<K, V>> findCachedServices(final OrderedServicesCacheKey cacheKey) {
+        return Optional.ofNullable(CACHED_SERVICES.getIfPresent(cacheKey)).map(optional -> (Map<K, V>) optional);
     }
     
     /**
      * Cache services.
      * 
-     * @param types types
-     * @param orderedSPIClass class of ordered SPI
+     * @param cacheKey cache key
      * @param services ordered services
      * @param <K> type of key
      * @param <V> type of ordered SPI class
      */
-    public static <K, V extends OrderedSPI<?>> void cacheServices(final Collection<K> types, final Class<V> orderedSPIClass, final Map<K, V> services) {
-        CACHED_SERVICES.put(orderedSPIClass, new CachedOrderedServices(types, services));
+    public static <K, V extends OrderedSPI<?>> void cacheServices(final OrderedServicesCacheKey cacheKey, final Map<K, V> services) {

Review comment:
       Is it necessary to add generic type `K` and `V`? 




-- 
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@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] strongduanmu commented on a change in pull request #11796: solve the OrderedServicesCache concurrency exception

Posted by GitBox <gi...@apache.org>.
strongduanmu commented on a change in pull request #11796:
URL: https://github.com/apache/shardingsphere/pull/11796#discussion_r688271810



##########
File path: shardingsphere-infra/shardingsphere-infra-common/src/main/java/org/apache/shardingsphere/infra/spi/ordered/cache/OrderedServicesCacheKey.java
##########
@@ -17,20 +17,19 @@
 
 package org.apache.shardingsphere.infra.spi.ordered.cache;
 
-import lombok.Getter;
+import lombok.EqualsAndHashCode;
 import lombok.RequiredArgsConstructor;
 
 import java.util.Collection;
-import java.util.Map;
 
 /**
- * Cached ordered services.
+ * Ordered services cache key.
  */
+@EqualsAndHashCode
 @RequiredArgsConstructor
-@Getter
-public final class CachedOrderedServices {
+public class OrderedServicesCacheKey {

Review comment:
       > How about consider move this class as inner class of `OrderedServicesCache` and simplify the class name as `Key`?
   
   @terrymanu It's a goods idea, i will optimize it.




-- 
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@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] terrymanu commented on a change in pull request #11796: solve the OrderedServicesCache concurrency exception

Posted by GitBox <gi...@apache.org>.
terrymanu commented on a change in pull request #11796:
URL: https://github.com/apache/shardingsphere/pull/11796#discussion_r688259018



##########
File path: shardingsphere-infra/shardingsphere-infra-common/src/main/java/org/apache/shardingsphere/infra/spi/ordered/cache/OrderedServicesCache.java
##########
@@ -17,50 +17,45 @@
 
 package org.apache.shardingsphere.infra.spi.ordered.cache;
 
+import com.google.common.cache.Cache;
+import com.google.common.cache.CacheBuilder;
 import lombok.AccessLevel;
 import lombok.NoArgsConstructor;
 import org.apache.shardingsphere.infra.spi.ordered.OrderedSPI;
 
-import java.util.Collection;
 import java.util.Map;
 import java.util.Optional;
-import java.util.concurrent.ConcurrentHashMap;
 
 /**
  * Ordered services cached.
  */
 @NoArgsConstructor(access = AccessLevel.PRIVATE)
 public final class OrderedServicesCache {
     
-    private static final Map<Class<?>, CachedOrderedServices> CACHED_SERVICES = new ConcurrentHashMap<>();
+    private static final Cache<OrderedServicesCacheKey, Map<?, ?>> CACHED_SERVICES = CacheBuilder.newBuilder().softValues().build();
     
     /**
      * Find cached services.
      * 
-     * @param types types
-     * @param orderedSPIClass class of ordered SPI
+     * @param cacheKey cache key
      * @param <K> type of key
      * @param <V> type of ordered SPI class
      * @return cached ordered services
      */
-    public static <K, V extends OrderedSPI<?>> Optional<CachedOrderedServices> findCachedServices(final Collection<K> types, final Class<V> orderedSPIClass) {
-        return isHitCache(types, orderedSPIClass) ? Optional.of(CACHED_SERVICES.get(orderedSPIClass)) : Optional.empty();
-    }
-    
-    private static <K, V extends OrderedSPI<?>> boolean isHitCache(final Collection<K> types, final Class<V> orderedSPIClass) {
-        return CACHED_SERVICES.containsKey(orderedSPIClass) && CACHED_SERVICES.get(orderedSPIClass).getTypes().equals(types);
+    @SuppressWarnings("unchecked")
+    public static <K, V extends OrderedSPI<?>> Optional<Map<K, V>> findCachedServices(final OrderedServicesCacheKey cacheKey) {
+        return Optional.ofNullable(CACHED_SERVICES.getIfPresent(cacheKey)).map(optional -> (Map<K, V>) optional);
     }
     
     /**
      * Cache services.
      * 
-     * @param types types
-     * @param orderedSPIClass class of ordered SPI
+     * @param cacheKey cache key
      * @param services ordered services
      * @param <K> type of key
      * @param <V> type of ordered SPI class
      */
-    public static <K, V extends OrderedSPI<?>> void cacheServices(final Collection<K> types, final Class<V> orderedSPIClass, final Map<K, V> services) {
-        CACHED_SERVICES.put(orderedSPIClass, new CachedOrderedServices(types, services));
+    public static <K, V extends OrderedSPI<?>> void cacheServices(final OrderedServicesCacheKey cacheKey, final Map<K, V> services) {

Review comment:
       Is it necessary to add generic types `K` and `V`? 




-- 
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@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] terrymanu commented on a change in pull request #11796: solve the OrderedServicesCache concurrency exception

Posted by GitBox <gi...@apache.org>.
terrymanu commented on a change in pull request #11796:
URL: https://github.com/apache/shardingsphere/pull/11796#discussion_r688258172



##########
File path: shardingsphere-infra/shardingsphere-infra-common/src/main/java/org/apache/shardingsphere/infra/spi/ordered/cache/OrderedServicesCacheKey.java
##########
@@ -17,20 +17,19 @@
 
 package org.apache.shardingsphere.infra.spi.ordered.cache;
 
-import lombok.Getter;
+import lombok.EqualsAndHashCode;
 import lombok.RequiredArgsConstructor;
 
 import java.util.Collection;
-import java.util.Map;
 
 /**
- * Cached ordered services.
+ * Ordered services cache key.
  */
+@EqualsAndHashCode
 @RequiredArgsConstructor
-@Getter
-public final class CachedOrderedServices {
+public class OrderedServicesCacheKey {

Review comment:
       How about consider move this class as inner class of `OrderedServicesCache` and simplify the class name as `Key`?




-- 
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@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org