jchen21 commented on pull request #5689:
URL: https://github.com/apache/geode/pull/5689#issuecomment-740265251
> ```
> I read your new fix and have different opinion. The root cause is the new
oplog (i.e. #2) created during offline compaction wrote a krf with
totalCount==0.
>
> You fix is to still read the krf and when found it's totalCount is 0,
change it to be totalLiveCount. This is just a patch fix. The real issue is: we
should NOT create such a krf with totalCount==0. Actually, when offline
compaction, it is not allowed to create krf.
>
> Here is the better fix for this issue:
>
> diff --git
a/geode-core/src/main/java/org/apache/geode/internal/cache/Oplog.java
b/geode-core/src/main/java/org/apache/geode/internal/cache/Oplog.java
> index f6daa3a1d6..0a31af9386 100755
> --- a/geode-core/src/main/java/org/apache/geode/internal/cache/Oplog.java
> +++ b/geode-core/src/main/java/org/apache/geode/internal/cache/Oplog.java
> @@ -582,7 +582,7 @@ public class Oplog implements CompactableOplog,
Flushable {
> createDrf(null);
> createCrf(null);
> // open krf for offline compaction
> - if (getParent().isOfflineCompacting()) {
> + if (getParent().isOfflineCompacting() && couldHaveKrf()) {
> krfFileCreate();
> }
> } catch (Exception ex) {
> @@ -3980,7 +3980,7 @@ public class Oplog implements CompactableOplog,
Flushable {
> } catch (IOException ignore) {
> }
>
> - if (this.krf.f.exists()) {
> + if (this.krf.f != null && this.krf.f.exists()) {
> this.krf.f.delete();
> }
> }
> @@ -5789,7 +5789,7 @@ public class Oplog implements CompactableOplog,
Flushable {
> if (((rv / (double) rvHWMtmp) * 100) <=
parent.getCompactionThreshold()) {
> return true;
> }
> - } else if (this.totalLiveCount.get() <= 0) {
> + } else {
> return true;
> }
>
>
> I don't know who introduced "if (getParent().isOfflineCompacting()" here.
To be conservative, I still keep this check. Maybe we can remove the 3 lines.
But I want to be conservative for now.
> ```
I don't think we should avoid creating krf during offline compaction. Your
proposed fix avoid creating the krf file during offline compaction. After the
compaction, the original non-compacted krf is also deleted. Without a krf,
there is a significant impact on the performance of persistent recovery.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
