Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug][Producer]Inaccurate transaction endSendOrAckOp for chunked message #1060

Closed
gunli opened this issue Jul 19, 2023 · 3 comments · Fixed by #1121
Closed

[Bug][Producer]Inaccurate transaction endSendOrAckOp for chunked message #1060

gunli opened this issue Jul 19, 2023 · 3 comments · Fixed by #1121

Comments

@gunli
Copy link
Contributor

gunli commented Jul 19, 2023

Expected behavior

When chunking, call transaction.endSendOrAckOp() only for the last chunked reqeust???

Actual behavior

I am NOT sure if it is a bug.

In internalSend(), when chunking, we share the transaction between all the chunked request:

nsr := &sendRequest{
	ctx:              request.ctx,
	msg:              request.msg,
	callback:         request.callback,
	callbackOnce:     request.callbackOnce,
	publishTime:      request.publishTime,
	blockCh:          request.blockCh,
	closeBlockChOnce: request.closeBlockChOnce,
	totalChunks:      totalChunks,
	chunkID:          chunkID,
	uuid:             uuid,
	chunkRecorder:    cr,
	transaction:      request.transaction,
}

In ReceivedSendReceipt(), we call transaction.endSendOrAckOp() for every request without checking if it is the last chunked one

if sr.transaction != nil {
	sr.transaction.endSendOrAckOp(nil)
}

In failTimeoutMessages(), we call transaction.endSendOrAckOp() for every request without checking if it is the last chunked one

if sr.transaction != nil {
	sr.transaction.endSendOrAckOp(nil) // And I think it should be errSendTimeout here
}

Steps to reproduce

Code review.

System configuration

Pulsar version: master

@RobertIndie @Gleiphir2769

@RobertIndie
Copy link
Member

Could you take a look at this? @liangyepianzhou

@liangyepianzhou
Copy link
Contributor

liangyepianzhou commented Jul 22, 2023

@gunli You are right. Thanks for you point it out. I have pushed a PR to fix it.

@liangyepianzhou
Copy link
Contributor

In failTimeoutMessages(), we call transaction.endSendOrAckOp() for every request without checking if it is the last >chunked one

if sr.transaction != nil {
  sr.transaction.endSendOrAckOp(nil) // And I think it should be errSendTimeout here
}

The errSendTimeout already was passed into the callback just before sr.transaction.endSendOrAckOp(nil).

if sr.callback != nil {
	sr.callbackOnce.Do(func() {
		sr.callback(nil, sr.msg, errSendTimeout)
	})
}
if (sr.totalChunks <= 1 || sr.chunkID == sr.totalChunks-1) && sr.transaction != nil {
	sr.transaction.endSendOrAckOp(nil)
}

@RobertIndie RobertIndie changed the title [Bug][Producer]Inaccurate transaction endSendOrAckOp for chunked message??? [Bug][Producer]Inaccurate transaction endSendOrAckOp for chunked message Jul 26, 2023
RobertIndie pushed a commit that referenced this issue Jul 26, 2023
…nk message (#1069)

Master #1060
### Motivation
1. For the chunk message, we only register the send operation once but end the send operation multiple times when receiving the send response. It will make the transaction can be committed before all the operations are completed.
2. When we use transaction ack for chunk messages, the provided transaction is ignored, resulting in the chunk message actually being acknowledged using the non-transactional ack method.
### Modifications
1. Only end the send operation when receive the last chunk message.
2. Add the check for the transaction when the massage is a chunk message.
RobertIndie pushed a commit that referenced this issue Sep 7, 2023
…nk message (#1069)

Master #1060
### Motivation
1. For the chunk message, we only register the send operation once but end the send operation multiple times when receiving the send response. It will make the transaction can be committed before all the operations are completed.
2. When we use transaction ack for chunk messages, the provided transaction is ignored, resulting in the chunk message actually being acknowledged using the non-transactional ack method.
### Modifications
1. Only end the send operation when receive the last chunk message.
2. Add the check for the transaction when the massage is a chunk message.

(cherry picked from commit 59ef32b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants