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

Adjust GC related settings for tests #15632

Merged
merged 1 commit into from
Jan 9, 2023

Conversation

arhimondr
Copy link
Contributor

Description

To exclude the possibility of GCLocker and PreventiveGC related issues

Additional context and related issues

#15293

Release notes

(X) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@arhimondr arhimondr requested a review from findepi January 6, 2023 16:48
@cla-bot cla-bot bot added the cla-signed label Jan 6, 2023
@arhimondr arhimondr force-pushed the adjust-gc-settings-for-tests branch from 06bd876 to 1a6cc43 Compare January 6, 2023 18:20
@findepi
Copy link
Member

findepi commented Jan 6, 2023

To exclude the possibility of GCLocker and PreventiveGC related issues

please include this in commit message and/or code comment

@@ -92,6 +92,9 @@
Force bigger region size in attempt to help G1 utilize heap fully. -->
<air.test.jvm.additional-arguments>
-XX:G1HeapRegionSize=32M
-XX:+UnlockDiagnosticVMOptions
-XX:GCLockerRetryAllocationCount=10
Copy link
Member

Choose a reason for hiding this comment

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

only 10?

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 2 by default, I think 10 should be sufficient. Do you think it should be higher?

Copy link
Member

Choose a reason for hiding this comment

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

oh, thought default is about 10. fine.
maybe add <!-- bump from default of 2 --> ?

@@ -92,6 +92,9 @@
Force bigger region size in attempt to help G1 utilize heap fully. -->
<air.test.jvm.additional-arguments>
-XX:G1HeapRegionSize=32M
-XX:+UnlockDiagnosticVMOptions
Copy link
Member

Choose a reason for hiding this comment

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

is it required to set XX:GCLockerRetryAllocationCount ?

Copy link
Contributor Author

@arhimondr arhimondr Jan 6, 2023

Choose a reason for hiding this comment

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

It's actually needed for both 😃

@arhimondr arhimondr force-pushed the adjust-gc-settings-for-tests branch from 1a6cc43 to 7d057be Compare January 6, 2023 21:12
Copy link
Member

@findepi findepi left a comment

Choose a reason for hiding this comment

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

% comments

To exclude the possibility of GCLocker and PreventiveGC related issues
@arhimondr arhimondr force-pushed the adjust-gc-settings-for-tests branch from 7d057be to b7ca58a Compare January 9, 2023 17:13
@arhimondr arhimondr merged commit 163ecac into trinodb:master Jan 9, 2023
@arhimondr arhimondr deleted the adjust-gc-settings-for-tests branch January 9, 2023 18:45
@github-actions github-actions bot added this to the 406 milestone Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants