You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by "xmas92 (GitHub)" <gi...@apache.org> on 2019/02/26 14:05:54 UTC
[GitHub] [geode] xmas92 opened pull request #3235: GEODE-5135:
Refactor AbstractRegionMap dependencies
`BaseRegionMap` will be an abstract base class for `AbstractRegionMap`
and `ProxyRegionMap`. Will move common functionality into this class.
Aims to remove dependency between `AbstractRegionMap` and `ProxyRegionMap`
Move ARMLockTestHook to RegionMap to remove circular dependency
while keeping the impact as small as possible.
Move `forceInvalidateEvent`, `shouldInvokeCallbacks` and
`switchEventOwnerAndOriginRemote` to `BaseRegionMap` to reduce
dependency between `ProxyRegionMap` and `AbstractRegionMap`
Move `createCallbackEvent` from `AbstractRegionMap` to `EntryEventImpl`
This reduces dependecy between `AbstractRegionMap` and `ProxyRegionMap`
Before:
After:
Complete patch:
Co-Authored-By: Patric Lantz <he...@users.noreply.github.com>
Co-Authored-By: Sayyed Ali Kiaian Mousavy <sa...@kth.se>
Co-Authored-By: Nicole Jagelid <ni...@kth.se>
Co-Authored-By: ddahlgren95 <46...@users.noreply.github.com>
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?
- [ ] Have you written or updated unit tests to verify your changes?
- [x] 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/3235 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #3235:
GEODE-5135: Refactor AbstractRegionMap dependencies
Posted by "kirklund (GitHub)" <gi...@apache.org>.
It might be better to move this to an EntryEventFactory type class instead of EntryEventImpl.
[ Full content available at: https://github.com/apache/geode/pull/3235 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund closed pull request #3235: GEODE-5135:
Refactor AbstractRegionMap dependencies
Posted by "kirklund (GitHub)" <gi...@apache.org>.
[ pull request closed by kirklund ]
[ Full content available at: https://github.com/apache/geode/pull/3235 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] xmas92 commented on issue #3235: GEODE-5135:
Refactor AbstractRegionMap dependencies
Posted by "xmas92 (GitHub)" <gi...@apache.org>.
Should `EntryEventFactory` be kept as a separate class or should it be turned into an interface that `EntryEventImpl` (or potential future `EntryEventFactoryImpl` class) implements?
All three types of implementations seems to be used within the package. Is there a preferred one that the Geode project strive to use?
[ Full content available at: https://github.com/apache/geode/pull/3235 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] xmas92 commented on pull request #3235: GEODE-5135:
Refactor AbstractRegionMap dependencies
Posted by "xmas92 (GitHub)" <gi...@apache.org>.
We were hesitant at first to introduce more classes. But separating this functionallity into its own class is probably makes its intent clearer. Will change
[ Full content available at: https://github.com/apache/geode/pull/3235 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] xmas92 commented on issue #3235: GEODE-5135:
Refactor AbstractRegionMap dependencies
Posted by "xmas92 (GitHub)" <gi...@apache.org>.
As part of a course here at [KTH](https://www.kth.se/en) we had to find an open issue related to refactoring on any open-source project (with more than 10k loc) and attempt to resolve it.
Looking for issues in the JIRA with refactoring in the title we found [GEODE-5135](https://issues.apache.org/jira/browse/GEODE-5135). It is quite hard to find refactoring issues in JIRA as most newly created issues are already being worked on and the old ones are not always still relevant. GEODE-5135 even though old and not 100% relevant still seemed like an improvement that could be implemented.
[ Full content available at: https://github.com/apache/geode/pull/3235 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on issue #3235: GEODE-5135:
Refactor AbstractRegionMap dependencies
Posted by "kirklund (GitHub)" <gi...@apache.org>.
@lkn2993 I don't have a way to retrigger the precheckin but you can by pushing an empty commit `git commit --allow-empty -m "Trigger precheckin"`. When I merge this to develop, I'll squash it down to one commit anyways.
[ Full content available at: https://github.com/apache/geode/pull/3235 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on issue #3235: GEODE-5135:
Refactor AbstractRegionMap dependencies
Posted by "kirklund (GitHub)" <gi...@apache.org>.
This change set has been merged to geode develop branch which means it's scheduled for inclusion in the Apache Geode 1.10.0 release. Thank you!
[ Full content available at: https://github.com/apache/geode/pull/3235 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] lkn2993 commented on issue #3235: GEODE-5135:
Refactor AbstractRegionMap dependencies
Posted by "lkn2993 (GitHub)" <gi...@apache.org>.
I think there was some sort of server problem during the test. Mind if we ask you to run the failed test case again?
[ Full content available at: https://github.com/apache/geode/pull/3235 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on issue #3235: GEODE-5135:
Refactor AbstractRegionMap dependencies
Posted by "kirklund (GitHub)" <gi...@apache.org>.
@xmas92 Making `EntryEventFactory` an interface is a good approach. I would avoid having `EntryEventImpl` implement `EntryEventFactory`, so you could go with `EntryEventFactoryImpl` for now unless you can think of a better name (I can't think of anything right now).
[ Full content available at: https://github.com/apache/geode/pull/3235 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] xmas92 commented on pull request #3235: GEODE-5135:
Refactor AbstractRegionMap dependencies
Posted by "xmas92 (GitHub)" <gi...@apache.org>.
We were hesitant at first to introduce more classes. But separating this functionality into its own class is probably makes its intent clearer. Will change
[ Full content available at: https://github.com/apache/geode/pull/3235 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] xmas92 commented on pull request #3235: GEODE-5135:
Refactor AbstractRegionMap dependencies
Posted by "xmas92 (GitHub)" <gi...@apache.org>.
The `BaseRegionMap` prefix seems to just be an artifact of the refactoring. It can be safely removed even for static methods as they are available in both subclasses.
It might be clearer to keep them static as it shows that the functionality has nothing to do with an actual RegionMap instance. (They are functional helper functions for the `RegionMap`)
Will cleanup the `BaseRegionMap` prefixes
[ Full content available at: https://github.com/apache/geode/pull/3235 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] xmas92 commented on issue #3235: GEODE-5135:
Refactor AbstractRegionMap dependencies
Posted by "xmas92 (GitHub)" <gi...@apache.org>.
@kirklund Changed it to an interface.
There are probably some more methods like `createPutAllEvent` and `createRemoveAllEvent` that are candidates for being moved to the factory class. Perhaps not in the scope of the current issue.
[ Full content available at: https://github.com/apache/geode/pull/3235 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] xmas92 commented on pull request #3235: GEODE-5135:
Refactor AbstractRegionMap dependencies
Posted by "xmas92 (GitHub)" <gi...@apache.org>.
We were hesitant at first to introduce more classes. But separating this functionality into its own class probably makes its intent clearer. Will change
[ Full content available at: https://github.com/apache/geode/pull/3235 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #3235:
GEODE-5135: Refactor AbstractRegionMap dependencies
Posted by "kirklund (GitHub)" <gi...@apache.org>.
If you change these package-private static methods to non-static, the subclasses can invoke them without the BaseRegionMap. prefix. I recommend changing all of these to non-static:
```
void forceInvalidateEvent(EntryEventImpl event, LocalRegion owner)
boolean shouldInvokeCallbacks(final LocalRegion owner, final boolean isInitialized)
EntryEventImpl switchEventOwnerAndOriginRemote(EntryEventImpl event, boolean originRemote)
```
[ Full content available at: https://github.com/apache/geode/pull/3235 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #3235:
GEODE-5135: Refactor AbstractRegionMap dependencies
Posted by "kirklund (GitHub)" <gi...@apache.org>.
Actually, one more change to make. Please extract the `new EntryEventFactoryImpl()` calls into a private final member field like:
```
private final EntryEventFactory entryEventFactory = new EntryEventFactoryImpl();
```
Otherwise, we'll end up generating a lot of instances for garbage collection to deal with. AbstractRegionMap experiences a lot of traffic such as thousands of operations per second.
[ Full content available at: https://github.com/apache/geode/pull/3235 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #3235:
GEODE-5135: Refactor AbstractRegionMap dependencies
Posted by "kirklund (GitHub)" <gi...@apache.org>.
Actually, one more change to make. Please extract the `new EntryEventFactoryImpl()` calls into a private final member field and then use that field to create entry events:
```
private final EntryEventFactory entryEventFactory = new EntryEventFactoryImpl();
```
Otherwise, we'll end up generating a lot of instances for garbage collection to deal with. AbstractRegionMap experiences a lot of traffic such as thousands of operations per second.
[ Full content available at: https://github.com/apache/geode/pull/3235 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org