GEODE-6696: EntryEvenImpl.offHeapLock created only if off-heap in use #3581
Conversation
geode-core/src/main/java/org/apache/geode/internal/cache/EntryEventImpl.java
Outdated
Show resolved
Hide resolved
geode-core/src/main/java/org/apache/geode/internal/cache/EntryEventImpl.java
Outdated
Show resolved
Hide resolved
geode-core/src/main/java/org/apache/geode/internal/cache/EntryEventImpl.java
Outdated
Show resolved
Hide resolved
geode-core/src/main/java/org/apache/geode/internal/cache/EntryEventImpl.java
Outdated
Show resolved
Hide resolved
geode-core/src/main/java/org/apache/geode/internal/cache/EntryEventImpl.java
Outdated
Show resolved
Hide resolved
mivanac
force-pushed
the
feature/GEODE-6696
branch
from
ae77bb5 to
9d45d63
Compare
| InternalDistributedSystem ds = | ||
| (InternalDistributedSystem) region.getCache().getDistributedSystem(); | ||
| if (ds.getOffHeapStore() != null) { | ||
| this.offHeapLock = new Object(); |
There was a problem hiding this comment.
It looks like we could end up initializing the offHeapLock more than one place, which seems like an issue to me. Shouldn't this only be initialized in the constructor (if needed) and then used everywhere else? Or, alternately, a check could be added here for if offHeapLock is already non-null. My concern is that we would be creating a new lock each time fromData is called (and it is called all over the place), and then for each call get a different lock object so that we wouldn't actually be locking, because each call would be the only one holding that particular lock.
There was a problem hiding this comment.
I am unclear how this is working given that offHeapLock is final. Is it actually only being set once?
There was a problem hiding this comment.
You are right about it being final. EntryEventImpl has multiple constructors and this is just one of them. I don't see fromData initializing offHeapLock. For an EntryEventImpl that we deserialize it will use the no-arg constructor which sets offHeapLock to null.
There was a problem hiding this comment.
You're right. The way it was showing the diff in Github made it look like that line was in fromData instead of in a constructor
geode-core/src/main/java/org/apache/geode/internal/cache/EntryEventImpl.java
Show resolved
Hide resolved
geode-core/src/main/java/org/apache/geode/internal/cache/EntryEventImpl.java
Outdated
Show resolved
Hide resolved
geode-core/src/main/java/org/apache/geode/internal/cache/EntryEventImpl.java
Outdated
Show resolved
Hide resolved
mivanac
force-pushed
the
feature/GEODE-6696
branch
from
9d45d63 to
189bb86
Compare
|
Since changes are approved, could you merge this changes. |
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 buildrun 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?
Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to [email protected].