Faster topic removal in MultiDC setup #1884
Conversation
| try { | ||
| remove(paths.topicPath(topicName)); | ||
| deleteInTransaction(pathsForRemoval); |
There was a problem hiding this comment.
should we also use this method in other places ? because now we have two different methods for removing paths🤔 for paths without children it will be no-brainer (just use a simple remove), but for those with children we now have a choice
There was a problem hiding this comment.
Transactional removal should only be used when needed - it has a higher overhead + it is required to supply all the paths. Most of the time we remove nodes with prefix e.g. delete all nodes for topic ((like /hermes/groups/group/topic/*) and this requires deleteChildrenIfNeeded which is not available in transactions. So imo these two methods have different usage scenarios
| @@ -58,7 +61,7 @@ private <T> void execute(RepositoryCommand<T> command, boolean isRollbackEnabled | |||
| throw ExceptionWrapper.wrapInInternalProcessingExceptionIfNeeded(e, command.toString(), repoHolder.getDatacenterName()); | |||
| } | |||
| } catch (Exception e) { | |||
| logger.warn("Execute failed with an error", e); | |||
| logger.warn("Failed to execute repository command: {} in ZK dc: {}", command, System.currentTimeMillis() - start, e); | |||
There was a problem hiding this comment.
Failed to execute repository command: {} in ZK dc: {} in: {} ms", repoHolder.getDatacenterName(), System.currentTimeMillis() - start
moscicky
force-pushed
the
consisteny-analysis
branch
from
c38b67b
to
a75a7bb
Compare
| throw new InternalProcessingException("Attempting to remove empty set of paths from ZK"); | ||
| } | ||
| ensureConnected(); | ||
| CuratorTransactionFinal transaction = zookeeper.inTransaction().delete().forPath(paths.get(0)).and(); |
There was a problem hiding this comment.
issue: ZooKeeper API mentions that inTransaction method is deprecated.
/**
* Start a transaction builder
*
* @return builder object
* @deprecated use {@link #transaction()} instead
*/
public CuratorTransaction inTransaction();Can we use transaction() method here?
There was a problem hiding this comment.
Will be resolved here: #1886
| } | ||
|
|
||
| transaction.commit(); | ||
| } |
There was a problem hiding this comment.
note: Here is my proposition for refactor:
protected void deleteInTransaction(List<String> paths) throws Exception {
if (paths.isEmpty()) {
throw new InternalProcessingException("Attempting to remove empty set of paths from ZK");
}
ensureConnected();
zookeeper.transaction().forOperations(
paths.stream().map(x -> {
try {
return zookeeper.transactionOp().delete().forPath(x);
} catch (Exception e) {
throw new RuntimeException(e);
}
}).collect(Collectors.toList())
);
}Feel free to tweak with it :)
There was a problem hiding this comment.
Will be resolved here: #1886
Fixes a race condition due to which topic/group removal in remote DC was taking up to 150s. Exact bug is described in
pl.allegro.tech.hermes.infrastructure.zookeeper.ZookeeperTopicRepository#removeTopic. After this change group/topic removal takes tens of ms in remote DC.