Fix schema ledger deletion when deleting topic with delete schema. #10383
Conversation
codelipenghui
requested review from
congbobo184,
315157973,
eolivelli,
jiazhai,
merlimat and
sijie
|
@BewareMyPower Please also help review this PR. |
| deleteFuture.complete(null); | ||
| }, null); | ||
| }); | ||
| FutureUtil.waitForAll(deleteFutures).whenComplete((v, e) -> { |
There was a problem hiding this comment.
whenComplete((v, e) need to handle the exception?
There was a problem hiding this comment.
No, one ledger delete failed, we only print the warn log instead failed the whole delete schema operation.
There was a problem hiding this comment.
If one ledger delete failed, does it need to be cleaned again? If delete zk path of the schema, we couldn't find its ledgers again.
| try { | ||
| ZkUtils.deleteFullPathOptimistic(zooKeeper, getSchemaPath(schemaId), -1); | ||
| } catch (InterruptedException | KeeperException thr) { | ||
| future.completeExceptionally(thr); |
There was a problem hiding this comment.
should return after future.completeExceptionally(thr)
| }); | ||
| FutureUtil.waitForAll(deleteFutures).whenComplete((v, e) -> { | ||
| try { | ||
| ZkUtils.deleteFullPathOptimistic(zooKeeper, getSchemaPath(schemaId), -1); |
There was a problem hiding this comment.
This call can be dangerous (I think it was already the case in the existing code). We're making a blocking call from the callback of an async call. It can lead to deadlock the ZK thread. We should never mix sync/async calls, rather we should convert into using ZkUtils.asyncDeleteFullPathOptimistic()
There was a problem hiding this comment.
@merlimat Fixed, PTAL.
|
@merlimat please take your final look, |
Related to #8167
Motivation
Fix schema ledger deletion when deleting topic with delete schema.
Modifications
Delete ledgers according to the schema index.
Verifying this change
New unit tests added.
Does this pull request potentially affect one of the following parts:
If
yeswas chosen, please highlight the changesDocumentation