-
Notifications
You must be signed in to change notification settings - Fork 683
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-10106: Use queueConnection snapshot for if #7740
Conversation
geode-core/src/main/java/org/apache/geode/cache/client/internal/QueueManagerImpl.java
Outdated
Show resolved
Hide resolved
259ad01
to
5cd3e6e
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.
I would like to see more tests to cover all the branches in this method.
@@ -966,6 +965,16 @@ public void recoverPrimary(Set<ServerLocation> excludedServers) { | |||
} | |||
} | |||
|
|||
private boolean isPrimaryRecoveryNeeded(final ConnectionList queueConnectionList) { |
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.
If you make this static you could easily create unit tests that could test all the paths in this method.
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.
@pivotal-jbarrett done, all branches are covered now.
d294487
to
8f78eeb
Compare
@@ -966,6 +965,17 @@ public void recoverPrimary(Set<ServerLocation> excludedServers) { | |||
} | |||
} | |||
|
|||
@VisibleForTesting | |||
public static boolean isPrimaryRecoveryNeeded(final ConnectionList queueConnectionList) { |
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.
For unit tests in the same package this doesn't need to be public. Then you can also drop the annotation.
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.
done
* Using queueConnection local ref for multiple if checks * As it is a volatile variable, the value may become null mid checks.
8f78eeb
to
86a4472
Compare
* Using queueConnection local ref for multiple if checks * As it is a volatile variable, the value may become null mid checks. (cherry picked from commit b16dafa)
For all changes:
Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
Has your PR been rebased against the latest commit within the target branch (typically
develop
)?Is your initial contribution a single, squashed commit?
Does
gradlew build
run cleanly?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?