Skip to content
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.

Sign up for GitHub

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

Jump to bottom

GEODE-7678: Support for cache-listener and client-notification for Partitioned Region Clear operation. #4987

Closed
wants to merge 20 commits into from
Closed

GEODE-7678: Support for cache-listener and client-notification for Partitioned Region Clear operation. #4987

wants to merge 20 commits into from

Conversation

agingade
Copy link

The changes are made to PR clear messaging and locking mechanism to preserve
cache-listener and client-events ordering during concurrent cache operation
while clear in progress.

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:

  • [Y] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

  • [Y] Has your PR been rebased against the latest commit within the target branch (typically develop)?

  • [Y] Is your initial contribution a single, squashed commit?

  • [Y] Does gradlew build run cleanly?

  • [Y] Have you written or updated unit tests to verify your changes?

  • [NA] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?

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.

gesterzhou and others added 7 commits March 16, 2020 14:36
@gesterzhou @BenjaminPerryRoss
@DonalEvans @nabarunnag
GEODE-7683: introduce BR.cmnClearRegion
7d557d5 
Co-authored-by: Xiaojian Zhou <gzhou@pivotal.io>

GEODE-7684: Create messaging class for PR Clear (apache#4689)

* Added new message class and test

Co-authored-by: Benjamin Ross <bross@pivotal.io>
Co-authored-by: Donal Evans <doevans@pivotal.io>
@gesterzhou @nabarunnag
GEODE-7682: add PR.clear API (apache#4755)
7b62741 
* GEODE-7683: introduce BR.cmnClearRegion

Co-authored-by: Xiaojian Zhou <gzhou@pivotal.io>
@gesterzhou
PR.clear's event id should be created and used in BR (apache#4805)
49872b4 
* GEODE-7857: PR.clear's event id should be created and used in BR
@gesterzhou
GEODE-7912: cacheWriter should be triggered when PR.clear (apache#4882)
04e2081 
        Co-authored-by: Anil <agingade@pivotal.io>
        Co-authored-by: Xiaojian Zhou <gzhou@pivotal.io>
@gesterzhou
GEODE-7983: Clear region writer callbacks should not be invoked for b…
81a9178 
…ucket regions (apache#4954)
@jujoramos
GEODE-7676: Add PR clear with expiration tests (apache#4970)
e87b3b7 
Added distributed tests to verify the clear operation on Partitioned
Regions works as expected when expiration is configured.

- Added unit and distributed tests.
- Fixed LocalRegion class to clear the entryExpiryTasks Map whenever
  the cancelAllEntryExpiryTasks method is invoked.
GEODE-7678: Add support for cache listener and client notification fo…
3f263e2 
…r PR clear

The changes are made to PR clear messaging and locking mechanism to preserve
cache-listener and client-events ordering during concurrent cache operation
while clear in progress.
@lgtm-com
Copy link

lgtm-com bot commented Apr 22, 2020

This pull request introduces 1 alert when merging 90928f3 into e87b3b7 - view on LGTM.com

new alerts:

  • 1 for Non-synchronized override of synchronized method

Add new message class into serializer text.
65cdf02 
Add check for client interests with local filter.
@lgtm-com
Copy link

lgtm-com bot commented Apr 23, 2020

This pull request introduces 1 alert when merging 65cdf02 into e87b3b7 - view on LGTM.com

new alerts:

  • 1 for Non-synchronized override of synchronized method

Handle member departure during clear.
106f1c4 
Added wait for secondary buckets to become primary.
DonalEvans
DonalEvans reviewed Apr 28, 2020
View reviewed changes
Copy link
Contributor

@DonalEvans DonalEvans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly just some code cleanup and typos, but I have a couple of questions inline as well.

geode-core/src/distributedTest/java/org/apache/geode/cache/PRCacheListenerDistributedTest.java Outdated Show resolved Hide resolved
...e/src/distributedTest/java/org/apache/geode/cache/ReplicateCacheListenerDistributedTest.java Show resolved Hide resolved
...t/java/org/apache/geode/internal/cache/PartitionedRegionAfterClearNotificationDUnitTest.java Outdated Show resolved Hide resolved
geode-core/src/main/java/org/apache/geode/internal/cache/BucketRegion.java Outdated Show resolved Hide resolved
geode-core/src/main/java/org/apache/geode/internal/cache/ClearPartitionedRegion.java Outdated Show resolved Hide resolved
geode-core/src/main/java/org/apache/geode/internal/cache/ClearPartitionedRegion.java Outdated Show resolved Hide resolved
geode-core/src/main/java/org/apache/geode/internal/cache/ClearPartitionedRegionMessage.java Outdated Show resolved Hide resolved
geode-core/src/main/java/org/apache/geode/internal/cache/ClearPartitionedRegionMessage.java Outdated Show resolved Hide resolved
geode-core/src/main/java/org/apache/geode/internal/cache/ClearPartitionedRegionMessage.java Outdated Show resolved Hide resolved
geode-core/src/main/java/org/apache/geode/internal/cache/ClearPartitionedRegionMessage.java Outdated Show resolved Hide resolved
jujoramos
jujoramos reviewed Apr 29, 2020
View reviewed changes
geode-core/src/distributedTest/java/org/apache/geode/cache/PRCacheListenerDistributedTest.java Outdated Show resolved Hide resolved
...e/src/distributedTest/java/org/apache/geode/cache/ReplicateCacheListenerDistributedTest.java Show resolved Hide resolved
...t/java/org/apache/geode/internal/cache/PartitionedRegionAfterClearNotificationDUnitTest.java Outdated Show resolved Hide resolved
...t/java/org/apache/geode/internal/cache/PartitionedRegionAfterClearNotificationDUnitTest.java Outdated Show resolved Hide resolved
...t/java/org/apache/geode/internal/cache/PartitionedRegionAfterClearNotificationDUnitTest.java Outdated Show resolved Hide resolved
geode-core/src/main/java/org/apache/geode/internal/cache/ClearPartitionedRegion.java Outdated Show resolved Hide resolved
geode-core/src/main/java/org/apache/geode/internal/cache/ClearPartitionedRegion.java Outdated Show resolved Hide resolved
geode-core/src/main/java/org/apache/geode/internal/cache/InternalRegion.java Outdated Show resolved Hide resolved
geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegionDataStore.java Outdated Show resolved Hide resolved
geode-core/src/main/java/org/apache/geode/internal/cache/partitioned/RegionAdvisor.java Outdated Show resolved Hide resolved
anilkumar gingade added 2 commits April 29, 2020 19:19
Incorporate review comments.
d348955 
Add PartitionedRegionPartialClearException.java
Add check to verify primary buckets are available for all the local secondary buckets before clear.
run spA
12ece70
anilkumar gingade added 3 commits May 3, 2020 16:08
Add review comments
1cde1df 
Add validation to check the buckets cleared after clear completed on all nodes.
Throw CacheWriterException during netSearch timeout failure with cache write.
Remove unused fields
5e11608
Remove processException() which was just invoking method from super c…
d41397c 
…lass.
jchen21
jchen21 reviewed May 14, 2020
View reviewed changes
geode-core/src/distributedTest/java/org/apache/geode/cache/PRCacheListenerDistributedTest.java Outdated Show resolved Hide resolved
geode-core/src/distributedTest/java/org/apache/geode/cache/PRCacheListenerDistributedTest.java Outdated Show resolved Hide resolved
geode-core/src/distributedTest/java/org/apache/geode/cache/PRCacheListenerDistributedTest.java Outdated Show resolved Hide resolved
...e/src/distributedTest/java/org/apache/geode/cache/ReplicateCacheListenerDistributedTest.java Show resolved Hide resolved
...e/src/distributedTest/java/org/apache/geode/cache/ReplicateCacheListenerDistributedTest.java Show resolved Hide resolved
...t/java/org/apache/geode/internal/cache/PartitionedRegionAfterClearNotificationDUnitTest.java Outdated Show resolved Hide resolved
...t/java/org/apache/geode/internal/cache/PartitionedRegionAfterClearNotificationDUnitTest.java Outdated Show resolved Hide resolved
...t/java/org/apache/geode/internal/cache/PartitionedRegionAfterClearNotificationDUnitTest.java Show resolved Hide resolved
...utedTest/java/org/apache/geode/internal/cache/PartitionedRegionPersistentClearDUnitTest.java Outdated Show resolved Hide resolved
DonalEvans
DonalEvans reviewed May 14, 2020
View reviewed changes
...t/java/org/apache/geode/internal/cache/PartitionedRegionAfterClearNotificationDUnitTest.java Show resolved Hide resolved
...t/java/org/apache/geode/internal/cache/PartitionedRegionAfterClearNotificationDUnitTest.java
Comment on lines +132 to +143
RegionShortcut shortcut = getRegionShortCut();
if (shortcut.isPersistent()) {
if (shortcut == RegionShortcut.PARTITION_PERSISTENT) {
shortcut = RegionShortcut.PARTITION;
} else if (shortcut == RegionShortcut.PARTITION_PERSISTENT_OVERFLOW) {
shortcut = RegionShortcut.PARTITION_OVERFLOW;
} else if (shortcut == RegionShortcut.PARTITION_REDUNDANT_PERSISTENT) {
shortcut = RegionShortcut.PARTITION_REDUNDANT;
} else if (shortcut == RegionShortcut.PARTITION_REDUNDANT_PERSISTENT_OVERFLOW) {
shortcut = RegionShortcut.PARTITION_REDUNDANT_OVERFLOW;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this test, RegionShortcut is always PARTITION_REDUNDANT so this check is not needed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch :)

...rc/distributedTest/java/org/apache/geode/internal/cache/PartitionedRegionClearDUnitTest.java Outdated Show resolved Hide resolved
...rc/distributedTest/java/org/apache/geode/internal/cache/PartitionedRegionClearDUnitTest.java Outdated Show resolved Hide resolved
...t/java/org/apache/geode/internal/cache/PartitionedRegionAfterClearNotificationDUnitTest.java Show resolved Hide resolved
...rc/distributedTest/java/org/apache/geode/internal/cache/PartitionedRegionClearDUnitTest.java Outdated
Comment on lines 158 to 159
// TODO: notify register clients
// client2.invoke(()->verifyRegionSize(true, expectedNum));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can these comments be removed?

geode-core/src/main/java/org/apache/geode/cache/Region.java Show resolved Hide resolved
geode-core/src/test/java/org/apache/geode/internal/cache/DistributedRegionJUnitTest.java Outdated
Comment on lines 259 to 265
@Test(expected = UnsupportedOperationException.class)
public void localClearIsNotSupportedOnReplicatedRegion() {
RegionEventImpl event = createClearRegionEvent();
DistributedRegion region = (DistributedRegion) event.getRegion();
region.basicLocalClear(event);
fail("Expect UnsupportedOperationException");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this test instead assert that the UnsupportedOperationException is thrown from the basicLocalClear() call, rather than using an expected exception?

geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java
@Override
public void clear() {
throw new UnsupportedOperationException();
/* @Override */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this commented out section be removed?

geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegionClear.java Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented May 15, 2020

This pull request introduces 122 alerts and fixes 2 when merging 17f3903 into 81102ce - view on LGTM.com

new alerts:

  • 49 for Useless null check
  • 29 for Missing space in string literal
  • 19 for Non-synchronized override of synchronized method
  • 4 for Implicit conversion from array to string
  • 3 for Equals method does not inspect argument type
  • 3 for Dereferenced variable may be null
  • 3 for Implicit narrowing conversion in compound assignment
  • 2 for Container contents are never accessed
  • 2 for Result of multiplication cast to wider type
  • 2 for Potential database resource leak
  • 2 for Spurious Javadoc @param tags
  • 1 for Information exposure through a stack trace
  • 1 for Potential output resource leak
  • 1 for Boxed variable is never null
  • 1 for Race condition in double-checked locking object initialization

fixed alerts:

  • 1 for Useless null check
  • 1 for Disabled Spring CSRF protection

@mhansonp mhansonp closed this May 15, 2020
@mhansonp mhansonp reopened this May 15, 2020
@agingade agingade closed this May 27, 2020
@agingade agingade deleted the feature/GEODE-7678 branch May 27, 2020 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jujoramos jujoramos jujoramos left review comments

@jchen21 jchen21 jchen21 left review comments

@DonalEvans DonalEvans DonalEvans left review comments

@gesterzhou gesterzhou gesterzhou approved these changes

@BenjaminPerryRoss BenjaminPerryRoss Awaiting requested review from BenjaminPerryRoss BenjaminPerryRoss is a code owner

@nabarunnag nabarunnag Awaiting requested review from nabarunnag nabarunnag is a code owner

@pivotal-eshu pivotal-eshu Awaiting requested review from pivotal-eshu pivotal-eshu is a code owner

@jhuynh1 jhuynh1 Awaiting requested review from jhuynh1

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@agingade @jujoramos @gesterzhou @pivotal-eshu @jchen21 @DonalEvans @mhansonp