You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@geode.apache.org by "jinmeiliao (GitHub)" <gi...@apache.org> on 2019/01/07 22:59:44 UTC

[GitHub] [geode] jinmeiliao opened pull request #3054: GEODE-5971: refactor AlterRegionCommand to use RegionConfig object

Co-authored-by: Kenneth Howe <kh...@pivotal.io>
Co-authored-by: Aditya Anchuri <aa...@pivotal.io>
Co-authored-by: Peter Tran <pt...@pivotal.io>

* refactor AlterRegionCommand to use ResultModel and RegionConfig
* only alter region in the same group as the region is created on
* when cluster configuration is enabled, if region does not exist in CC, the command would error out.
* The command would no longer alter region not created by gfsh command
* add more unit tests

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, you check travis-ci 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.


[ Full content available at: https://github.com/apache/geode/pull/3054 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] pdxrunner commented on pull request #3054: GEODE-5971: refactor AlterRegionCommand to use RegionConfig object

Posted by "pdxrunner (GitHub)" <gi...@apache.org>.

can't this just be
```
existing = new ExpirationAttributesType(0, ExpirationAction.INVALIDATE);
```

[ Full content available at: https://github.com/apache/geode/pull/3054 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] dschneider-pivotal commented on pull request #3054: GEODE-5971: refactor AlterRegionCommand to use RegionConfig object

Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.

ExpirationAction is an external API so you probably do not want to add this method here. Instead add it on one of the internal classes. External apis need to be documented and can not change in future releases (without first being deprecated and then a major release done). It seems like this method could instead be on one of the internal xml parsing classes. 
If you do end up adding it here then add a javadoc comments that defines the allowed "xmlValue" and that it will throw IllegalArgumentException, etc.

[ Full content available at: https://github.com/apache/geode/pull/3054 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jinmeiliao closed pull request #3054: GEODE-5971: refactor AlterRegionCommand to use RegionConfig object

Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.

[ pull request closed by jinmeiliao ]

[ Full content available at: https://github.com/apache/geode/pull/3054 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] pdxrunner commented on pull request #3054: GEODE-5971: refactor AlterRegionCommand to use RegionConfig object

Posted by "pdxrunner (GitHub)" <gi...@apache.org>.

New public methods in the external classes should have a javadoc

[ Full content available at: https://github.com/apache/geode/pull/3054 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] pdxrunner commented on pull request #3054: GEODE-5971: refactor AlterRegionCommand to use RegionConfig object

Posted by "pdxrunner (GitHub)" <gi...@apache.org>.

Correct spelling: `evication` -> `eviction`

[ Full content available at: https://github.com/apache/geode/pull/3054 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] pdxrunner commented on pull request #3054: GEODE-5971: refactor AlterRegionCommand to use RegionConfig object

Posted by "pdxrunner (GitHub)" <gi...@apache.org>.

If I understand what this test is about, I think a more descriptive name would be something like `alterEvictionMaxOnRegionWIthoutEvictionActionHasNoEffect`

[ Full content available at: https://github.com/apache/geode/pull/3054 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] pdxrunner commented on pull request #3054: GEODE-5971: refactor AlterRegionCommand to use RegionConfig object

Posted by "pdxrunner (GitHub)" <gi...@apache.org>.

javadoc?

[ Full content available at: https://github.com/apache/geode/pull/3054 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] pdxrunner commented on pull request #3054: GEODE-5971: refactor AlterRegionCommand to use RegionConfig object

Posted by "pdxrunner (GitHub)" <gi...@apache.org>.

javadocs for public method? This one and the following methods.

[ Full content available at: https://github.com/apache/geode/pull/3054 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jinmeiliao commented on pull request #3054: GEODE-5971: refactor AlterRegionCommand to use RegionConfig object

Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.

setter removed

[ Full content available at: https://github.com/apache/geode/pull/3054 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] pdxrunner commented on pull request #3054: GEODE-5971: refactor AlterRegionCommand to use RegionConfig object

Posted by "pdxrunner (GitHub)" <gi...@apache.org>.

Comment should reflect that this is a `help method`, not `helper class`

[ Full content available at: https://github.com/apache/geode/pull/3054 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jinmeiliao commented on pull request #3054: GEODE-5971: refactor AlterRegionCommand to use RegionConfig object

Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.

there is also customExpiry class name and init properties in the constructor as well.

[ Full content available at: https://github.com/apache/geode/pull/3054 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] pdxrunner commented on pull request #3054: GEODE-5971: refactor AlterRegionCommand to use RegionConfig object

Posted by "pdxrunner (GitHub)" <gi...@apache.org>.

Can't they passed to the constructor as `null` as above in line 283?

[ Full content available at: https://github.com/apache/geode/pull/3054 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jinmeiliao commented on pull request #3054: GEODE-5971: refactor AlterRegionCommand to use RegionConfig object

Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.

javadoc added

[ Full content available at: https://github.com/apache/geode/pull/3054 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jinmeiliao commented on pull request #3054: GEODE-5971: refactor AlterRegionCommand to use RegionConfig object

Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.

comments added

[ Full content available at: https://github.com/apache/geode/pull/3054 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] pdxrunner commented on pull request #3054: GEODE-5971: refactor AlterRegionCommand to use RegionConfig object

Posted by "pdxrunner (GitHub)" <gi...@apache.org>.

The `throws` declaration is not needed

[ Full content available at: https://github.com/apache/geode/pull/3054 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jinmeiliao commented on issue #3054: GEODE-5971: refactor AlterRegionCommand to use RegionConfig object

Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.

@Petahhh @aditya87 

[ Full content available at: https://github.com/apache/geode/pull/3054 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jinmeiliao commented on pull request #3054: GEODE-5971: refactor AlterRegionCommand to use RegionConfig object

Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.

the constructor also takes customExpiry classname and a initProperties, so instead of using constructor with two null values, I used this way.

[ Full content available at: https://github.com/apache/geode/pull/3054 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jinmeiliao commented on pull request #3054: GEODE-5971: refactor AlterRegionCommand to use RegionConfig object

Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.

fixed

[ Full content available at: https://github.com/apache/geode/pull/3054 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] pdxrunner commented on pull request #3054: GEODE-5971: refactor AlterRegionCommand to use RegionConfig object

Posted by "pdxrunner (GitHub)" <gi...@apache.org>.

Typo:`exitingAction` -> `existingAction`

[ Full content available at: https://github.com/apache/geode/pull/3054 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] pdxrunner commented on pull request #3054: GEODE-5971: refactor AlterRegionCommand to use RegionConfig object

Posted by "pdxrunner (GitHub)" <gi...@apache.org>.

Unneeded `throws Exception` should be removed from test declarations

[ Full content available at: https://github.com/apache/geode/pull/3054 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] dschneider-pivotal commented on pull request #3054: GEODE-5971: refactor AlterRegionCommand to use RegionConfig object

Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.

ExpirationAttributes is in the geode external API so you should probably not change it. It is wrong to add settors to it since the class is supposed to be immutable. The only reason its fields are not "final" was to allow serialization to be implemented. So instead of settors just use the constructor to set the timeout and action.

[ Full content available at: https://github.com/apache/geode/pull/3054 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org