Skip to content

OAK-11570 oak-run fullGC fullGcMaxAge default value is wrongly calculated#2157

Merged
Joscorbe merged 2 commits intoapache:trunkfrom
daniancu:OAK-11570
Mar 18, 2025
Merged

OAK-11570 oak-run fullGC fullGcMaxAge default value is wrongly calculated#2157
Joscorbe merged 2 commits intoapache:trunkfrom
daniancu:OAK-11570

Conversation

@daniancu
Copy link
Contributor

@daniancu daniancu commented Mar 7, 2025

fixed conversion where default value is converted to millis not seconds

assertTrue(output.contains("FullGcBatchSize is : 1000"));
assertTrue(output.contains("FullGcProgressSize is : 10000"));
assertTrue(output.contains("FullGcMaxAgeInSecs is : 86400"));
assertTrue(output.contains("FullGcMaxAgeInSecs is : 86400\n"));
Copy link
Member

Choose a reason for hiding this comment

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

I assume the \n is to ensure the value is exactly 86400 and not something else containing that string, but I wonder if we should then apply the same to the rest of asserts to maintain consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, the wrong value was 86400000 and was still passing the test
i've updated the same for all numeric values checks

Copy link
Contributor

Choose a reason for hiding this comment

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

This change breaks tests on Windows systems.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpicking: why is there a space before the colons?

Copy link
Contributor

@rishabhdaim rishabhdaim Apr 1, 2025

Choose a reason for hiding this comment

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

This change breaks tests on Windows systems.

Do we need to change this before CF?

Nitpicking: why is there a space before the colons?

I think this has been done to it consistent with other verifications done.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. It should be trivial; I'm on it.

  2. Ack.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Joscorbe Joscorbe merged commit 709fab2 into apache:trunk Mar 18, 2025
1 of 3 checks passed
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.

4 participants