[GitHub] [geode] jvarenina commented on a change in pull request #5360: GEODE-8329: Fix for durable CQ reqistration recovery

[GitBox <gi...@apache.org>]

jvarenina commented on a change in pull request #5360:
URL: https://github.com/apache/geode/pull/5360#discussion_r452734828



##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/QueueManagerImpl.java
##########
@@ -1112,7 +1112,8 @@ private void recoverCqs(Connection recoveredConnection, boolean isDurable) {
             .set(((DefaultQueryService) this.pool.getQueryService()).getUserAttributes(name));
       }
       try {
-        if (((CqStateImpl) cqi.getState()).getState() != CqStateImpl.INIT) {
+        if (((CqStateImpl) cqi.getState()).getState() != CqStateImpl.INIT

Review comment:
       Hi @agingade ,
   
   Thanks for your comments!
   
   My understanding was that it is possible to configure durable client identity with the combination of non-durable and durable CQs/Interests. For Example the parameters and API used to configure durable client identity, CQs and region Interests :
   
   ```
   // API
   registerInterestForAllKeys(InterestResultPolicy policy, boolean isDurable)
   newCq(String queryString, CqAttributes cqAttr, boolean isDurable)
   // Parameters
   durable-client-id, durable-client-timeout
   ```
   
   If you look at the `recoverAllInterestTypes` function code with this in mind, then the code there make sense:
      
   -  First register non-durable CQs and Interests
   -  In case of durable client identity, then also register durable CQs and Interests (Not possible to have durable CQ/Interests without durable client identity)
   
   Also when we look deeper at function level in the `recoverInterestList->recoverSingleList->getRegionToInterestsMap->getInterestLookupIndex(isDurable, receiveUpdatesAsInvalidates)` :
   
   ```
   public static int getInterestLookupIndex(boolean isDurable, boolean receiveUpdatesAsInvalidates) {
     if (isDurable) {
       if (receiveUpdatesAsInvalidates) {
         return durableInterestListIndexForUpdatesAsInvalidates;
       } else {
         return durableInterestListIndex;
       }
     } else {
       if (receiveUpdatesAsInvalidates) {
         return interestListIndexForUpdatesAsInvalidates;
       } else {
         return interestListIndex;
       }
     }
   }
   ```
   It filters Interest lists based on the durable flag. So in `recoverAllInterestTypes` function this would result that the non-durable Interests are recovered first, and then the durable (If durable client identity is configured).
   
   My solution was to simply follow this approach with CQs and to first register non-durable CQs, and then durable. I have used `CqQuery.isDurable() `flag since it is set with following API:
   
   `newCq(String queryString, CqAttributes cqAttr, **boolean isDurable**)`
   
   I think that the code you proposed would not register **non-durable** Interests, and would register only **durable** Interests in case durable client is configured. Additionally in case of durable client, it would register all CQs as **durable** (even those that are configured as non-durable). I will try to perform some test with the solution you proposed, since there are couple cases.
   
   BR/Jakov




----------------------------------------------------------------
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.

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