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

Posted to notifications@geode.apache.org by "aditya87 (GitHub)" <gi...@apache.org> on 2018/12/13 06:10:22 UTC

[GitHub] [geode] aditya87 opened pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument

- Reduce dependence on RegionFunctionArgs
- Also clean up CreateRegionCommand to achieve a clean separation between validations and calling RegionCreateFunction

Signed-off-by: Ken Howe <kh...@pivotal.io>

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:
- [X] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

- [X] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?

- [X] Is your initial contribution a single, squashed commit?

- [X] Does `gradlew build` run cleanly?

- [X] 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/2998 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] aditya87 commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument

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

There's nothing in the XML POJO that indicates that, and we didn't want to touch it. But yes, that would be a better abstraction.

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

[GitHub] [geode] aditya87 commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument

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

Paired on this with @jinmeiliao, we have a resolution.

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

[GitHub] [geode] aditya87 commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument

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

I think we can refactor this condition a bit.

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

[GitHub] [geode] Petahhh commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument

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

This looks like code duplication with `org.apache.geode.management.internal.cli.functions.RegionFunctionArgs.PartitionArgs#getUserSpecifiedPartitionAttributes`

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

[GitHub] [geode] aditya87 commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument

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

The reason we had the factory class in the first place is to be able to test-drive in isolation. The original unit test was quite easy, it's just that with all these parameters it's a little harder to read now. But I'm not sure if I agree that it would be easier if this was all in CreateRegionCommand? Sure, you could set up the unit test correctly, but the generated RegionConfig object would be used within the code (without being an explicit output), making it harder to extract and test. If we had to unit test it in that situation, I feel that the abstractions we'd have to make would be a little mock-heavy.

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

[GitHub] [geode] aditya87 commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument

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

Sure, we could change the name.

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

[GitHub] [geode] Petahhh commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument

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

The name `checkIfRegionAlreadyExists` can be better. My understanding of the current behaviour is:

If the region doesn't exist, do not throw an error

If the region exists and there is conflict in the configuration, throw an error.

I'm not sure what a better name would be. Perhaps you could introduce a new method that checks for the region's existence and return a boolean? Then the current function could be renamed `checkForRegionConfigurationConflicts`?

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

[GitHub] [geode] aditya87 commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument

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

The reason we had the factory class in the first place is to be able to test-drive in isolation. The original unit test was quite easy, it's just that with all these parameters it's a little harder to read now. But I'm not sure if I agree that it would be easier if this was all in CreateRegionCommand? Sure, you could set up the unit test correctly, but the generated RegionConfig object would be used within the code (without being an explicit output), making it harder to extract and test. If we had to unit test it in that situation, I feel it the abstractions we'd have to make would be a little mock-heavy.

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

[GitHub] [geode] aditya87 commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument

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

The reason we had the factory class in the first place is to be able to test-drive in isolation. The original unit test was quite easy, it's just that with all these parameters it's a little harder to read now. But I'm not sure if I agree that it would be easier if this was all in CreateRegionCommand? Sure, you could set up the unit test correctly, but the generated RegionConfig object would be embedded in the code, making it harder to extract. If we had to unit test it in that situation, I feel it the abstractions we'd have to make would be a little mock-heavy.

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

[GitHub] [geode] aditya87 commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument

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

I think you missed one thing about the behavior -- is that if "ifNotExists" is set, throw an EntityExistsException with an "OK" status, so that the caller can check that status and not fail. So it's a little more subtle. I think we can figure out a better name for sure.

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

[GitHub] [geode] aditya87 commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument

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

Yes, and the plan is to get rid of the other one when we refactor AlterRegionCommand and remove RegionFunctionArgs

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

[GitHub] [geode] jinmeiliao commented on issue #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument

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

@aditya87 the test failures seems to be that region's persisted attributes are not as expected. Since the failures are dunit/acceptance failures, I would suggest adding more junit tests around the functionalities you modified, like in the command, where you are creating the regionConfig and where you are persisting the config. 

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

[GitHub] [geode] aditya87 commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument

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

Sure, that's a refactor we could do.

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

[GitHub] [geode] jinmeiliao commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument

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

why we don't care about algorithm anymore here?

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

[GitHub] [geode] Petahhh commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument

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

Given this function is already at 250 lines and there are lots of small independent sections of code, maybe you can bring this out into a series of smaller private methods. It would make this function easier to read

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

[GitHub] [geode] aditya87 commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument

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

Sure -- this function was essentially a copy-pasta.

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

[GitHub] [geode] aditya87 commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument

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

Sure

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

[GitHub] [geode] jinmeiliao commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument

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

CacheElement itself is already Serializable.

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

[GitHub] [geode] aditya87 commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument

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

The reason we had the factory class in the first place is to be able to test-drive in isolation. The original unit test was quite easy, it's just that with all these parameters it's a little harder to read. But I'm not sure if I agree that it would be easier if this was all in CreateRegionCommand?

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

[GitHub] [geode] Petahhh commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument

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

For readability, can this condition be inverted and return if the region doesn't exist?

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

[GitHub] [geode] aditya87 commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument

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

We have enough end-to-end test coverage such that we don't need a unit test here.

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

[GitHub] [geode] aditya87 commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument

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

I think you missed one more thing about the behavior -- is that if "ifNotExists" is set, throw an EntityExistsException with an "OK" status, so that the caller can check that status and not fail. So it's a little more subtle. I think we can figure out a better name for sure.

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

[GitHub] [geode] aditya87 commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument

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

The reason we had the factory class in the first place is to be able to test-drive in isolation. The original unit test was quite easy, it's just that with all these parameters it's a little harder to read now. But I'm not sure if I agree that it would be easier if this was all in CreateRegionCommand?

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

[GitHub] [geode] Petahhh commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument

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

I don't see a test for this new method. Do you feel like there is enough coverage for this behaviour in another test?

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

[GitHub] [geode] Petahhh commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument

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

Can `regionPathData.getParent()` be `null`? We should do a null check on it

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

[GitHub] [geode] aditya87 commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument

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

We have enough end-to-end test coverage such that we don't need a unit test here. However, we can backfill one.

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

[GitHub] [geode] jinmeiliao closed pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument

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

[ pull request closed by jinmeiliao ]

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

[GitHub] [geode] jinmeiliao commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument

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

EvictionAttributes is already Serializable

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

[GitHub] [geode] Petahhh commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument

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

Same with `PartitionAttributesImpl.fromConfig` and `ExpirationAttributesImpl.fromConfig` 

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

[GitHub] [geode] jinmeiliao commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument

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

I am wondering should we also have a similar concept in our config object to indicate NONE. If the update case, if user pass in null, that means I want to leave the existing configuration alone, if they pass in NONE, that means I want to wipe out the previous configuration.

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

[GitHub] [geode] aditya87 commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument

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

I think you missed one more thing about the behavior -- is that if "ifNotExists" is set, throw an EntityExistsException with an "OK" status, so that the caller can check that status and not fail. So it's a little more subtle. I think we can figure out a better name for sure. A boolean return might be harder because we need to throw specific exception messages based on conditions.

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

[GitHub] [geode] jinmeiliao commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument

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

but having this huge list of parameters in method is very anti-pattern. You can use Junit's ArgumentCaptor to capture the RegionConfig passed to the function call and verify that way.

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

[GitHub] [geode] aditya87 commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument

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

Sure, will fix.

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

[GitHub] [geode] aditya87 commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument

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

We had a test case where the algorithm was provided but the intended behavior was no eviction (since action was none).

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

[GitHub] [geode] jinmeiliao commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument

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

I wonder if this is really worthwhile. This function is going to be so hard to maintain and test. I thought we would completely get rid of this factory classes, and just construct the config object right in the command and set the values as we validate each group of parameters. We do not want to repeat the same number parameters as the command again. It's easy to unit test the command class using the parserRule, but it's not easy to unit test this.

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