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

[ECO-4650] fix: remove message grouping for Ably Batch calls #180

Merged
merged 1 commit into from
Mar 13, 2024

Conversation

ttypic
Copy link
Contributor

@ttypic ttypic commented Mar 12, 2024

Resolves #171
Jira issue ECO-4650

The kafka connector put every message sent to a single channel in one BatchSpec, meaning we treated them atomically and send them as a single ProtocolMessage.

This causes a problem when the total ProtocolMessage size gets way too big.

That's why now we put every message in its own BatchSpec

@ttypic ttypic force-pushed the ECO-4650/remove-message-grouping-for-batch branch from b8b7336 to b59bb2b Compare March 12, 2024 13:25
@SimonWoolf
Copy link
Member

SimonWoolf commented Mar 12, 2024

@ttypic I assume you meant to tag me & andy as reviewers on this, not assign it to us?

@ttypic
Copy link
Contributor Author

ttypic commented Mar 12, 2024

@SimonWoolf , oh, I am sorry 🙈 miss clicked

@ttypic ttypic force-pushed the ECO-4650/remove-message-grouping-for-batch branch from b59bb2b to 732b03a Compare March 12, 2024 15:36
@github-actions github-actions bot temporarily deployed to staging/pull/180/kafka-connect-ably March 12, 2024 15:43 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/180/kafka-connect-ably-msk-plugin March 12, 2024 15:43 Inactive
@ttypic ttypic force-pushed the ECO-4650/remove-message-grouping-for-batch branch from 732b03a to ede5923 Compare March 12, 2024 18:40
@github-actions github-actions bot temporarily deployed to staging/pull/180/kafka-connect-ably March 12, 2024 18:44 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/180/kafka-connect-ably-msk-plugin March 12, 2024 18:44 Inactive
@ttypic ttypic force-pushed the ECO-4650/remove-message-grouping-for-batch branch from ede5923 to 834e9ec Compare March 12, 2024 18:55
@github-actions github-actions bot temporarily deployed to staging/pull/180/kafka-connect-ably March 12, 2024 18:59 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/180/kafka-connect-ably-msk-plugin March 12, 2024 18:59 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/180/kafka-connect-ably March 12, 2024 20:35 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/180/kafka-connect-ably-msk-plugin March 12, 2024 20:35 Inactive
@ttypic ttypic force-pushed the ECO-4650/remove-message-grouping-for-batch branch from b81b202 to bd964d9 Compare March 12, 2024 20:36
@github-actions github-actions bot temporarily deployed to staging/pull/180/kafka-connect-ably March 12, 2024 20:40 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/180/kafka-connect-ably-msk-plugin March 12, 2024 20:40 Inactive
@ttypic ttypic force-pushed the ECO-4650/remove-message-grouping-for-batch branch from bd964d9 to 80ea287 Compare March 12, 2024 21:48
@github-actions github-actions bot temporarily deployed to staging/pull/180/kafka-connect-ably March 12, 2024 21:53 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/180/kafka-connect-ably-msk-plugin March 12, 2024 21:53 Inactive
@ttypic ttypic changed the title fix: remove message grouping for Ably Batch calls [ECO-4650] fix: remove message grouping for Ably Batch calls Mar 12, 2024
@ttypic ttypic force-pushed the ECO-4650/remove-message-grouping-for-batch branch from 80ea287 to 7c45690 Compare March 12, 2024 22:28
@github-actions github-actions bot temporarily deployed to staging/pull/180/kafka-connect-ably March 12, 2024 22:33 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/180/kafka-connect-ably-msk-plugin March 12, 2024 22:33 Inactive
Copy link
Member

@SimonWoolf SimonWoolf left a comment

Choose a reason for hiding this comment

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

I'm not able to review the code in detail, will leave that for andy, but just looking at the tests it looks like like it successfully resolves the issue 👍

The kafka connector put every message sent to a single channel in one `BatchSpec`, meaning we treated them atomically and send them as a single `ProtocolMessage`.

This causes a problem when the total `ProtocolMessage` size gets way too big.

That's why now we put every message in its own `BatchSpec`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

In a batch request each message should be in its own batchspec
3 participants