HDDS-15634. Avoid updating container delete transaction ID on SCM delete log append

[xichen01]

What changes were proposed in this pull request?

Reduce long-tail latency of OM allocateBlock

When SCM handles DeleteScmKeyBlocks requests, AllocateScmBlock can see high tail latency because SequenceIdGenerator#getNextId waits for SCMStateMachine to finish applying DeleteScmKeyBlocks's DeletedBlockLogStateManagerImpl#addTransactionsToDB.

Profiling shows a large part of DeletedBlockLogStateManagerImpl#addTransactionsToDB time is spent updating SCM-side ContainerInfo#deleteTransactionId.
This field is not used by current SCM business logic, while DN-side delete transaction tracking is maintained independently.

This commit removes the SCM-side ContainerInfo#deleteTransactionId update from addTransactionsToDB to reduce unnecessary StateMachine apply work and improve write tail latency.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-15634

How was this patch tested?

existing test

https://github.com/xichen01/ozone/actions/runs/27909174625

[ivandika3]

Thanks @xichen01 . Overall LGTM.

How about we deprecate the ContainerInfo deleteTransactionId entirely by marking proto and fields to be deprecated and adding some comments? Also we might just remove the ContainerManager#updateDeleteTransactionId

[xichen01]

Thanks @xichen01 . Overall LGTM.

How about we deprecate the ContainerInfo deleteTransactionId entirely by marking proto and fields to be deprecated and adding some comments? Also we might just remove the ContainerManager#updateDeleteTransactionId

deprecate ContainerInfo deleteTransactionId and ContainerStateManagerImpl#updateDeleteTransactionId

[ivandika3]

@xichen01 Thanks for the update. LGTM +1.

[szetszwo]

... SequenceIdGenerator#getNextId waits for SCMStateMachine to finish applying DeleteScmKeyBlocks's DeletedBlockLogStateManagerImpl#addTransactionsToDB

Quick question: why getNextId will wait for addTransactionsToDB? Is it happen only if a new batch of ids is needed?

[szetszwo]

@xichen01 , the change looks good. Just have a question (in my previous comment) and a comment inlined.

[szetszwo]

Since this is a local method, we could just delete it without deprecated it.

[xichen01]

... SequenceIdGenerator#getNextId waits for SCMStateMachine to finish applying DeleteScmKeyBlocks's DeletedBlockLogStateManagerImpl#addTransactionsToDB

Quick question: why getNextId will wait for addTransactionsToDB? Is it happen only if a new batch of ids is needed?

@szetszwo
Yes it is happen only if a new batch of ids is needed, to request the next batch ID, you must go through Ratis. SCMStateMachine's applyTransaction is also single-threaded; any slow transaction will block subsequent transactions.

This is a typical stack.

"IPC Server handler 23 on default port 9863" #289 [3407343] daemon prio=5 os_prio=0 cpu=581314.26ms elapsed=287858.01s tid=0x00007feb27451810 nid=3407343 waiting on condition  [0x00007feb8e0b9000]
   java.lang.Thread.State: TIMED_WAITING (parking)
	at jdk.internal.misc.Unsafe.park(java.base@21-sdi/Native Method)
	- parking to wait for  <0x000004161650b580> (a java.util.concurrent.CompletableFuture$Signaller)
	at java.util.concurrent.locks.LockSupport.parkNanos(java.base@21-sdi/LockSupport.java:269)
	at java.util.concurrent.CompletableFuture$Signaller.block(java.base@21-sdi/CompletableFuture.java:1866)
	at java.util.concurrent.ForkJoinPool.unmanagedBlock(java.base@21-sdi/ForkJoinPool.java:3780)
	at java.util.concurrent.ForkJoinPool.managedBlock(java.base@21-sdi/ForkJoinPool.java:3725)
	at java.util.concurrent.CompletableFuture.timedGet(java.base@21-sdi/CompletableFuture.java:1939)
	at java.util.concurrent.CompletableFuture.get(java.base@21-sdi/CompletableFuture.java:2095)
	at org.apache.hadoop.hdds.scm.ha.SCMRatisServerImpl.submitRequest(SCMRatisServerImpl.java:227)
	at org.apache.hadoop.hdds.scm.ha.SCMHAInvocationHandler.invokeRatisServer(SCMHAInvocationHandler.java:123)
	at org.apache.hadoop.hdds.scm.ha.SCMHAInvocationHandler.invokeRatis(SCMHAInvocationHandler.java:112)
	at org.apache.hadoop.hdds.scm.ha.SCMHAInvocationHandler.invoke(SCMHAInvocationHandler.java:74)
	at org.apache.hadoop.hdds.scm.ha.$Proxy14.allocateBatch(Unknown Source)
	at org.apache.hadoop.hdds.scm.ha.SequenceIdGenerator.getNextId(SequenceIdGenerator.java:142)
	at org.apache.hadoop.hdds.scm.block.BlockManagerImpl.newBlock(BlockManagerImpl.java:201)
	at org.apache.hadoop.hdds.scm.block.BlockManagerImpl.allocateBlock(BlockManagerImpl.java:177)
	at org.apache.hadoop.hdds.scm.server.SCMBlockProtocolServer.allocateBlock(SCMBlockProtocolServer.java:243)
	at org.apache.hadoop.hdds.scm.protocol.ScmBlockLocationProtocolServerSideTranslatorPB.allocateScmBlock(ScmBlockLocationProtocolServerSideTranslatorPB.java:222)
	at org.apache.hadoop.hdds.scm.protocol.ScmBlockLocationProtocolServerSideTranslatorPB.processMessage(ScmBlockLocationProtocolServerSideTranslatorPB.java:150)
	at 
//...

If the cluster's write and delete QPS are high, the end result will be a large number of long-tail allocateSCMBlocks.

[szetszwo]

+1 the change looks good.

[ivandika3]

Thanks @xichen01 for the patch and @szetszwo for the review.