You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2020/07/09 05:50:36 UTC
[GitHub] [geode] jvarenina opened a new pull request #5360: GEODE-8329: Fix for durable CQ reqistration recovery
jvarenina opened a new pull request #5360:
URL: https://github.com/apache/geode/pull/5360
This change solves the issue when the client without configured HA is
wrongly re-registering durable CQs as non durable during the server
failover.
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [x] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [x] Is your initial contribution a single, squashed commit?
- [x] Does `gradlew build` run cleanly?
- [x] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
----------------------------------------------------------------
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
[GitHub] [geode] jvarenina commented on a change in pull request #5360: GEODE-8329: Fix for durable CQ reqistration recovery
Posted by GitBox <gi...@apache.org>.
jvarenina commented on a change in pull request #5360:
URL: https://github.com/apache/geode/pull/5360#discussion_r494922796
##########
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 ,
Sorry for bothering, but this request change hangs here for a long period of time. If you don't have time or my comments are not clear please don't hesitate to contact me, but I would be really grateful if we could decide how to proceed with this PR.
----------------------------------------------------------------
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
[GitHub] [geode] agingade commented on a change in pull request #5360: GEODE-8329: Fix for durable CQ reqistration recovery
Posted by GitBox <gi...@apache.org>.
agingade commented on a change in pull request #5360:
URL: https://github.com/apache/geode/pull/5360#discussion_r453128596
##########
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:
Thanks for pointing "isDurable" as argument to the CQ API. I missed it.
You are right, even though the client is durable the CQs and Interests doesn't have to be durable. Based on this condition either they need to be registered as durable or non-durable on durable client.
If you look into, how the recoverCQ is performed, it is done in one go. Its not like register non-durable CQs first and durable CQs next. Please point me if I missed the code path.
And looking at the code, looks like the durable value passed by user is ignored.
The code change I suggested with "recoverAllInterestTypes" looks right. With additional code change in:
```
private void recoverCqs(Connection recoveredConnection, boolean isDurable) {
-
-
// Change
cqi.createOn(recoveredConnection, isDurable);
// TO:
cqi.createOn(recoveredConnection, (isDurable && cqi.isDurable());
}
```
Also, you may want to look at the "CreateCQOp.execute()"
This is called when CQ is executed and while during redundancy-recovery. In case of execute, it seems to take the isDurable value set while creating the CQ into consideration not the client durability. Not sure if there are any additional mechanism in server side to know if its a durable client and only use the isDurable value passed during the CQ/interest creation.
----------------------------------------------------------------
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
[GitHub] [geode] agingade commented on a change in pull request #5360: GEODE-8329: Fix for durable CQ reqistration recovery
Posted by GitBox <gi...@apache.org>.
agingade commented on a change in pull request #5360:
URL: https://github.com/apache/geode/pull/5360#discussion_r452538829
##########
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:
The value for "isDurable" is passed by the caller. If you look into the only caller of this method; it calls the "recoverCQs" twice if the client is durable, with isDurable value as true. Not sure why its doing twice...
This method is also called while satisfying the redundancy-level, which is not related to client durability.
Say if the redundancy is set to 5 and there are only 3 servers available; when a new server is added to the cluster this code is executed to satisfy the redundancy. You could try adding test scenario for this.
Also, isDurable is the meta-info sent to server to say if its durable client (in this context).
In QueueManagerImpl can you try changing the following method:
``
private void recoverAllInterestTypes(final Connection recoveredConnection,
boolean isFirstNewConnection) {
if (PoolImpl.BEFORE_RECOVER_INTEREST_CALLBACK_FLAG) {
ClientServerObserver bo = ClientServerObserverHolder.getInstance();
bo.beforeInterestRecovery();
}
recoverInterestList(recoveredConnection, false, true, isFirstNewConnection);
recoverInterestList(recoveredConnection, false, false, isFirstNewConnection);
recoverCqs(recoveredConnection, false);
if (getPool().isDurableClient()) {
recoverInterestList(recoveredConnection, true, true, isFirstNewConnection);
recoverInterestList(recoveredConnection, true, false, isFirstNewConnection);
recoverCqs(recoveredConnection, true);
}
}
TO:
private void recoverAllInterestTypes(final Connection recoveredConnection,
boolean isFirstNewConnection) {
if (PoolImpl.BEFORE_RECOVER_INTEREST_CALLBACK_FLAG) {
ClientServerObserver bo = ClientServerObserverHolder.getInstance();
bo.beforeInterestRecovery();
}
boolean isDurableClient = getPool().isDurableClient();
recoverInterestList(recoveredConnection, isDurableClient, true, isFirstNewConnection);
recoverInterestList(recoveredConnection, isDurableClient, false, isFirstNewConnection);
recoverCqs(recoveredConnection, isDurableClient);
}
``
----------------------------------------------------------------
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
[GitHub] [geode] jvarenina commented on a change in pull request #5360: GEODE-8329: Fix for durable CQ reqistration recovery
Posted by GitBox <gi...@apache.org>.
jvarenina commented on a change in pull request #5360:
URL: https://github.com/apache/geode/pull/5360#discussion_r457926652
##########
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 ,
Have you had a time to check above comments?
----------------------------------------------------------------
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
[GitHub] [geode] agingade commented on a change in pull request #5360: GEODE-8329: Fix for durable CQ reqistration recovery
Posted by GitBox <gi...@apache.org>.
agingade commented on a change in pull request #5360:
URL: https://github.com/apache/geode/pull/5360#discussion_r452538829
##########
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:
The value for "isDurable" is passed by the caller. If you look into the only caller of this method; it calls the "recoverCQs" twice if the client is durable, with isDurable value as true. Not sure why its doing twice...
This method is also called while satisfying the redundancy-level, which is not related to client durability.
Say if the redundancy is set to 5 and there are only 3 servers available; when a new server is added to the cluster this code is executed to satisfy the redundancy. You could try adding test scenario for this.
Also, isDurable is the meta-info sent to server to say if its durable client (in this context).
----------------------------------------------------------------
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
[GitHub] [geode] kohlmu-pivotal commented on pull request #5360: GEODE-8329: Fix for durable CQ reqistration recovery
Posted by GitBox <gi...@apache.org>.
kohlmu-pivotal commented on pull request #5360:
URL: https://github.com/apache/geode/pull/5360#issuecomment-720804056
@agingade do you have further comments on this PR?
----------------------------------------------------------------
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
[GitHub] [geode] jvarenina commented on a change in pull request #5360: GEODE-8329: Fix for durable CQ reqistration recovery
Posted by 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
[GitHub] [geode] agingade commented on a change in pull request #5360: GEODE-8329: Fix for durable CQ reqistration recovery
Posted by GitBox <gi...@apache.org>.
agingade commented on a change in pull request #5360:
URL: https://github.com/apache/geode/pull/5360#discussion_r452538829
##########
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:
The value for "isDurable" is passed by the caller. If you look into the only caller of this method; it calls the "recoverCQs" twice if the client is durable, with isDurable value as true. Not sure why its doing twice...
This method is also called while satisfying the redundancy-level, which is not related to client durability.
Say if the redundancy is set to 5 and there are only 3 servers available; when a new server is added to the cluster this code is executed to satisfy the redundancy. You could try adding test scenario for this.
Also, isDurable is the meta-info sent to server to say if its durable client (in this context).
In QueueManagerImpl can you try changing the following method:
` private void recoverAllInterestTypes(final Connection recoveredConnection,
boolean isFirstNewConnection) {
if (PoolImpl.BEFORE_RECOVER_INTEREST_CALLBACK_FLAG) {
ClientServerObserver bo = ClientServerObserverHolder.getInstance();
bo.beforeInterestRecovery();
}
recoverInterestList(recoveredConnection, false, true, isFirstNewConnection);
recoverInterestList(recoveredConnection, false, false, isFirstNewConnection);
recoverCqs(recoveredConnection, false);
if (getPool().isDurableClient()) {
recoverInterestList(recoveredConnection, true, true, isFirstNewConnection);
recoverInterestList(recoveredConnection, true, false, isFirstNewConnection);
recoverCqs(recoveredConnection, true);
}
}
TO:
private void recoverAllInterestTypes(final Connection recoveredConnection,
boolean isFirstNewConnection) {
if (PoolImpl.BEFORE_RECOVER_INTEREST_CALLBACK_FLAG) {
ClientServerObserver bo = ClientServerObserverHolder.getInstance();
bo.beforeInterestRecovery();
}
boolean isDurableClient = getPool().isDurableClient();
recoverInterestList(recoveredConnection, isDurableClient, true, isFirstNewConnection);
recoverInterestList(recoveredConnection, isDurableClient, false, isFirstNewConnection);
recoverCqs(recoveredConnection, isDurableClient);
}`
----------------------------------------------------------------
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
[GitHub] [geode] jvarenina commented on a change in pull request #5360: GEODE-8329: Fix for durable CQ reqistration recovery
Posted by GitBox <gi...@apache.org>.
jvarenina commented on a change in pull request #5360:
URL: https://github.com/apache/geode/pull/5360#discussion_r453607399
##########
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:
Thanks for your comments!
Related to the Interest recovery, I have tried following case:
```
Start three servers. Start client with following config:
- redundnacy set to 0
- register non-durable Interests
- configure durable id
```
After I shutdown primary server I expected that the client should register/recover Interests on the another running server. I have tried exactly same case with and without the code you suggested. What I have noticed that some steps are missing related to the recovery of non-durable Interest when using solution you suggested (please check logs below). Is this expected?
without your code:
```
[debug 2020/07/13 13:56:37.574 CEST <queueTimer-HAInterestBaseTestPool1> tid=0x5a] SubscriptionManager redundancy satisfier - Non backup server was made primary. Recovering interest jakov:26486
[info 2020/07/13 13:56:37.574 CEST <Cache Client Updater Thread on 192.168.1.101(12538)<v4>:41002 port 26486> tid=0x5b] Cache Client Updater Thread on 192.168.1.101(12538)<v4>:41002 port 26486 (jakov:26486) : ready to process messages.
[debug 2020/07/13 13:56:37.576 CEST <queueTimer-HAInterestBaseTestPool1> tid=0x5a] org.apache.geode.cache.client.internal.QueueManagerImpl@69610f07.recoverSingleRegion starting kind=KEY region=/HAInterestBaseTest_region: {k1=KEYS, k2=KEYS}
[debug 2020/07/13 13:56:37.576 CEST <queueTimer-HAInterestBaseTestPool1> tid=0x5a] registerInterestsStarted: new count = 1
[debug 2020/07/13 13:56:37.578 CEST <queueTimer-HAInterestBaseTestPool1> tid=0x5a] localDestroyNoCallbacks key=k2
[debug 2020/07/13 13:56:37.579 CEST <queueTimer-HAInterestBaseTestPool1> tid=0x5a] basicDestroyPart2: k2, version=null
[debug 2020/07/13 13:56:37.580 CEST <queueTimer-HAInterestBaseTestPool1> tid=0x5a] VersionedThinRegionEntryHeapStringKey1@1f47aafa (key=k2; rawValue=REMOVED_PHASE1; version={v1; rv2; mbr=192.168.1.101(12538)<v4>:41002; time=1594641397170};member=192.168.1.101(12538)<v4>:41002) dispatching event EntryEventImpl[op=LOCAL_DESTROY;region=/HAInterestBaseTest_region;key=k2;callbackArg=null;originRemote=false;originMember=jakov(12395:loner):57906:02b10848]
[debug 2020/07/13 13:56:37.580 CEST <queueTimer-HAInterestBaseTestPool1> tid=0x5a] localDestroyNoCallbacks key=k1
[debug 2020/07/13 13:56:37.580 CEST <queueTimer-HAInterestBaseTestPool1> tid=0x5a] basicDestroyPart2: k1, version=null
[debug 2020/07/13 13:56:37.580 CEST <queueTimer-HAInterestBaseTestPool1> tid=0x5a] VersionedThinRegionEntryHeapStringKey1@5ecf6c9c (key=k1; rawValue=REMOVED_PHASE1; version={v1; rv1; mbr=192.168.1.101(12538)<v4>:41002; time=1594641397148};member=192.168.1.101(12538)<v4>:41002) dispatching event EntryEventImpl[op=LOCAL_DESTROY;region=/HAInterestBaseTest_region;key=k1;callbackArg=null;originRemote=false;originMember=jakov(12395:loner):57906:02b10848]
[debug 2020/07/13 13:56:37.580 CEST <queueTimer-HAInterestBaseTestPool1> tid=0x5a] org.apache.geode.cache.client.internal.QueueManagerImpl@69610f07.recoverSingleRegion :Endpoint recovered is primary so clearing the keys of interest starting kind=KEY region=/HAInterestBaseTest_region: [k1, k2]
[debug 2020/07/13 13:56:37.584 CEST <queueTimer-HAInterestBaseTestPool1> tid=0x5a] org.apache.geode.internal.cache.LocalRegion[path='/HAInterestBaseTest_region';scope=LOCAL';dataPolicy=NORMAL; concurrencyChecksEnabled] refreshEntriesFromServerKeys count=2 policy=KEYS
k1
k2
[debug 2020/07/13 13:56:37.584 CEST <queueTimer-HAInterestBaseTestPool1> tid=0x5a] refreshEntries region=/HAInterestBaseTest_region
[debug 2020/07/13 13:56:37.585 CEST <queueTimer-HAInterestBaseTestPool1> tid=0x5a] registerInterestCompleted: new value = 0
[debug 2020/07/13 13:56:37.585 CEST <queueTimer-HAInterestBaseTestPool1> tid=0x5a] registerInterestCompleted: Signalling end of register-interest
[debug 2020/07/13 13:56:37.586 CEST <queueTimer-HAInterestBaseTestPool1> tid=0x5a] Primary recovery not needed
```
with your code:
```
[debug 2020/07/13 13:44:20.028 CEST <queueTimer-HAInterestBaseTestPool1> tid=0x5a] SubscriptionManager redundancy satisfier - Non backup server was made primary. Recovering interest jakov:28101
[info 2020/07/13 13:44:20.028 CEST <Cache Client Updater Thread on 192.168.1.101(11053)<v4>:41002 port 28101> tid=0x5b] Cache Client Updater Thread on 192.168.1.101(11053)<v4>:41002 port 28101 (jakov:28101) : ready to process messages.
[debug 2020/07/13 13:44:20.030 CEST <queueTimer-HAInterestBaseTestPool1> tid=0x5a] Primary recovery not needed
```
----------------------------------------------------------------
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
[GitHub] [geode] jvarenina commented on a change in pull request #5360: GEODE-8329: Fix for durable CQ reqistration recovery
Posted by GitBox <gi...@apache.org>.
jvarenina commented on a change in pull request #5360:
URL: https://github.com/apache/geode/pull/5360#discussion_r453489804
##########
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:
I have tested TC with redundancy configured and it seems that the recovery of CQs is done a differently in this case. The remaining server sends within `InitialImageOperation$FilterInfoMessage` all releveant CQs information to the starting server. At the reception of the message the starting server then registers CQs as durable (so no problem in this case observed).
**Primary server:**
```
[debug 2020/07/10 13:30:54.916 CEST <P2P message reader for 192.168.1.102(server3:31347)<v4>:41001 shared unordered uid=1 local port=53683 remote port=45674> tid=0x57] Received message 'InitialImageOperation$RequestFilterInfoMessage(region path='/_gfe_durable_client_with_id_AppCounters_1_queue'; sender=192.168.1.102(server3:31347)<v4>:41001; processorId=27)' from <192.168.1.102(server3:31347)<v4>:41001>
```
**Starting server:**
```
[debug 2020/07/10 13:30:54.916 CEST <Client Queue Initialization Thread 1> tid=0x48] Sending (InitialImageOperation$RequestFilterInfoMessage(region path='/_gfe_durable_client_with_id_AppCounters_1_queue'; sender=192.168.1.102(server3:31347)<v4>:41001; processorId=27)) to 1 peers ([192.168.1.102(server1:30862)<v1>:41000]) via tcp/ip
[debug 2020/07/10 13:30:54.918 CEST <P2P message reader for 192.168.1.102(server1:30862)<v1>:41000 shared unordered uid=5 local port=52175 remote port=46552> tid=0x30] Received message 'InitialImageOperation$FilterInfoMessage processorId=27 from 192.168.1.102(server1:30862)<v1>:41000; NON_DURABLE allKeys=0; allKeysInv=0; keysOfInterest=0; keysOfInterestInv=0; patternsOfInterest=0; patternsOfInterestInv=0; filtersOfInterest=0; filtersOfInterestInv=0; DURABLE allKeys=0; allKeysInv=0; keysOfInterest=0; keysOfInterestInv=0; patternsOfInterest=0; patternsOfInterestInv=0; filtersOfInterest=0; filtersOfInterestInv=0; cqs=1' from <192.168.1.102(server1:30862)<v1>:41000>
[debug 2020/07/10 13:30:54.919 CEST <Pooled High Priority Message Processor 3> tid=0x3d] Processing FilterInfo for proxy: CacheClientProxy[identity(192.168.1.102(31226:loner):45576:8b927d38,connection=1,durableAttributes=DurableClientAttributes[id=AppCounters; timeout=200]); port=57552; primary=false; version=GEODE 1.12.0] : InitialImageOperation$FilterInfoMessage processorId=27 from 192.168.1.102(server1:30862)<v1>:41000; NON_DURABLE allKeys=0; allKeysInv=0; keysOfInterest=0; keysOfInterestInv=0; patternsOfInterest=0; patternsOfInterestInv=0; filtersOfInterest=0; filtersOfInterestInv=0; DURABLE allKeys=0; allKeysInv=0; keysOfInterest=0; keysOfInterestInv=0; patternsOfInterest=0; patternsOfInterestInv=0; filtersOfInterest=0; filtersOfInterestInv=0; cqs=1
[debug 2020/07/10 13:30:54.944 CEST <Pooled High Priority Message Processor 3> tid=0x3d] Server side query for the cq: randomTracker is: SELECT * FROM /example-region i where i > 70
[debug 2020/07/10 13:30:54.944 CEST <Pooled High Priority Message Processor 3> tid=0x3d] Added CQ to the base region: /example-region With key as: randomTracker__AppCounters
[debug 2020/07/10 13:30:54.944 CEST <Pooled High Priority Message Processor 3> tid=0x3d] Adding CQ into MatchingCQ map, CQName: randomTracker__AppCounters Number of matched querys are: 1
[debug 2020/07/10 13:30:54.945 CEST <Pooled High Priority Message Processor 3> tid=0x3d] Adding to CQ Repository. CqName : randomTracker ServerCqName : randomTracker__AppCounters
[debug 2020/07/10 13:30:54.945 CEST <Pooled High Priority Message Processor 3> tid=0x3d] Adding CQ randomTracker__AppCounters to this members FilterProfile.
[debug 2020/07/10 13:30:54.945 CEST <Pooled High Priority Message Processor 3> tid=0x3d] Successfully created CQ on the server. CqName : randomTracker
```
I can attach full logs if you need. Also, I have found the the following comment in the client code:
```
// Even though the new redundant queue will usually recover
// subscription information (see bug #39014) from its initial
// image provider, in bug #42280 we found that this is not always
// the case, so clients must always register interest with the new
// redundant server.
if (recoverInterest) {
recoverInterest(queueConnection, isFirstNewConnection);
}
```
It is stated here the there is possible case when redundant queue isn't recovered by `InitialImageOperation$FilterInfoMessage`, but I haven't been able to reproduce that case. Do you see any benefit in finding and creating TC for this scenario, since recovery of durable CQ is already tested with TC without redundancy?
----------------------------------------------------------------
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
[GitHub] [geode] jvarenina commented on a change in pull request #5360: GEODE-8329: Fix for durable CQ reqistration recovery
Posted by 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 it will take some time.
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
[GitHub] [geode] jvarenina commented on a change in pull request #5360: GEODE-8329: Fix for durable CQ reqistration recovery
Posted by GitBox <gi...@apache.org>.
jvarenina commented on a change in pull request #5360:
URL: https://github.com/apache/geode/pull/5360#discussion_r453489804
##########
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:
I have tested TC with redundancy configured and it seems that the recovery of CQs is done a differently in this case. The primary server sends within `InitialImageOperation$FilterInfoMessage` all releveant CQs information to the starting server. At the reception of the message the starting server then registers CQs as durable (so no problem in this case observed).
**Primary server:**
```
[debug 2020/07/10 13:30:54.916 CEST <P2P message reader for 192.168.1.102(server3:31347)<v4>:41001 shared unordered uid=1 local port=53683 remote port=45674> tid=0x57] Received message 'InitialImageOperation$RequestFilterInfoMessage(region path='/_gfe_durable_client_with_id_AppCounters_1_queue'; sender=192.168.1.102(server3:31347)<v4>:41001; processorId=27)' from <192.168.1.102(server3:31347)<v4>:41001>
```
**Starting server:**
```
[debug 2020/07/10 13:30:54.916 CEST <Client Queue Initialization Thread 1> tid=0x48] Sending (InitialImageOperation$RequestFilterInfoMessage(region path='/_gfe_durable_client_with_id_AppCounters_1_queue'; sender=192.168.1.102(server3:31347)<v4>:41001; processorId=27)) to 1 peers ([192.168.1.102(server1:30862)<v1>:41000]) via tcp/ip
[debug 2020/07/10 13:30:54.918 CEST <P2P message reader for 192.168.1.102(server1:30862)<v1>:41000 shared unordered uid=5 local port=52175 remote port=46552> tid=0x30] Received message 'InitialImageOperation$FilterInfoMessage processorId=27 from 192.168.1.102(server1:30862)<v1>:41000; NON_DURABLE allKeys=0; allKeysInv=0; keysOfInterest=0; keysOfInterestInv=0; patternsOfInterest=0; patternsOfInterestInv=0; filtersOfInterest=0; filtersOfInterestInv=0; DURABLE allKeys=0; allKeysInv=0; keysOfInterest=0; keysOfInterestInv=0; patternsOfInterest=0; patternsOfInterestInv=0; filtersOfInterest=0; filtersOfInterestInv=0; cqs=1' from <192.168.1.102(server1:30862)<v1>:41000>
[debug 2020/07/10 13:30:54.919 CEST <Pooled High Priority Message Processor 3> tid=0x3d] Processing FilterInfo for proxy: CacheClientProxy[identity(192.168.1.102(31226:loner):45576:8b927d38,connection=1,durableAttributes=DurableClientAttributes[id=AppCounters; timeout=200]); port=57552; primary=false; version=GEODE 1.12.0] : InitialImageOperation$FilterInfoMessage processorId=27 from 192.168.1.102(server1:30862)<v1>:41000; NON_DURABLE allKeys=0; allKeysInv=0; keysOfInterest=0; keysOfInterestInv=0; patternsOfInterest=0; patternsOfInterestInv=0; filtersOfInterest=0; filtersOfInterestInv=0; DURABLE allKeys=0; allKeysInv=0; keysOfInterest=0; keysOfInterestInv=0; patternsOfInterest=0; patternsOfInterestInv=0; filtersOfInterest=0; filtersOfInterestInv=0; cqs=1
[debug 2020/07/10 13:30:54.944 CEST <Pooled High Priority Message Processor 3> tid=0x3d] Server side query for the cq: randomTracker is: SELECT * FROM /example-region i where i > 70
[debug 2020/07/10 13:30:54.944 CEST <Pooled High Priority Message Processor 3> tid=0x3d] Added CQ to the base region: /example-region With key as: randomTracker__AppCounters
[debug 2020/07/10 13:30:54.944 CEST <Pooled High Priority Message Processor 3> tid=0x3d] Adding CQ into MatchingCQ map, CQName: randomTracker__AppCounters Number of matched querys are: 1
[debug 2020/07/10 13:30:54.945 CEST <Pooled High Priority Message Processor 3> tid=0x3d] Adding to CQ Repository. CqName : randomTracker ServerCqName : randomTracker__AppCounters
[debug 2020/07/10 13:30:54.945 CEST <Pooled High Priority Message Processor 3> tid=0x3d] Adding CQ randomTracker__AppCounters to this members FilterProfile.
[debug 2020/07/10 13:30:54.945 CEST <Pooled High Priority Message Processor 3> tid=0x3d] Successfully created CQ on the server. CqName : randomTracker
```
I can attach full logs if you need. Also, I have found the the following comment in the client code:
```
// Even though the new redundant queue will usually recover
// subscription information (see bug #39014) from its initial
// image provider, in bug #42280 we found that this is not always
// the case, so clients must always register interest with the new
// redundant server.
if (recoverInterest) {
recoverInterest(queueConnection, isFirstNewConnection);
}
```
It is stated here the there is possible case when redundant queue isn't recovered by `InitialImageOperation$FilterInfoMessage`, but I haven't been able to reproduce that case. Do you see any benefit in finding and creating TC for this scenario, since recovery of durable CQ is already tested with TC without redundancy?
----------------------------------------------------------------
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
[GitHub] [geode] agingade commented on a change in pull request #5360: GEODE-8329: Fix for durable CQ reqistration recovery
Posted by GitBox <gi...@apache.org>.
agingade commented on a change in pull request #5360:
URL: https://github.com/apache/geode/pull/5360#discussion_r452538829
##########
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:
The value for "isDurable" is passed by the caller. If you look into the only caller of this method; it calls the "recoverCQs" twice if the client is durable, with isDurable value as true. Not sure why its doing twice...
This method is also called while satisfying the redundancy-level, which is not related to client durability.
Say if the redundancy is set to 5 and there are only 3 servers available; when a new server is added to the cluster this code is executed to satisfy the redundancy. You could try adding test scenario for this.
Also, isDurable is the meta-info sent to server to say if its durable client (in this context).
In QueueManagerImpl can you try changing the following method:
private void recoverAllInterestTypes(final Connection recoveredConnection,
boolean isFirstNewConnection) {
if (PoolImpl.BEFORE_RECOVER_INTEREST_CALLBACK_FLAG) {
ClientServerObserver bo = ClientServerObserverHolder.getInstance();
bo.beforeInterestRecovery();
}
recoverInterestList(recoveredConnection, false, true, isFirstNewConnection);
recoverInterestList(recoveredConnection, false, false, isFirstNewConnection);
recoverCqs(recoveredConnection, false);
if (getPool().isDurableClient()) {
recoverInterestList(recoveredConnection, true, true, isFirstNewConnection);
recoverInterestList(recoveredConnection, true, false, isFirstNewConnection);
recoverCqs(recoveredConnection, true);
}
}
TO:
private void recoverAllInterestTypes(final Connection recoveredConnection,
boolean isFirstNewConnection) {
if (PoolImpl.BEFORE_RECOVER_INTEREST_CALLBACK_FLAG) {
ClientServerObserver bo = ClientServerObserverHolder.getInstance();
bo.beforeInterestRecovery();
}
boolean isDurableClient = getPool().isDurableClient();
recoverInterestList(recoveredConnection, isDurableClient, true, isFirstNewConnection);
recoverInterestList(recoveredConnection, isDurableClient, false, isFirstNewConnection);
recoverCqs(recoveredConnection, isDurableClient);
}
----------------------------------------------------------------
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
[GitHub] [geode] mkevo commented on pull request #5360: GEODE-8329: Fix for durable CQ reqistration recovery
Posted by GitBox <gi...@apache.org>.
mkevo commented on pull request #5360:
URL: https://github.com/apache/geode/pull/5360#issuecomment-737718555
Hi @agingade ,
If you have any concerns please write them by the end of the week, if not we will go with merging 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] agingade commented on a change in pull request #5360: GEODE-8329: Fix for durable CQ reqistration recovery
Posted by GitBox <gi...@apache.org>.
agingade commented on a change in pull request #5360:
URL: https://github.com/apache/geode/pull/5360#discussion_r452538829
##########
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:
The value for "isDurable" is passed by the caller. If you look into the only caller of this method; it calls the "recoverCQs" twice if the client is durable, with isDurable value as true. Not sure why its doing twice...
This method is also called while satisfying the redundancy-level, which is not related to client durability.
Say if the redundancy is set to 5 and there are only 3 servers available; when a new server is added to the cluster this code is executed to satisfy the redundancy. You could try adding test scenario for this.
Also, isDurable is the meta-info sent to server to say if its durable client (in this context).
In QueueManagerImpl can you try changing the following method:
` private void recoverAllInterestTypes(final Connection recoveredConnection,
boolean isFirstNewConnection) {
if (PoolImpl.BEFORE_RECOVER_INTEREST_CALLBACK_FLAG) {
ClientServerObserver bo = ClientServerObserverHolder.getInstance();
bo.beforeInterestRecovery();
}
recoverInterestList(recoveredConnection, false, true, isFirstNewConnection);
recoverInterestList(recoveredConnection, false, false, isFirstNewConnection);
recoverCqs(recoveredConnection, false);
if (getPool().isDurableClient()) {
recoverInterestList(recoveredConnection, true, true, isFirstNewConnection);
recoverInterestList(recoveredConnection, true, false, isFirstNewConnection);
recoverCqs(recoveredConnection, true);
}
}
TO:
private void recoverAllInterestTypes(final Connection recoveredConnection,
boolean isFirstNewConnection) {
if (PoolImpl.BEFORE_RECOVER_INTEREST_CALLBACK_FLAG) {
ClientServerObserver bo = ClientServerObserverHolder.getInstance();
bo.beforeInterestRecovery();
}
boolean isDurableClient = getPool().isDurableClient();
recoverInterestList(recoveredConnection, isDurableClient, true, isFirstNewConnection);
recoverInterestList(recoveredConnection, isDurableClient, false, isFirstNewConnection);
recoverCqs(recoveredConnection, isDurableClient);
}
`
----------------------------------------------------------------
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
[GitHub] [geode] mkevo merged pull request #5360: GEODE-8329: Fix for durable CQ reqistration recovery
Posted by GitBox <gi...@apache.org>.
mkevo merged pull request #5360:
URL: https://github.com/apache/geode/pull/5360
----------------------------------------------------------------
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
[GitHub] [geode] jvarenina commented on a change in pull request #5360: GEODE-8329: Fix for durable CQ reqistration recovery
Posted by GitBox <gi...@apache.org>.
jvarenina commented on a change in pull request #5360:
URL: https://github.com/apache/geode/pull/5360#discussion_r494922796
##########
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 ,
Sorry for bothering, but this request change hangs here for a long period of time. If you don't have time or my comments are not clear please don't hesitate to contact me, but I would be really grateful if we could decide how to proceed with this PR.
----------------------------------------------------------------
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
[GitHub] [geode] agingade commented on a change in pull request #5360: GEODE-8329: Fix for durable CQ reqistration recovery
Posted by GitBox <gi...@apache.org>.
agingade commented on a change in pull request #5360:
URL: https://github.com/apache/geode/pull/5360#discussion_r452538829
##########
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:
The value for "isDurable" is passed by the caller. If you look into the only caller of this method; it calls the "recoverCQs" twice if the client is durable, with isDurable value as false.
This method is also called while satisfying the redundancy-level, which is not related to client durability.
Say if the redundancy is set to 5 and there are only 3 servers available; when a new server is added to the cluster this code is executed to satisfy the redundancy.
Also, isDurable is the meta-info sent to server to say if its durable client (in this context).
----------------------------------------------------------------
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
[GitHub] [geode] agingade commented on a change in pull request #5360: GEODE-8329: Fix for durable CQ reqistration recovery
Posted by GitBox <gi...@apache.org>.
agingade commented on a change in pull request #5360:
URL: https://github.com/apache/geode/pull/5360#discussion_r452538829
##########
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:
The value for "isDurable" is passed by the caller. If you look into the only caller of this method; it calls the "recoverCQs" twice if the client is durable, with isDurable value as true. Not sure why its doing twice...
This method is also called while satisfying the redundancy-level, which is not related to client durability.
Say if the redundancy is set to 5 and there are only 3 servers available; when a new server is added to the cluster this code is executed to satisfy the redundancy. You could try adding test scenario for this.
Also, isDurable is the meta-info sent to server to say if its durable client (in this context).
In QueueManagerImpl can you try changing the following method:
```
private void recoverAllInterestTypes(final Connection recoveredConnection,
boolean isFirstNewConnection) {
if (PoolImpl.BEFORE_RECOVER_INTEREST_CALLBACK_FLAG) {
ClientServerObserver bo = ClientServerObserverHolder.getInstance();
bo.beforeInterestRecovery();
}
recoverInterestList(recoveredConnection, false, true, isFirstNewConnection);
recoverInterestList(recoveredConnection, false, false, isFirstNewConnection);
recoverCqs(recoveredConnection, false);
if (getPool().isDurableClient()) {
recoverInterestList(recoveredConnection, true, true, isFirstNewConnection);
recoverInterestList(recoveredConnection, true, false, isFirstNewConnection);
recoverCqs(recoveredConnection, true);
}
}
TO
private void recoverAllInterestTypes(final Connection recoveredConnection,
boolean isFirstNewConnection) {
if (PoolImpl.BEFORE_RECOVER_INTEREST_CALLBACK_FLAG) {
ClientServerObserver bo = ClientServerObserverHolder.getInstance();
bo.beforeInterestRecovery();
}
boolean isDurableClient = getPool().isDurableClient();
recoverInterestList(recoveredConnection, isDurableClient, true, isFirstNewConnection);
recoverInterestList(recoveredConnection, isDurableClient, false, isFirstNewConnection);
recoverCqs(recoveredConnection, isDurableClient);
}
```
----------------------------------------------------------------
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
[GitHub] [geode] jvarenina commented on a change in pull request #5360: GEODE-8329: Fix for durable CQ reqistration recovery
Posted by 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 it will take some time.
Related to your comment about redundancy, I have already tested with configured redundancy and didn't observe any problem in that case. I will take a look at that case in more details, since it seems that the recovery of CQs and Interests is performed differently than when redundancy is not configured for subscriptions event queues.
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
[GitHub] [geode] agingade commented on a change in pull request #5360: GEODE-8329: Fix for durable CQ reqistration recovery
Posted by GitBox <gi...@apache.org>.
agingade commented on a change in pull request #5360:
URL: https://github.com/apache/geode/pull/5360#discussion_r452538829
##########
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:
The value for "isDurable" is passed by the caller. If you look into the only caller of this method; it calls the "recoverCQs" twice if the client is durable, with isDurable value as true. Not sure why its doing twice...
This method is also called while satisfying the redundancy-level, which is not related to client durability.
Say if the redundancy is set to 5 and there are only 3 servers available; when a new server is added to the cluster this code is executed to satisfy the redundancy. You could try adding test scenario for this.
Also, isDurable is the meta-info sent to server to say if its durable client (in this context).
In QueueManagerImpl can you try changing the following method:
`` private void recoverAllInterestTypes(final Connection recoveredConnection,
boolean isFirstNewConnection) {
if (PoolImpl.BEFORE_RECOVER_INTEREST_CALLBACK_FLAG) {
ClientServerObserver bo = ClientServerObserverHolder.getInstance();
bo.beforeInterestRecovery();
}
recoverInterestList(recoveredConnection, false, true, isFirstNewConnection);
recoverInterestList(recoveredConnection, false, false, isFirstNewConnection);
recoverCqs(recoveredConnection, false);
if (getPool().isDurableClient()) {
recoverInterestList(recoveredConnection, true, true, isFirstNewConnection);
recoverInterestList(recoveredConnection, true, false, isFirstNewConnection);
recoverCqs(recoveredConnection, true);
}
}
TO:
private void recoverAllInterestTypes(final Connection recoveredConnection,
boolean isFirstNewConnection) {
if (PoolImpl.BEFORE_RECOVER_INTEREST_CALLBACK_FLAG) {
ClientServerObserver bo = ClientServerObserverHolder.getInstance();
bo.beforeInterestRecovery();
}
boolean isDurableClient = getPool().isDurableClient();
recoverInterestList(recoveredConnection, isDurableClient, true, isFirstNewConnection);
recoverInterestList(recoveredConnection, isDurableClient, false, isFirstNewConnection);
recoverCqs(recoveredConnection, isDurableClient);
}``
----------------------------------------------------------------
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
[GitHub] [geode] agingade commented on a change in pull request #5360: GEODE-8329: Fix for durable CQ reqistration recovery
Posted by GitBox <gi...@apache.org>.
agingade commented on a change in pull request #5360:
URL: https://github.com/apache/geode/pull/5360#discussion_r452538829
##########
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:
The value for "isDurable" is passed by the caller. If you look into the only caller of this method; it calls the "recoverCQs" twice if the client is durable, with isDurable value as true. Not sure why its doing twice...
This method is also called while satisfying the redundancy-level, which is not related to client durability.
Say if the redundancy is set to 5 and there are only 3 servers available; when a new server is added to the cluster this code is executed to satisfy the redundancy. You could try adding test scenario for this.
Also, isDurable is the meta-info sent to server to say if its durable client (in this context).
In QueueManagerImpl can you try changing the following method:
``
private void recoverAllInterestTypes(final Connection recoveredConnection,
boolean isFirstNewConnection) {
if (PoolImpl.BEFORE_RECOVER_INTEREST_CALLBACK_FLAG) {
ClientServerObserver bo = ClientServerObserverHolder.getInstance();
bo.beforeInterestRecovery();
}
recoverInterestList(recoveredConnection, false, true, isFirstNewConnection);
recoverInterestList(recoveredConnection, false, false, isFirstNewConnection);
recoverCqs(recoveredConnection, false);
if (getPool().isDurableClient()) {
recoverInterestList(recoveredConnection, true, true, isFirstNewConnection);
recoverInterestList(recoveredConnection, true, false, isFirstNewConnection);
recoverCqs(recoveredConnection, true);
}
}
TO
private void recoverAllInterestTypes(final Connection recoveredConnection,
boolean isFirstNewConnection) {
if (PoolImpl.BEFORE_RECOVER_INTEREST_CALLBACK_FLAG) {
ClientServerObserver bo = ClientServerObserverHolder.getInstance();
bo.beforeInterestRecovery();
}
boolean isDurableClient = getPool().isDurableClient();
recoverInterestList(recoveredConnection, isDurableClient, true, isFirstNewConnection);
recoverInterestList(recoveredConnection, isDurableClient, false, isFirstNewConnection);
recoverCqs(recoveredConnection, isDurableClient);
}
``
----------------------------------------------------------------
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
[GitHub] [geode] agingade commented on a change in pull request #5360: GEODE-8329: Fix for durable CQ reqistration recovery
Posted by GitBox <gi...@apache.org>.
agingade commented on a change in pull request #5360:
URL: https://github.com/apache/geode/pull/5360#discussion_r452538829
##########
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:
The value for "isDurable" is passed by the caller. If you look into the only caller of this method; it calls the "recoverCQs" twice if the client is durable, with isDurable value as true. Not sure why its doing twice...
This method is also called while satisfying the redundancy-level, which is not related to client durability.
Say if the redundancy is set to 5 and there are only 3 servers available; when a new server is added to the cluster this code is executed to satisfy the redundancy. You could try adding test scenario for this.
Also, isDurable is the meta-info sent to server to say if its durable client (in this context).
In QueueManagerImpl can you try changing the following method:
`
private void recoverAllInterestTypes(final Connection recoveredConnection,
boolean isFirstNewConnection) {
if (PoolImpl.BEFORE_RECOVER_INTEREST_CALLBACK_FLAG) {
ClientServerObserver bo = ClientServerObserverHolder.getInstance();
bo.beforeInterestRecovery();
}
recoverInterestList(recoveredConnection, false, true, isFirstNewConnection);
recoverInterestList(recoveredConnection, false, false, isFirstNewConnection);
recoverCqs(recoveredConnection, false);
if (getPool().isDurableClient()) {
recoverInterestList(recoveredConnection, true, true, isFirstNewConnection);
recoverInterestList(recoveredConnection, true, false, isFirstNewConnection);
recoverCqs(recoveredConnection, true);
}
}
TO:
private void recoverAllInterestTypes(final Connection recoveredConnection,
boolean isFirstNewConnection) {
if (PoolImpl.BEFORE_RECOVER_INTEREST_CALLBACK_FLAG) {
ClientServerObserver bo = ClientServerObserverHolder.getInstance();
bo.beforeInterestRecovery();
}
boolean isDurableClient = getPool().isDurableClient();
recoverInterestList(recoveredConnection, isDurableClient, true, isFirstNewConnection);
recoverInterestList(recoveredConnection, isDurableClient, false, isFirstNewConnection);
recoverCqs(recoveredConnection, isDurableClient);
}
`
----------------------------------------------------------------
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