dschneider-pivotal commented on a change in pull request #7145:
URL: https://github.com/apache/geode/pull/7145#discussion_r762137062
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/Oplog.java
##########
@@ -7649,4 +7653,26 @@ public void sendTo(DataOutput out) throws IOException {
}
+ /**
+ * Used to track all information's about live entries that region has in
this oplog.
+ * That information is only needed until oplog is compacted. This is because
compaction will
+ * clear all live entries from this oplog.
+ */
+ private static class RegionMap {
+
+ final AtomicReference<ConcurrentMap<Long, DiskRegionInfo>> regionMap =
+ new AtomicReference<>(new ConcurrentHashMap<>());
+
+ public synchronized void close() {
+ regionMap.set(null);
+ }
+
+ public synchronized ConcurrentMap<Long, DiskRegionInfo> get() {
Review comment:
remove the synchronized
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/Oplog.java
##########
@@ -7649,4 +7653,26 @@ public void sendTo(DataOutput out) throws IOException {
}
+ /**
+ * Used to track all information's about live entries that region has in
this oplog.
+ * That information is only needed until oplog is compacted. This is because
compaction will
+ * clear all live entries from this oplog.
+ */
+ private static class RegionMap {
+
+ final AtomicReference<ConcurrentMap<Long, DiskRegionInfo>> regionMap =
+ new AtomicReference<>(new ConcurrentHashMap<>());
+
+ public synchronized void close() {
Review comment:
remove the synchronized. The final atomic reference give you protection
from concurrent callers of close and get.
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/Oplog.java
##########
@@ -7649,4 +7653,26 @@ public void sendTo(DataOutput out) throws IOException {
}
+ /**
+ * Used to track all information's about live entries that region has in
this oplog.
+ * That information is only needed until oplog is compacted. This is because
compaction will
+ * clear all live entries from this oplog.
+ */
+ private static class RegionMap {
+
+ final AtomicReference<ConcurrentMap<Long, DiskRegionInfo>> regionMap =
+ new AtomicReference<>(new ConcurrentHashMap<>());
+
+ public synchronized void close() {
+ regionMap.set(null);
+ }
+
+ public synchronized ConcurrentMap<Long, DiskRegionInfo> get() {
+ if (regionMap.get() == null) {
Review comment:
Instead of calling "regionMap.get()" twice in this method, call it once
and save the result into a local var. Then test the local var and return it.
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/Oplog.java
##########
@@ -1303,16 +1305,18 @@ void close(DiskRegion dr) {
// while a krf is being created can not close a region
lockCompactor();
try {
- addUnrecoveredRegion(dr.getId());
- DiskRegionInfo dri = getDRI(dr);
- if (dri != null) {
- long clearCount = dri.clear(null);
- if (clearCount != 0) {
- totalLiveCount.addAndGet(-clearCount);
- // no need to call handleNoLiveValues because we now have an
- // unrecovered region.
+ if (!isDrfOnly()) {
Review comment:
If this is a safe change to make to close then it seems like you should
also avoid calling lockCompactor if isDrfOnly(). It is called to prevent the
compactor from running while the code inside the try is running. Or did you
determine that isDrfOnly can only be safely called while the compactor is not
running?
The only concern I have about this new isDrfOnly check here is that it also
causes us not to call addUnrecoveredRegion. Most of the places that use the
unrecoveredRegion count, use it to figure out if this oplog might have some
live values. So for this drfOnly oplog it does not have any live values so
those places in the code don't need to worry about this oplog having live
values.
But it is also used in checkForRecoverableRegion which will call
dri.testAndSetRecovered which will associate the dri with its DiskRegionView. I
think we probably still need to do this and if I'm correct then we still need
to call addUnrecoveredRegion here even if isDrfOnly() is true.
For those places that do use unrecoveredRegionCount > 0 to indicate that
something can't be done; could you in those places also check for isDrfOnly?
I'm thinking of hasNoLiveValues() and needsCompaction().
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
