[GitHub] [geode] aditya87 commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument
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
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
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
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
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] aditya87 commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument
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] aditya87 commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument
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
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
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] aditya87 commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument
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
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] aditya87 commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument
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] aditya87 commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument
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
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
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] aditya87 commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument
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
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] aditya87 commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument
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] aditya87 commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument
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] aditya87 commented on pull request #2998: GEODE-6103 RegionCreateFunction takes RegionConfig in argument
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
