Skip to content

Conversation

r1viollet
Copy link
Collaborator

@r1viollet r1viollet commented Jul 9, 2025

What does this PR do?:

Add limits to the number of nodes and links.

Motivation:

Ensure we don't go in OOM error in the stresstest.

Additional Notes:

How to test the change?:

For Datadog employees:

  • If this PR touches code that signs or publishes builds or packages, or handles
    credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
  • This PR doesn't touch any of that.
  • JIRA: [JIRA-XXXX]

Unsure? Have a question? Request a review!

Copy link

github-actions bot commented Jul 9, 2025

🔧 Report generated by pr-comment-cppcheck

CppCheck Report

Errors (2)

Warnings (4)

Style Violations (297)

Copy link

github-actions bot commented Jul 9, 2025

🔧 Report generated by pr-comment-scanbuild

public void link(GraphNode node) {
nodes.add(node);
// Simple round-robin replacement - no expensive size checks or polling
int index = (linkIndex++) % MAX_LINKS;
Copy link
Collaborator

Choose a reason for hiding this comment

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

The increment operator is not atomic, unfortunately.
You can use AtomicInteger for the linkIndex instead.

Comment on lines 24 to 26
// Optional: method to access links if needed for verification
public GraphNode getLink(int index) {
return links.get(index % MAX_LINKS);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this used/going to be used soon?

@r1viollet r1viollet force-pushed the r1viollet/avoid_stress_testtimeout branch from 80a2a60 to 9762603 Compare July 9, 2025 15:07
@r1viollet
Copy link
Collaborator Author

Thanks for the feedback,
I had changed the workload too much, I'm trying to iterate on something that is not too different from previous workload

@r1viollet r1viollet marked this pull request as draft July 9, 2025 15:11
@r1viollet r1viollet force-pushed the r1viollet/avoid_stress_testtimeout branch from 9762603 to c58e008 Compare July 9, 2025 15:18
@r1viollet r1viollet marked this pull request as ready for review July 9, 2025 15:18
@r1viollet r1viollet requested a review from jbachorik July 9, 2025 16:08
Copy link
Collaborator

@jbachorik jbachorik left a comment

Choose a reason for hiding this comment

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

Looks reasonable.
Just in case - did you check that the stresstest counters are somewhat comparable before and after this change?

@r1viollet
Copy link
Collaborator Author

This leads to comparable results

@r1viollet r1viollet merged commit 82e550f into main Jul 10, 2025
95 checks passed
@r1viollet r1viollet deleted the r1viollet/avoid_stress_testtimeout branch July 10, 2025 10:08
@github-actions github-actions bot added this to the 1.29.0 milestone Jul 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants