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 2022/01/11 21:17:25 UTC
[GitHub] [geode] mhansonp opened a new pull request #7258: Do not review GEODE-9704
mhansonp opened a new pull request #7258:
URL: https://github.com/apache/geode/pull/7258
Do not review.
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mhansonp commented on a change in pull request #7258: Do not review GEODE-9704
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #7258:
URL: https://github.com/apache/geode/pull/7258#discussion_r783502830
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/QueueManagerImpl.java
##########
@@ -926,6 +930,9 @@ private void recoverPrimary(Set<ServerLocation> excludedServers) {
excludedServers.add(newPrimary.getServer());
newPrimary = null;
}
+
+ markAsQueueAsReadyForEvents(newPrimary);
Review comment:
I wanted to see if I broke anything, I was not looking for feedback. I am not done with this bug. Thank you for the feedback though.
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mhansonp commented on a change in pull request #7258: GEODE-9704: Ready for events was happening before it should
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #7258:
URL: https://github.com/apache/geode/pull/7258#discussion_r821032939
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/QueueManagerImpl.java
##########
@@ -802,10 +803,13 @@ private QueueConnectionImpl createNewPrimary(Set<ServerLocation> excludedServers
excludedServers.addAll(servers);
}
+ return primary;
Review comment:
I am still working out the unit tests.
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mhansonp commented on a change in pull request #7258: Do not review GEODE-9704
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #7258:
URL: https://github.com/apache/geode/pull/7258#discussion_r783510389
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/QueueManagerImpl.java
##########
@@ -926,6 +930,9 @@ private void recoverPrimary(Set<ServerLocation> excludedServers) {
excludedServers.add(newPrimary.getServer());
newPrimary = null;
}
+
+ markAsQueueAsReadyForEvents(newPrimary);
Review comment:
I got the impression from the bug, that the ready for events should be after the register interest. I know I have seen us previously fix a test like that back in the days of the green team. As far as the clear of the local cache, I am still trying to figure out where that gets triggered.
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mhansonp commented on a change in pull request #7258: GEODE-9704: Ready for events was happening before it should
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #7258:
URL: https://github.com/apache/geode/pull/7258#discussion_r821031942
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/QueueManagerImpl.java
##########
@@ -802,10 +803,13 @@ private QueueConnectionImpl createNewPrimary(Set<ServerLocation> excludedServers
excludedServers.addAll(servers);
}
+ return primary;
Review comment:
@pivotal-amurmann Yes, I did. There are two calls and I have made sure that we are calling `markQueueAsReadyForEvents` after each one. So, it is covered.
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] pivotal-amurmann commented on a change in pull request #7258: GEODE-9704: Ready for events was happening before it should
Posted by GitBox <gi...@apache.org>.
pivotal-amurmann commented on a change in pull request #7258:
URL: https://github.com/apache/geode/pull/7258#discussion_r806089234
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/QueueManagerImpl.java
##########
@@ -802,10 +803,13 @@ private QueueConnectionImpl createNewPrimary(Set<ServerLocation> excludedServers
excludedServers.addAll(servers);
}
+ return primary;
Review comment:
@mhansonp were you able to find an answer to Jinmei's question?
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] pivotal-amurmann commented on a change in pull request #7258: GEODE-9704: Ready for events was happening before it should
Posted by GitBox <gi...@apache.org>.
pivotal-amurmann commented on a change in pull request #7258:
URL: https://github.com/apache/geode/pull/7258#discussion_r806089234
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/QueueManagerImpl.java
##########
@@ -802,10 +803,13 @@ private QueueConnectionImpl createNewPrimary(Set<ServerLocation> excludedServers
excludedServers.addAll(servers);
}
+ return primary;
Review comment:
@mhansonp were you able to find an answer to Jinmei's question?
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mhansonp commented on a change in pull request #7258: Do not review GEODE-9704
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #7258:
URL: https://github.com/apache/geode/pull/7258#discussion_r783526855
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/QueueManagerImpl.java
##########
@@ -926,6 +930,9 @@ private void recoverPrimary(Set<ServerLocation> excludedServers) {
excludedServers.add(newPrimary.getServer());
newPrimary = null;
}
+
+ markAsQueueAsReadyForEvents(newPrimary);
Review comment:
So I talked with Jinmei and I am satisfied that I have the correct ordering based on her understanding and mine. As far as a test goes, there is a test that is already in place, but it only fails periodically on my machine and not in the cloud. I will update that test with a couple of very minor changes, but I think that it (the test) is pretty much ready to go. There are more tests to run though beyond the one that reveals this issue.
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mhansonp closed pull request #7258: GEODE-9704: Ready for events was happening before it should
Posted by GitBox <gi...@apache.org>.
mhansonp closed pull request #7258:
URL: https://github.com/apache/geode/pull/7258
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mhansonp commented on pull request #7258: GEODE-9704: Ready for events was happening before it should
Posted by GitBox <gi...@apache.org>.
mhansonp commented on pull request #7258:
URL: https://github.com/apache/geode/pull/7258#issuecomment-1015661154
This PR is ready for review!
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #7258: GEODE-9704: Ready for events was happening before it should
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on a change in pull request #7258:
URL: https://github.com/apache/geode/pull/7258#discussion_r791075268
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/QueueManagerImpl.java
##########
@@ -802,10 +803,13 @@ private QueueConnectionImpl createNewPrimary(Set<ServerLocation> excludedServers
excludedServers.addAll(servers);
}
+ return primary;
Review comment:
My only concern is that after this change, `createNewPrimary()` method doesn't send `readyForEvent` to the server anymore. This method has two usages: `recoveryPrimary` and `initializeConnections`, `readyForEvent` will be sent by the first usage. As long as we know for sure it has no impact on the 2nd usage. Then this fix should be fine.
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #7258: GEODE-9704: Ready for events was happening before it should
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on a change in pull request #7258:
URL: https://github.com/apache/geode/pull/7258#discussion_r791075268
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/QueueManagerImpl.java
##########
@@ -802,10 +803,13 @@ private QueueConnectionImpl createNewPrimary(Set<ServerLocation> excludedServers
excludedServers.addAll(servers);
}
+ return primary;
Review comment:
My only concern is that after this change, `createNewPrimary()` method doesn't send `readyForEvent` to the server anymore. This method has two usages: `recoveryPrimary` and `initializeConnections`, `readyForEvent` will be sent by the first usage. As long as we know for sure it has no impact on the 2nd usage. Then this fix should be fine.
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] agingade commented on a change in pull request #7258: Do not review GEODE-9704
Posted by GitBox <gi...@apache.org>.
agingade commented on a change in pull request #7258:
URL: https://github.com/apache/geode/pull/7258#discussion_r783426191
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/QueueManagerImpl.java
##########
@@ -926,6 +930,9 @@ private void recoverPrimary(Set<ServerLocation> excludedServers) {
excludedServers.add(newPrimary.getServer());
newPrimary = null;
}
+
+ markAsQueueAsReadyForEvents(newPrimary);
Review comment:
The "readyForEvents" op, along with indicating client is ready and starting the dispatcher thread on the server; it also adds a Marker message; which dictates whether the events delivered from server is applied to local client cache. Any events that arrives before the marker message is not applied to local cache.
The sequence of action that should happen is:
- Get the primary connection
- Clear the local cache
- Send readyForEvent message (it adds the marker message behind the events in server side queue)
- Do interest registration (this gets those events before the marker as part of the response, so need to apply to local cache)
You need to have a test; where data is getting added to server cache; while readyForEvents and register-interest in progress; and then compare the entries in cliient and server cache/region.
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org