jsedding commented on code in PR #3137:
URL: https://github.com/apache/jackrabbit-oak/pull/3137#discussion_r4092285984
##########
oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/GCJournal.java:
##########
@@ -68,7 +68,15 @@ public synchronized void persist(long reclaimedSize, long
repoSize,
@NotNull GCGeneration gcGeneration, long nodes, @NotNull String
root
) {
GCJournalEntry current = read();
- if (current.getGcGeneration().equals(gcGeneration)) {
+ GCGeneration currentGeneration = current.getGcGeneration();
+ // Compare generation and fullGeneration only, not isCompacted: the
on-disk journal
+ // format never persists isCompacted (GCJournalEntry.fromString always
deserializes it
+ // as false), so comparing the full GCGeneration would wrongly treat
every persist() of
+ // an already-journaled, still-compacted generation as new after a
restart (e.g. a
+ // standalone cleanup() on a store re-opened with an already-compacted
head), causing a
+ // duplicate entry to be written on every such restart.
+ if (currentGeneration.getGeneration() == gcGeneration.getGeneration()
+ && currentGeneration.getFullGeneration() ==
gcGeneration.getFullGeneration()) {
Review Comment:
I don't understand why you write
> 1. We are not updating gc.log after cleanup on the standBy side. (Cleanup
is enabled by default).
> 2. And this is problematic since a stale gc.log would cause SNFE when
referenced segments don't exist.
Isn't that exactly what this PR is meant to resolve?
Regarding the alternative to this change that I suggested, I tested it
locally and it works (all tests pass). The only gotcha is that `GcJournalTest`
needs all assertions against `GCGeneration` adjusted (GCGeneration.isCompacted
needs to flipped from false to true), because it asserts directly against the
change in `GCJorunalEntry.fromString()`.
But you go ahead with this change if you disagree. It's not 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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
