-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
[Fix][connector-rocketmq] commit a correct offset to broker & reduce ThreadInterruptedException log #6668
Conversation
…ThreadInterruptedException log
Thanks @YalikWang for created this PR! Is it possible to add some test cases for these bugs? |
Yes, I am willing to add some test cases. e2e or unit test ?Do you have any additional suggestions about it ? |
E2E would be better. Because we should check offset in rocketmq after connector read data, this is only way to make sure same bug not happen again. |
I am trying. The reason for this failure is that I did not set up the correct consumer.group. I fixed it today, but the problem still exists: In E2E testing, when the batch job ends, the consume offset cannot be read through RocketmqAdminApi. I hope it can be resolved tomorrow。 |
@Hisoka-X
And this is the consumerOffset.json in rocektmq broker container (4.9.4): cat consumerOffset.json
{
"offsetTable":{
"test_topic_text_offset@SeaTunnel-Consumer-Group":{0:0,1:0,2:0,3:0
}
}
} This is topic status in rocektmq broker container (4.9.4):
If I call the function to submit offset when there is no more element (RocketMqSourceReader#pollNext) or reader close (RocketMqSourceReader#pollNext), it does work. But I'm not sure if this modification is appropriate. Could you give me some advice about it ? |
In batch mode, this is a known issue, we can skip it for now. It should work fine in zeta
Offset only can be submited in |
I have rolled back the submission regarding rocketmq consume offset verification in the e2e test, and now the CI has passed. Perhaps in the future, we can add this check in e2e test. |
How about only verify it in the zeta? |
I added an e2e test case to check the consumption offset, which only runs with zeta. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Over all LGTM.
…ThreadInterruptedException log (apache#6668)
Purpose of this pull request
Fix issue 6645 and 6665.
Reduce ThreadInterruptedException log print on the console.
Commit a correct offset to the broker when snapshot.
Does this PR introduce any user-facing change?
No
How was this patch tested?
test locally and test with existed test cases
Check list
New License Guide
release-note
.