HDDS-12007. BlockDataStreamOutput should only send one PutBlock during close.

[szetszwo]

What changes were proposed in this pull request?

Do not do putBlock in link(..). Otherwise,

• it has two putBlock requests, and
• it will add the block to db and causing a truncation problem.

What is the link to the Apache JIRA

HDDS-12007

How was this patch tested?

Using existing tests.

[ivandika3]

@szetszwo Thanks for the patch.

Should we also revert the changes for HDDS-6500 and HDDS-6137?

[szetszwo]

@ivandika3 , reverting the code can lead to an incompatible change. Currently, the client sends a PutBlock when closing the stream, and the server reads the PutBlock and processes it. If we revert HDDS-6500 and HDDS-6137, the new client won't send a PutBlock, then the old server will fail since it expects a PutBlock.

The change in this PR is to ignore the PutBlock although the client still sends it.

[ivandika3]

@szetszwo Thanks for the info.

The new client won't send a PutBlock, then the old server will fail since it expects a PutBlock.

Got it, in that case, shall we add some comments about the deprecation so that readers can see that the first putBlock will be ignored by the datanode?

However, eventually the client logic needs to be cleaned up.

[ivandika3]

@szetszwo I have another question. Does sendForward trigger applyTransaction as well? In that case, although ContainerStateMachine#link will ignore it, will ContainerStateMachine#applyTransaction update the DN state?

[xichen01]

should we remove KeyValueStreamDataChannel#putBlockRequest, KeyValueStreamDataChannel#buffers and related code, if we remove PutBlock here ?
As I understand it, in order to create the PutBlockReques, we keep a KeyValueStreamDataChannel#buffers in memory, which guarantees that the buffers data will only be written at close. If we remove the PutBlock here, do we not need it anymore?

[szetszwo]

@xichen01 , thanks for reviewing this!

Yes, the putBlockRequest can be removed.

Unfortunately, we still need the buffers since clients still send a putBlockRequest. We need the buffers so the request won't be written to the file as data.

[szetszwo]

... Does sendForward trigger applyTransaction as well? ...

sendForward is to send the stream header request using the async API.

For Ozone, the request currently is a WriteChunk request but not a PutBlock request. See

ozone/hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockDataStreamOutput.java

Lines 206 to 226 in ab29a55

	ContainerProtos.ContainerCommandRequestProto.Builder builder =
	ContainerProtos.ContainerCommandRequestProto.newBuilder()
	.setCmdType(ContainerProtos.Type.StreamInit)
	.setContainerID(blockID.get().getContainerID())
	.setDatanodeUuid(id).setWriteChunk(writeChunkRequest);
	if (tokenString != null) {
	builder.setEncodedToken(tokenString);
	}
	ContainerCommandRequestMessage message =
	ContainerCommandRequestMessage.toMessage(builder.build(), null);
	if (isDatastreamPipelineMode) {
	return Preconditions.checkNotNull(xceiverClient.getDataStreamApi())
	.stream(message.getContent().asReadOnlyByteBuffer(),
	RatisHelper.getRoutingTable(pipeline));
	} else {
	return Preconditions.checkNotNull(xceiverClient.getDataStreamApi())
	.stream(message.getContent().asReadOnlyByteBuffer());
	}

In the future, we may combine WriteChunk and PutBlock as in HDDS-9130.

[adoroszlai]

Failure in acceptance (secure) is being fixed in #7668.

[ivandika3]

Thanks for the info. LGTM +1.

For Ozone, the request currently is a WriteChunk request but not a PutBlock request

Thank you for the info. Checking the code again seems that ContainerStateMachine#applyTransaction is no-op since WriteChunk sent has no data and the WriteStage is COMMIT_DATA. So maybe the sendForward can be skipped in the future.

[adoroszlai]

Thanks @szetszwo for the patch, @ivandika3, @xichen01 for the review.

[szetszwo]

@ivandika3 , @xichen01 , thanks a lot for the review!

@adoroszlai , thanks for helping out the tests and merging this!

[szetszwo]

... So maybe the sendForward can be skipped in the future.

@ivandika3 , sendForward currently is to add a WritChunk transaction. In the future, you are right that we may be able to skip it or do something else. There are a few options:

1. RATIS-1936 has changed sendForward to be configurable. We may disable it in Ozone.
2. It could send both WriteChunk and PutBlock together in sendForward.
3. Simply send PutBlock in sendForward.