Skip to content

[YUNIKORN-3111] Limit event size to 1024#1026

Closed
kousei47747 wants to merge 6 commits into
apache:masterfrom
kousei47747:YUNIKORN-3111
Closed

[YUNIKORN-3111] Limit event size to 1024#1026
kousei47747 wants to merge 6 commits into
apache:masterfrom
kousei47747:YUNIKORN-3111

Conversation

@kousei47747

Copy link
Copy Markdown

What is this PR for?

Truncate event message to 1024 characters for k8s compatibility.

What type of PR is it?

  • - Bug Fix
  • - Improvement
  • - Feature
  • - Documentation
  • - Hot Fix
  • - Refactoring

What is the Jira issue?

https://issues.apache.org/jira/browse/YUNIKORN-3111

@pbacsko
pbacsko self-requested a review August 11, 2025 18:15

@pbacsko pbacsko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@kousei47747 please add a short unit test (less than, equal, greater than than 1024) - it doesn't have to be one for AddEvent(), it can be one for truncateEventMessage().

@codecov

codecov Bot commented Aug 11, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.69%. Comparing base (fdd33d2) to head (7a48ce2).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1026      +/-   ##
==========================================
+ Coverage   82.66%   82.69%   +0.02%     
==========================================
  Files          98       98              
  Lines       15694    15703       +9     
==========================================
+ Hits        12974    12986      +12     
+ Misses       2444     2442       -2     
+ Partials      276      275       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@kousei47747

Copy link
Copy Markdown
Author

Hi @pbacsko, thanks for reviewing! I've added the unit test.

@kousei47747
kousei47747 requested a review from pbacsko August 11, 2025 18:41
Comment thread pkg/events/event_system_test.go Outdated
},
{
name: "message length exactly 1024 characters",
message: "Lorem ipsum dolor sit amet consectetur adipiscing elit. Quisque faucibus ex sapien vitae pellentesque sem placerat. In id cursus mi pretium tellus duis convallis. Tempus leo eu aenean sed diam urna tempor. Pulvinar vivamus fringilla lacus nec metus bibendum egestas. Iaculis massa nisl malesuada lacinia integer nunc posuere. Ut hendrerit semper vel class aptent taciti sociosqu. Ad litora torquent per conubia nostra inceptos himenaeos. Lorem ipsum dolor sit amet consectetur adipiscing elit. Quisque faucibus ex sapien vitae pellentesque sem placerat. In id cursus mi pretium tellus duis convallis. Tempus leo eu aenean sed diam urna tempor. Pulvinar vivamus fringilla lacus nec metus bibendum egestas. Iaculis massa nisl malesuada lacinia integer nunc posuere. Ut hendrerit semper vel class aptent taciti sociosqu. Ad litora torquent per conubia nostra inceptos himenaeos. Lorem ipsum dolor sit amet consectetur adipiscing elit. Quisque faucibus ex sapien vitae pellentesque sem placerat. In id cursus mi pretium tellus d",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I suggest a more simplistic approach. Just use a helper function which generates a string (eg. "xxxxxx") with the specifed number of characters (getTestString(5) --> "xxxxx"). Also, compare the contents as well, not just the length.

@pbacsko pbacsko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Avoid long literals, use a helper function.

Comment thread pkg/events/event_system_test.go Outdated
@pbacsko pbacsko closed this in 7610125 Aug 14, 2025
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 this pull request may close these issues.

2 participants