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.

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-254: Removed deprecated Region.keys and Region.entries #488

Closed
wants to merge 1 commit into from

Conversation

davinash
Copy link
Contributor

@davinash davinash commented May 3, 2017

No description provided.

@dschneider-pivotal
Copy link
Contributor

This looks good. Pull it in.

Copy link
Contributor

@kirklund kirklund left a comment

Choose a reason for hiding this comment

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

+1

@asfgit asfgit closed this in 8c4a369 May 4, 2017
Copy link
Contributor

@dschneider-pivotal dschneider-pivotal left a comment

Choose a reason for hiding this comment

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

Some problems missed by previous review

@@ -187,7 +187,7 @@ public void destroyRegion(Object callbackArgument) throws CacheWriterException,
public Set entries(boolean recursive) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is wrong to keep these methods your are removing from Region on classes that implement the Region interface. It is too bad that these impls did not have @OverRide on them because that would have caused compilation failures.

Copy link
Contributor

Choose a reason for hiding this comment

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

AdminRegion was another class that implements Region that should be updated to no longer impl the methods you are removing. You should look at all the classes that implement Region.

@@ -388,7 +388,7 @@ public Set keySetOnServer() {
public Set keys() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you want this old "keys()" implementation to now be "keySet()". The old "keySet" did not properly call preOp/postOp.

@@ -321,7 +321,7 @@ public void destroyRegion(Object aCallbackArgument)
}

public Set entries(boolean recursive) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Another example of a method that should no longer exist since it is no longer on Region

@@ -1702,6 +1702,8 @@ public Set entrySet(boolean recursive) {
return entries(recursive);
}

public abstract Set entries(boolean recursive);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why add this abstract method when you are getting rid of entries(boolean)?

@davinash
Copy link
Contributor Author

davinash commented May 9, 2017

Thanks @dschneider-pivotal for review. I will fix those issue and raise another PR, as this one is already merged.

davinash pushed a commit to davinash/geode that referenced this pull request May 9, 2017
Replacing all inststance of entries with entrySet
davinash pushed a commit to davinash/geode that referenced this pull request May 10, 2017
Replacing all inststance of entries with entrySet

GEODE-254: Fixing the Test Failures
 - Removed test testIndexMaintenanceWithIndexOnMethodEntries since this is no more applicable.

GEODE-254: Spotless fixing.
asfgit pushed a commit that referenced this pull request May 12, 2017
…stance of entries with entrySet

GEODE-254: Fixing the Test Failures
 - Removed test testIndexMaintenanceWithIndexOnMethodEntries since this is no more applicable.

GEODE-254: Spotless fixing.
This closes #504

Signed-off-by: adongre <adongre@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants