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/04/07 17:39:35 UTC
[GitHub] [geode] DonalEvans opened a new pull request, #7569: GEODE-10222: Handle null memberID in GII
DonalEvans opened a new pull request, #7569:
URL: https://github.com/apache/geode/pull/7569
- Change the exception thrown by DiskVersionTag.replaceNullIDs() from
AssertionError to IllegalStateException
- Catch the IllegalStateException in
InitialImageOperation.processChunk() and return false, to allow the
GII to be aborted and retried if appropriate
- Add debug log to record if this has occurred
- Add unit test for the new behaviour
Authored-by: Donal Evans <do...@vmware.com>
<!-- 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:
- [ ] 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](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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] gesterzhou commented on a diff in pull request #7569: GEODE-10222: Handle null memberID in GII
Posted by GitBox <gi...@apache.org>.
gesterzhou commented on code in PR #7569:
URL: https://github.com/apache/geode/pull/7569#discussion_r845541641
##########
geode-core/src/main/java/org/apache/geode/internal/cache/versions/DiskVersionTag.java:
##########
@@ -44,7 +44,7 @@ public void setMemberID(DiskStoreID memberID) {
@Override
public void replaceNullIDs(VersionSource memberID) {
if (getMemberID() == null) {
- throw new AssertionError("Member id should not be null for persistent version tags");
+ throw new IllegalStateException("Member id should not be null for persistent version tags");
Review Comment:
I would rather not to introduce a new Exception class. We can use DiskAccessException here.
--
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-jbarrett commented on a diff in pull request #7569: GEODE-10222: Handle null memberID in GII
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on code in PR #7569:
URL: https://github.com/apache/geode/pull/7569#discussion_r846320347
##########
geode-core/src/main/java/org/apache/geode/internal/cache/versions/DiskVersionTag.java:
##########
@@ -44,7 +44,7 @@ public void setMemberID(DiskStoreID memberID) {
@Override
public void replaceNullIDs(VersionSource memberID) {
if (getMemberID() == null) {
- throw new AssertionError("Member id should not be null for persistent version tags");
+ throw new IllegalStateException("Member id should not be null for persistent version tags");
Review Comment:
Whatever the exception is it should be a checked exception so it has to be handled at the immediate call site and never has a chance to leak out.
--
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] DonalEvans commented on pull request #7569: GEODE-10222: Handle null memberID in GII
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on PR #7569:
URL: https://github.com/apache/geode/pull/7569#issuecomment-1092286783
> Approved. But looking at the call hierarchy, it looks like that replaceNullIDs is called by a lot of other methods and was wondering if there were any call stacks where the assertionError was handled (possible behavior change)
I checked, and there was nowhere we were handling that exception.
--
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-jbarrett commented on a diff in pull request #7569: GEODE-10222: Handle null memberID in GII
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on code in PR #7569:
URL: https://github.com/apache/geode/pull/7569#discussion_r845420737
##########
geode-core/src/main/java/org/apache/geode/internal/cache/versions/DiskVersionTag.java:
##########
@@ -44,7 +44,7 @@ public void setMemberID(DiskStoreID memberID) {
@Override
public void replaceNullIDs(VersionSource memberID) {
if (getMemberID() == null) {
- throw new AssertionError("Member id should not be null for persistent version tags");
+ throw new IllegalStateException("Member id should not be null for persistent version tags");
Review Comment:
But we know them to be possibly `null` due to race/feature, so perhaps the oft overloaded `IllegalStateException` is too board. I would suggest an exception specific to this issue, like `NullMemberIdException`.
--
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-jbarrett commented on a diff in pull request #7569: GEODE-10222: Handle null memberID in GII
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on code in PR #7569:
URL: https://github.com/apache/geode/pull/7569#discussion_r846322418
##########
geode-core/src/main/java/org/apache/geode/internal/cache/versions/DiskVersionTag.java:
##########
@@ -44,7 +44,7 @@ public void setMemberID(DiskStoreID memberID) {
@Override
public void replaceNullIDs(VersionSource memberID) {
if (getMemberID() == null) {
- throw new AssertionError("Member id should not be null for persistent version tags");
+ throw new IllegalStateException("Member id should not be null for persistent version tags");
Review Comment:
Conservation of well defined strongly named `Exception` classes is an anti-pattern. It cost us nothing to create new exceptions but cost us dearly if we don't. See [GEODE-10066](https://issues.apache.org/jira/browse/GEODE-10066) for a really good example of how throwing the inappropriate unchecked exception can cause all sorts of ugly.
--
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] DonalEvans commented on a diff in pull request #7569: GEODE-10222: Handle null memberID in GII
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on code in PR #7569:
URL: https://github.com/apache/geode/pull/7569#discussion_r847839798
##########
geode-core/src/main/java/org/apache/geode/internal/cache/versions/DiskVersionTag.java:
##########
@@ -44,7 +44,7 @@ public void setMemberID(DiskStoreID memberID) {
@Override
public void replaceNullIDs(VersionSource memberID) {
if (getMemberID() == null) {
- throw new AssertionError("Member id should not be null for persistent version tags");
+ throw new IllegalStateException("Member id should not be null for persistent version tags");
Review Comment:
Given that throwing a checked exception from `replaceNullIDs()` resulted in pretty big cascading changes, I modified the approach to instead be to not throw an exception at all if the memberID is null, but instead check if it's still null after calling `replaceNullIDs()` and returning false from `processChunk()` if it is.
Given that we've never seen the old `AssertionError` show up anywhere else, and that it wasn't being handled anywhere, removing the exception throwing behaviour entirely should be safe.
--
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] nabarunnag commented on pull request #7569: GEODE-10222: Handle null memberID in GII
Posted by GitBox <gi...@apache.org>.
nabarunnag commented on PR #7569:
URL: https://github.com/apache/geode/pull/7569#issuecomment-1092253975
Approved. But looking at the call hierarchy, it looks like that replaceNullIDs is called by a lot of other methods and was wondering if there were any call stacks where the assertionError was handled (possible behavior change)
--
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] kirklund commented on a diff in pull request #7569: GEODE-10222: Handle null memberID in GII
Posted by GitBox <gi...@apache.org>.
kirklund commented on code in PR #7569:
URL: https://github.com/apache/geode/pull/7569#discussion_r845544951
##########
geode-core/src/main/java/org/apache/geode/internal/cache/versions/DiskVersionTag.java:
##########
@@ -44,7 +44,7 @@ public void setMemberID(DiskStoreID memberID) {
@Override
public void replaceNullIDs(VersionSource memberID) {
if (getMemberID() == null) {
- throw new AssertionError("Member id should not be null for persistent version tags");
+ throw new IllegalStateException("Member id should not be null for persistent version tags");
Review Comment:
`DiskAccessException` says "Indicates that an IOException during a disk region operation." so that's probably not a good fit here.
--
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] DonalEvans merged pull request #7569: GEODE-10222: Handle null memberID in GII
Posted by GitBox <gi...@apache.org>.
DonalEvans merged PR #7569:
URL: https://github.com/apache/geode/pull/7569
--
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] kirklund commented on a diff in pull request #7569: GEODE-10222: Handle null memberID in GII
Posted by GitBox <gi...@apache.org>.
kirklund commented on code in PR #7569:
URL: https://github.com/apache/geode/pull/7569#discussion_r845444917
##########
geode-core/src/main/java/org/apache/geode/internal/cache/versions/DiskVersionTag.java:
##########
@@ -44,7 +44,7 @@ public void setMemberID(DiskStoreID memberID) {
@Override
public void replaceNullIDs(VersionSource memberID) {
if (getMemberID() == null) {
- throw new AssertionError("Member id should not be null for persistent version tags");
+ throw new IllegalStateException("Member id should not be null for persistent version tags");
Review Comment:
Would `NullMemberIdException` extend `IllegalStateException`? We need to be concerned with how the call-path handles any runtime exceptions that are thrown. It's easy to ensure that internal-only checked exceptions don't make it to the User but runtime exceptions can easily slip through. Would you make it a new external visible User exception?
--
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] DonalEvans commented on a diff in pull request #7569: GEODE-10222: Handle null memberID in GII
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on code in PR #7569:
URL: https://github.com/apache/geode/pull/7569#discussion_r846211452
##########
geode-core/src/main/java/org/apache/geode/internal/cache/versions/DiskVersionTag.java:
##########
@@ -44,7 +44,7 @@ public void setMemberID(DiskStoreID memberID) {
@Override
public void replaceNullIDs(VersionSource memberID) {
if (getMemberID() == null) {
- throw new AssertionError("Member id should not be null for persistent version tags");
+ throw new IllegalStateException("Member id should not be null for persistent version tags");
Review Comment:
I'm happy to make whatever changes are requested, but I would like the changes to be something that everyone has agreed on as the best course of action. If @pivotal-jbarrett and @kirklund could settle on what the correct approach is, I will implement that.
--
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