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

fix(core/worker_events): overwrite the post method of worker_events #10862

Closed
wants to merge 17 commits into from

Conversation

liverpool8056
Copy link
Contributor

Summary

There is a limit for the size of payload allowed to be send in events lib. In some cases, Kong needs to send big payload. For example DAO:crud rely on events to invalidate cache. In these cases, the size of the payload may exceeds the limit due a large dao entity.
In this PR, the post method of the worker_events is overwritten to take a retry action when we get the 'payload too big' error message, and the retry action is just to re-post the event with a truncated payload so that the subsequent logic that relies on the event could have a chance to be informed rather than unaware at all. Based on this purpose, the retry action is taken and only taken once when the error happens.

The limit for the size of payload is defined inside the overwritten post method as a conventional value.

Checklist

Full changelog

  • [Implement ...]

Issue reference

FTI-4963
KAG-1523

@liverpool8056
Copy link
Contributor Author

liverpool8056 commented May 15, 2023

This one is waiting for a co-related PR Kong/lua-resty-events#28 to be merged and bumped

@liverpool8056 liverpool8056 force-pushed the chore/payload-too-big-in-worker-events branch from 3002db3 to 53e5a92 Compare May 15, 2023 03:18
@liverpool8056 liverpool8056 requested a review from chronolaw May 15, 2023 03:19
@liverpool8056 liverpool8056 marked this pull request as ready for review May 15, 2023 03:19
@liverpool8056 liverpool8056 force-pushed the chore/payload-too-big-in-worker-events branch from 53e5a92 to b146668 Compare May 15, 2023 03:19
@liverpool8056 liverpool8056 changed the title chore/(worker_events): overwrite the post method of worker_events chore(worker_events): overwrite the post method of worker_events May 15, 2023
@liverpool8056 liverpool8056 marked this pull request as draft May 15, 2023 09:24
@chronolaw
Copy link
Contributor

lua-resty-events has bumped to 0.1.5. See: #10883

@liverpool8056 liverpool8056 marked this pull request as ready for review May 18, 2023 06:42
@liverpool8056 liverpool8056 force-pushed the chore/payload-too-big-in-worker-events branch from b146668 to 2da3444 Compare May 18, 2023 06:42
spec/01-unit/28-worker_events_spec.lua Outdated Show resolved Hide resolved
spec/01-unit/28-worker_events_spec.lua Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
kong/global.lua Show resolved Hide resolved
kong/global.lua Show resolved Hide resolved
kong/global.lua Outdated Show resolved Hide resolved
@chronolaw chronolaw marked this pull request as draft May 20, 2023 09:24
@liverpool8056 liverpool8056 force-pushed the chore/payload-too-big-in-worker-events branch from c3a336b to 7a97443 Compare May 22, 2023 04:01
@pull-request-size pull-request-size bot added size/L and removed size/M labels May 22, 2023
@github-actions github-actions bot added the chore Not part of the core functionality of kong, but still needed label May 22, 2023
@liverpool8056 liverpool8056 marked this pull request as ready for review May 22, 2023 08:07
@liverpool8056 liverpool8056 force-pushed the chore/payload-too-big-in-worker-events branch from 335e3ea to 440d025 Compare May 22, 2023 09:10
@github-actions github-actions bot removed the chore Not part of the core functionality of kong, but still needed label May 22, 2023
kong/global.lua Outdated Show resolved Hide resolved
kong/global.lua Outdated Show resolved Hide resolved
kong/global.lua Outdated
if err == PAYLOAD_TOO_BIG_ERR then
if type(data) == "string" then
-- truncate the payload and send it again
data = PAYLOAD_TOO_BIG_ERR .. ", truncated payload: " .. string.sub(data, 1, PAYLOAD_MAX_LEN - LEN)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we send this too-big truncated payload?
Since it is truncated, could it be string.sub(data, 1, 1024) or something else?

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's a valuable thinking for the length. In my feel, any length to be truncated is not convincing enough except that we try our best and keep the content as enough as we can or we can just tell the fact that the payload is truncated without any information about the original payload. And I think try our best would be better.
So that's my opinion, WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Could the event receiver do something with this truncated data? if not, I think it may not make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think whether the receiver could do something or not, it depends on the receiver. The sender just try his best, and the senders don't know the ability and behavior of the receiver.
And since we have such a limit, in my feel like it should be always ok to send a payload like that size anytime.

@liverpool8056 liverpool8056 force-pushed the chore/payload-too-big-in-worker-events branch from cf564c1 to eb7fa18 Compare May 23, 2023 06:37
@chronolaw chronolaw changed the title chore(worker_events): overwrite the post method of worker_events feat(core/worker_events): overwrite the post method of worker_events May 23, 2023
@chronolaw chronolaw changed the title feat(core/worker_events): overwrite the post method of worker_events fix(core/worker_events): overwrite the post method of worker_events May 23, 2023
@liverpool8056 liverpool8056 force-pushed the chore/payload-too-big-in-worker-events branch from 959454b to 99f97b9 Compare May 25, 2023 09:06
@bungle
Copy link
Member

bungle commented May 29, 2023

In a real world, where do we have this issue happening?

The description says:

DAO:crud rely on events to invalidate cache. In these cases, the size of the payload may exceeds the limit due a large dao entity.

But where exactly, and why DAO crud event can be so big? Should we make it a bit more intelligent? E.g. truncate the fields or something? Or fix it at the source of event?

@liverpool8056
Copy link
Contributor Author

liverpool8056 commented May 29, 2023

In a real world, where do we have this issue happening?

For some plugin objects whose fields are not size-limited may be too large. As FTI-4963 suggested, the config field of request-validator plugin is too large for worker_event lib to send events. So large events are basically caused by size-unlimited fields.

Should we make it a bit more intelligent? E.g. truncate the fields or something?

I'm afraid it would be a bit flaky to try to truncate large fields of an entity, fields to be truncated might vary with the specific entities.

Or fix it at the source of event?

Fixing it at the source of event means we have to limit the size of fields that are unlimited initially, this would introduce a breaking change which might not such necessary at this moment.

liverpool8056 and others added 17 commits June 5, 2023 10:57
to be sent in events lib. In some cases, Kong needs to send big payload.
For example dao:crud rely on events to invalidate cache. In these cases,
the size of the payload may exceeds the limit due to a large dao entity.
In this PR, the `post` method of the worker_events is overwritten to
take a retry action when we get the 'payload too big' error message, and
the retry action is just to re-post the event with a truncated payload
so that the subsequent logic that relies on the event could have a
chance to be informed rather than unware at all. Based on this purpose,
the retry action is taken and only takn once when the error happens.

FTI-4963
KAG-1523
@liverpool8056 liverpool8056 force-pushed the chore/payload-too-big-in-worker-events branch from 99f97b9 to deca824 Compare June 5, 2023 02:57
@chronolaw chronolaw added the pr/discussion This PR is being debated. Probably just a few details. label Jun 21, 2023
@hbagdi hbagdi requested a review from chronolaw June 21, 2023 17:59
@liverpool8056 liverpool8056 marked this pull request as draft June 26, 2023 09:13
@liverpool8056
Copy link
Contributor Author

This PR could be closed as we had another approach for this, referring to Kong/lua-resty-events#37

@liverpool8056 liverpool8056 deleted the chore/payload-too-big-in-worker-events branch July 12, 2023 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog pr/discussion This PR is being debated. Probably just a few details. size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants