Fix force GC doesn't work under forceAllowCompaction when disk is full

[hangc0276]

Motivation

When I set forceAllowCompaction=true and one ledger disk reaches max usage threshold and transfer bookie to readOnly mode, I expire some pulsar topics or delete some topics to free up disk space. I found that ledger compression cannot be triggered when using curl -XPUT http://localhost:8000/api/v1/bookie/gc command.

The root cause is that when one ledger disk reaches max usage threshold, it will suspend minor and major compaction

bookkeeper/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java

Lines 1041 to 1058 in f7579fd

	public void diskFull(File disk) {
	if (gcThread.isForceGCAllowWhenNoSpace()) {
	gcThread.enableForceGC();
	} else {
	gcThread.suspendMajorGC();
	gcThread.suspendMinorGC();
	}
	}
	@Override
	public void allDisksFull(boolean highPriorityWritesAllowed) {
	if (gcThread.isForceGCAllowWhenNoSpace()) {
	gcThread.enableForceGC();
	} else {
	gcThread.suspendMajorGC();
	gcThread.suspendMinorGC();
	}
	}

When we use curl -XPUT http://localhost:8000/api/v1/bookie/gc command to trigger compaction, it will be filtered by suspendMajor and suspendMinor flag.

bookkeeper/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java

Lines 416 to 444 in f7579fd

	if (((isForceMajorCompactionAllow && force) || (enableMajorCompaction
	&& (force || curTime - lastMajorCompactionTime > majorCompactionInterval)))
	&& (!suspendMajor)) {
	// enter major compaction
	LOG.info("Enter major compaction, suspendMajor {}", suspendMajor);
	majorCompacting.set(true);
	try {
	doCompactEntryLogs(majorCompactionThreshold, majorCompactionMaxTimeMillis);
	} finally {
	lastMajorCompactionTime = System.currentTimeMillis();
	// and also move minor compaction time
	lastMinorCompactionTime = lastMajorCompactionTime;
	gcStats.getMajorCompactionCounter().inc();
	majorCompacting.set(false);
	}
	} else if (((isForceMinorCompactionAllow && force) || (enableMinorCompaction
	&& (force || curTime - lastMinorCompactionTime > minorCompactionInterval)))
	&& (!suspendMinor)) {
	// enter minor compaction
	LOG.info("Enter minor compaction, suspendMinor {}", suspendMinor);
	minorCompacting.set(true);
	try {
	doCompactEntryLogs(minorCompactionThreshold, minorCompactionMaxTimeMillis);
	} finally {
	lastMinorCompactionTime = System.currentTimeMillis();
	gcStats.getMinorCompactionCounter().inc();
	minorCompacting.set(false);
	}
	}

It will lead to

• The bookie won't clean up deleted ledgers
• Ledger disk can't free up disk usage
• Bookie can't recover from readOnly state into Writeable state.

And then we can only trigger compaction by the following steps.

• Increase max disk usage threshold
• Restart the bookie
• Use command curl -XPUT http://localhost:8000/api/v1/bookie/gc to trigger compaction

Changes

1. Don't take the suspendMajor and suspendMinor flag into consideration when setting forceAllowCompaction=true and triggered by force GC.

[hangc0276]

rerun failure checks

[nicoloboschi]

Could we add unit tests?

[zymap]

Looks like it's expected behavior? We have another configuration isForceGCAllowWhenNoSpace which is used to control the force gc is allowed when no space, maybe you need to set it to true?

[hangc0276]

Looks like it's expected behavior? We have another configuration isForceGCAllowWhenNoSpace which is used to control the force gc is allowed when no space, maybe you need to set it to true?

@zymap If we set isForceGCAllowWhenNoSpace to true, it won't suspend minor and major compaction on GC, which will easily lead to ledger disk space exhaustion. After we delete topic or shorten the retention to release disk space, we can't trigger compaction due to compaction need another disk space and the current disk space has been exhausted. So we are not recommend to set isForceGCAllowWhenNoSpace to true

What we need is that the bookie suspend the minor and major compaction when disk usage reaches max threshold, but when we deleted topic or shorten the retention, we trigger GC by rest api, and bookie server neglect the current suspendMajor and suspendMinor flag to trigger compaction.

[zymap]

But after your change, it will run the major compaction first, which may lead to the dist exhaust as well and cannot compact the entry, right?

[hangc0276]

But after your change, it will run the major compaction first, which may lead to the dist exhaust as well and cannot compact the entry, right?

@zymap In order to keep the original logic, I add two flag for REST API to trigger GC, Please take a look, thanks.

[hangc0276]

Could we add unit tests?

@nicoloboschi I have added the unit test, please take a look, thanks.

[hangc0276]

rerun failure checks

[hangc0276]

rerun failure checks

[zymap]

What's the difference between the if and else? Looks like they are asserting the same thing?

[hangc0276]

Whether we will enter major or minor compaction is controlled by

((isForceMajorCompactionAllow && force) || (enableMajorCompaction
                    && (force || curTime - lastMajorCompactionTime > majorCompactionInterval)))
                    && (!suspendMajor)

That is:

1. isForceMajorCompactionAllow && force or (enableMajorCompaction && (force || curTime - lastMajorCompactionTime > majorCompactionInterval)
2. suspendMajor

We must meet above both conditions.

The if and else is different code path controlled by isForceMajorCompactionAllow && force and (enableMajorCompaction && (force || curTime - lastMajorCompactionTime > majorCompactionInterval)