Skip to content

Conversation

@vmtuan12
Copy link
Contributor

@vmtuan12 vmtuan12 commented Jul 3, 2025

Description

Add support for max.request.size and batch.size configuration for Kafka producer of Kafka Event Listener
Fixes #26129

Release notes

(x) Release notes are required, with the following suggested text:

## Kafka Event Listener
* Add support for configuring max request size with the `kafka-event-listener.max-request-size` config property. ({issue}`26129`)
* Add support for configuring batch size with the `kafka-event-listener.batch-size` config property. ({issue}`26129`)

@cla-bot
Copy link

cla-bot bot commented Jul 3, 2025

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

Copy link
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some reivew comments in your older PR. Please address those comments.

@vmtuan12 vmtuan12 force-pushed the vmhtuan/add-kafka-producer-config branch from de6fd23 to a6cda56 Compare July 3, 2025 22:37
@cla-bot
Copy link

cla-bot bot commented Jul 3, 2025

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@github-actions github-actions bot added the docs label Jul 3, 2025
@vmtuan12
Copy link
Contributor Author

vmtuan12 commented Jul 3, 2025

I left some reivew comments in your older PR. Please address those comments.

I have already resolved these comments. Thanks!

@vmtuan12 vmtuan12 force-pushed the vmhtuan/add-kafka-producer-config branch from a6cda56 to d655ca2 Compare July 3, 2025 23:35
@cla-bot
Copy link

cla-bot bot commented Jul 3, 2025

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@vmtuan12 vmtuan12 force-pushed the vmhtuan/add-kafka-producer-config branch from d655ca2 to f2df89f Compare July 4, 2025 03:21
@cla-bot
Copy link

cla-bot bot commented Jul 4, 2025

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@vmtuan12 vmtuan12 force-pushed the vmhtuan/add-kafka-producer-config branch from f2df89f to 6805c6c Compare July 4, 2025 03:38
@cla-bot
Copy link

cla-bot bot commented Jul 4, 2025

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@vmtuan12 vmtuan12 force-pushed the vmhtuan/add-kafka-producer-config branch from 6805c6c to 20fded6 Compare July 4, 2025 03:59
@cla-bot
Copy link

cla-bot bot commented Jul 4, 2025

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@vmtuan12 vmtuan12 requested review from chenjian2664 and ebyhr July 4, 2025 06:04
@ebyhr
Copy link
Member

ebyhr commented Jul 6, 2025

@vmtuan12 Have you already submitted CLA?

@vmtuan12
Copy link
Contributor Author

vmtuan12 commented Jul 6, 2025

@vmtuan12 Have you already submitted CLA?

Yes, I sent the email 5 days ago, but I think it has not been processed. My email is [email protected], can you check that for me?

@vmtuan12 vmtuan12 force-pushed the vmhtuan/add-kafka-producer-config branch from 20fded6 to 45ab732 Compare July 6, 2025 22:54
@cla-bot
Copy link

cla-bot bot commented Jul 6, 2025

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@vmtuan12 vmtuan12 requested a review from ebyhr July 7, 2025 01:27
return batchSize;
}

@ConfigDescription("Attempt to batch records/messages together up to batch.size bytes before sending")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you update the config description? to me it's unclear what is batch.size bytes means?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it means that messages will be buffered before being sent, and sent when the sum of message size >= batch.size, instead of sending every individual message

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the description is modified to "Query events will be buffered, and sent when their total size reaches batch.size", will it be better?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about follow the document part that you described the config seems better

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok thanks! I have editted it

@vmtuan12 vmtuan12 force-pushed the vmhtuan/add-kafka-producer-config branch from 45ab732 to 9fce061 Compare July 8, 2025 01:28
@cla-bot
Copy link

cla-bot bot commented Jul 8, 2025

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@cla-bot cla-bot bot added the cla-signed label Jul 11, 2025
@vmtuan12
Copy link
Contributor Author

@ebyhr please review again

@ebyhr ebyhr force-pushed the vmhtuan/add-kafka-producer-config branch from f7053e9 to 98270f8 Compare July 11, 2025 03:55
Copy link
Contributor

@chenjian2664 chenjian2664 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@ebyhr ebyhr force-pushed the vmhtuan/add-kafka-producer-config branch 2 times, most recently from a3963b9 to 07729fd Compare July 17, 2025 01:04
@chenjian2664
Copy link
Contributor

The commit message : Add support for max.request.size and batch.size configuration Kafka Event Listener should be Add support for max.request.size and batch.size configuration for Kafka Event Listener?

@ebyhr ebyhr force-pushed the vmhtuan/add-kafka-producer-config branch from 07729fd to f45f965 Compare July 17, 2025 01:12
@ebyhr
Copy link
Member

ebyhr commented Jul 17, 2025

That looks too long. I changed to "Support max.request.size and batch.size config in Kafka Event Listener"

@vmtuan12
Copy link
Contributor Author

Can this be merged?

@ebyhr ebyhr force-pushed the vmhtuan/add-kafka-producer-config branch from 72870e0 to 01beda7 Compare July 17, 2025 01:59
@ebyhr
Copy link
Member

ebyhr commented Jul 17, 2025

@vmtuan12 Please use rebase when you apply changes from master. I re-forced pushed.

@vmtuan12
Copy link
Contributor Author

I do not have permission to merge into Master. Can you merge it @ebyhr

@ebyhr ebyhr merged commit e6c903a into trinodb:master Jul 17, 2025
18 checks passed
@github-actions github-actions bot added this to the 477 milestone Jul 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

Kafka Event Listener fails to send logs because max.request.size is fixed at 5242880

3 participants