Heap memory leak problem when ledger replication failed

[gaozhangmin]

Motivation

production environment, memory leak always happened, and there were ledger cannot be replicated successfully.

This cause by when openLedgerNoRecovery with BKNotEnoughBookiesException, the LedgerHandler won't closed properly, caused memory leak

bookkeeper/bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/ReplicationWorker.java

Lines 364 to 424 in c7236ad

	try (LedgerHandle lh = admin.openLedgerNoRecovery(ledgerIdToReplicate)) {
	Set<LedgerFragment> fragments =
	getUnderreplicatedFragments(lh, conf.getAuditorLedgerVerificationPercentage());
	if (LOG.isDebugEnabled()) {
	LOG.debug("Founds fragments {} for replication from ledger: {}", fragments, ledgerIdToReplicate);
	}
	boolean foundOpenFragments = false;
	for (LedgerFragment ledgerFragment : fragments) {
	if (!ledgerFragment.isClosed()) {
	foundOpenFragments = true;
	continue;
	}
	if (!tryReadingFaultyEntries(lh, ledgerFragment)) {
	LOG.error("Failed to read faulty entries, so giving up replicating ledgerFragment {}",
	ledgerFragment);
	continue;
	}
	try {
	admin.replicateLedgerFragment(lh, ledgerFragment, onReadEntryFailureCallback);
	} catch (BKException.BKBookieHandleNotAvailableException e) {
	LOG.warn("BKBookieHandleNotAvailableException while replicating the fragment", e);
	} catch (BKException.BKLedgerRecoveryException e) {
	LOG.warn("BKLedgerRecoveryException while replicating the fragment", e);
	} catch (BKException.BKNotEnoughBookiesException e) {
	LOG.warn("BKNotEnoughBookiesException while replicating the fragment", e);
	}
	}
	if (foundOpenFragments || isLastSegmentOpenAndMissingBookies(lh)) {
	deferLedgerLockRelease = true;
	deferLedgerLockRelease(ledgerIdToReplicate);
	return false;
	}
	fragments = getUnderreplicatedFragments(lh, conf.getAuditorLedgerVerificationPercentage());
	if (fragments.size() == 0) {
	LOG.info("Ledger replicated successfully. ledger id is: " + ledgerIdToReplicate);
	underreplicationManager.markLedgerReplicated(ledgerIdToReplicate);
	return true;
	} else {
	deferLedgerLockRelease = true;
	deferLedgerLockReleaseOfFailedLedger(ledgerIdToReplicate);
	numDeferLedgerLockReleaseOfFailedLedger.inc();
	// Releasing the underReplication ledger lock and compete
	// for the replication again for the pending fragments
	return false;
	}
	} catch (BKNoSuchLedgerExistsOnMetadataServerException e) {
	// Ledger might have been deleted by user
	LOG.info("BKNoSuchLedgerExistsOnMetadataServerException while opening "
	+ "ledger {} for replication. Other clients "
	+ "might have deleted the ledger. "
	+ "So, no harm to continue", ledgerIdToReplicate);
	underreplicationManager.markLedgerReplicated(ledgerIdToReplicate);
	getExceptionCounter("BKNoSuchLedgerExistsOnMetadataServerException").inc();
	return false;
	} catch (BKNotEnoughBookiesException e) {
	logBKExceptionAndReleaseLedger(e, ledgerIdToReplicate);
	throw e;

Changes

close LedgerHandler when openComplete with exception

[gaozhangmin]

@eolivelli PTAL

[gaozhangmin]

@hangc0276

[eolivelli]

@nicoloboschi @RaulGracia @Ghatage PTAL if you have time please

@gaozhangmin which memory is leaked ? direct memory ?

[gaozhangmin]

@nicoloboschi @RaulGracia @Ghatage PTAL if you have time please

@gaozhangmin which memory is leaked ? direct memory ?

heap memory

[gaozhangmin]

@nicoloboschi @RaulGracia @Ghatage PTAL if you have time please

@gaozhangmin which memory is leaked ? direct memory ?

[gaozhangmin]

[gaozhangmin]

I've tested this pr in my test cluster, it can solve the heap memory leak problem.

[nicoloboschi]

+1 nice catch

I'd like to point out that the dead objects are kept alive by AbstractZkLedgerManager

[dlg99]

LGTM, with one nit

[gaozhangmin]

@dlg99 PTAL

[dlg99]

@gaozhangmin looks good! Thank you.

[RaulGracia]

LGTM, would it make sense to add some specific test to validate the scenario in which memory was getting leaked (i.e., ledger handles are being closed in the situation in which they were left open before)?

[gaozhangmin]

LGTM, would it make sense to add some specific test to validate the scenario in which memory was getting leaked (i.e., ledger handles are being closed in the situation in which they were left open before)?

@RaulGracia I tried, But it's hard to verify if the ReadOnlyLedgerHandle is closed.