New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GEODE-9674: fix durable client message loss issue in tests. #6923
Conversation
jinmeiliao
commented
Sep 30, 2021
•
edited
edited
- do not try to dispatch residual messages when exception occurred.
- add more tests to verify peer connection, bulk operations and durable clients behavior
- add durable client flag when register interest
- move tests out of upgrade module if does not need older versions to test to avoid conflict in stress tests
14b8d18
to
f6e265c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it's getting there 👍
Region<Object, Object> region = clientCache | ||
.createClientRegionFactory(ClientRegionShortcut.CACHING_PROXY).create("region"); | ||
|
||
// use InterestResultPolicy.NONE to make sure the old queue is still around |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the durable
flag be mentioned here as well?
logger.warn("Client did not re-authenticate back successfully in " + elapsedTime | ||
+ "ms. Unregister this client proxy."); | ||
pauseOrUnregisterProxy(expired); | ||
synchronized (_stopDispatchingLock) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the bit that fixes the race condition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mostly by setting "exceptionOccured=true"
@@ -28,6 +31,7 @@ | |||
* make sure reset is called after each test to clean things up. | |||
*/ | |||
public class UpdatableUserAuthInitialize implements AuthInitialize { | |||
private static Logger logger = LogService.getLogger(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to be used anywhere? Should it stay around?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, should remove it.
@@ -422,7 +422,7 @@ protected void runDispatcher() { | |||
wait_for_re_auth_start_time = -1; | |||
} catch (NotAuthorizedException notAuthorized) { | |||
// behave as if the message is dispatched, remove from the queue | |||
logger.info("skip delivering message: " + clientMessage, notAuthorized); | |||
logger.warn("skip delivering message: " + clientMessage, notAuthorized); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the reason for changing log level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by a previous review comment when this was introduced. Thought I can address it here.
* do not try to dispatch residual messages when exception occurred. * add more tests to verify peer connection, bulk operations and durable clients behavior * add durable client flag when register interest * move tests out of upgrade module if does not need older versions to test to avoid conflict in stress tests