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

config: Add GoMemLimit config option and use with 10-node test #5975

Merged
merged 4 commits into from
Jun 12, 2024

Conversation

ohill
Copy link
Contributor

@ohill ohill commented Apr 8, 2024

Summary

This adds a config option GoMemLimit for a soft memory limit like GOMEMLIMIT. It uses it for TestPartitionHalfOffline which runs 10 nodes (TenNodesDistributedMultiWallet.json) for 1GB each. It also uses it for TestSendSigsAfterCatchpointCatchup which starts 3 nodes and has one generate a catchpoint snapshot for the others.

Test Plan

TestPartitionHalfOffline should run 10 nodes without using as much memory, and TestSendSigsAfterCatchpointCatchup should also use less memory.

Copy link

codecov bot commented Apr 8, 2024

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 56.00%. Comparing base (dfd95ff) to head (e0cb8d0).

Files Patch % Lines
cmd/algod/main.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5975   +/-   ##
=======================================
  Coverage   56.00%   56.00%           
=======================================
  Files         481      481           
  Lines       67646    67648    +2     
=======================================
+ Hits        37883    37887    +4     
- Misses      27197    27200    +3     
+ Partials     2566     2561    -5     

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

Copy link
Contributor

@gmalouf gmalouf left a comment

Choose a reason for hiding this comment

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

I think a config option is reasonable enough, understanding we may want to later remove if Golang changes the expected behavior/approach.

Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

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

LGTM but lets merge after p2p branch since it also adds v34 config, and rebasing this smaller PR will be easier.

Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

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

Well although feature/p2p has a conflict so requires manual remerge anyway, lets get it merged.

@algorandskiy algorandskiy changed the title tests: Add GoMemLimit config option and use with 10-node test config: Add GoMemLimit config option and use with 10-node test Jun 12, 2024
@algorandskiy algorandskiy merged commit 9191b1b into algorand:master Jun 12, 2024
19 checks passed
@ohill ohill deleted the GoMemLimit branch June 12, 2024 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants