jchen21 commented on pull request #5689: URL: https://github.com/apache/geode/pull/5689#issuecomment-730682983
> add unit test coverage of needsCompaction to OplogTest. > > It seems like the only change you made was to say we don't need to compact if an Oplog has a totalCount == 0. I think totalCount == 0 means that the oplog is empty. This seems like something we should compact. Do you understand why offline compaction gave us an empty oplog? I'm concerned this change might cause us not to remove empty oplogs. Or maybe we say it has no need to compact but still do a check and remove it for being empty somewhere else? I have added a unit test in `OplogTest`. `totalCount` == 0 does not necessarily mean the oplog is empty. You remind me of the case of an empty oplog. There is another field `totalLiveCount` to check. What I see in the persistent recovery immediately after an offline compaction is, the offline compacted oplog is non-empty. During the recovery, `totalCount` is `0`. However, `totalLiveCount` has the actual number of live entries in the oplog. Therefore I add one more condition to check `totalLiveCount`. If `totalCount` == 0 and `totalLiveCount` > 0, such as the case of recovery after offline compaction, we don't need compaction again. Are there any other cases that need compaction, even though `totalCount` == 0 and `totalLiveCount` > 0? If `totalCount` == 0 and `totalLiveCount` <= 0, then the oplog is empty, which needs compaction. Please correct me if I am wrong. ---------------------------------------------------------------- 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]
