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.

Sign up for GitHub

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

Jump to bottom

Improved pool stats handling #15764

Merged
merged 10 commits into from
Jul 17, 2023
Merged

Improved pool stats handling #15764

merged 10 commits into from
Jul 17, 2023

Conversation

ChiaMineJP
Copy link
Contributor

This PR is a partial PR cut from cmj.plot2_0 branch.

@ChiaMineJP ChiaMineJP requested a review from a team as a code owner July 14, 2023 00:42
@ChiaMineJP ChiaMineJP self-assigned this Jul 14, 2023
@ChiaMineJP ChiaMineJP added the Changed Required label for PR that categorizes merge commit message as "Changed" for changelog label Jul 14, 2023
@ChiaMineJP
Copy link
Contributor Author

I moved strip_old_entries to chia/farmer/farmer.py in order to prevent circular dependency.

@arvidn
Copy link
Contributor

arvidn commented Jul 14, 2023

can you describe what this patch does (more specific than "improve"), and do you intend to add test coverage? If not, is there a good reason to skip test coverage of this code?

@ChiaMineJP
Copy link
Contributor Author

The primary change of this PR is adding pool stats items like valid_partials_since_start, valid_partials_24h, etc.
There are several similar stat items and the mechanism to maintain 24h life time cycle for that items is almost the same.
So I added increment_pool_stat method.

arvidn
arvidn reviewed Jul 14, 2023
View reviewed changes
chia/farmer/farmer.py Show resolved Hide resolved
chia/farmer/farmer.py Outdated Show resolved Hide resolved
chia/farmer/farmer.py Outdated Show resolved Hide resolved
chia/farmer/farmer.py Outdated Show resolved Hide resolved
chia/farmer/farmer.py Outdated Show resolved Hide resolved
@ChiaMineJP ChiaMineJP requested a review from arvidn July 16, 2023 20:02
arvidn
arvidn reviewed Jul 17, 2023
View reviewed changes
tests/farmer_harvester/test_farmer.py Show resolved Hide resolved
tests/farmer_harvester/test_farmer.py Show resolved Hide resolved
tests/farmer_harvester/test_farmer.py Show resolved Hide resolved
tests/farmer_harvester/test_farmer.py Show resolved Hide resolved
tests/farmer_harvester/test_farmer.py Show resolved Hide resolved
tests/farmer_harvester/test_farmer.py Show resolved Hide resolved
tests/farmer_harvester/test_farmer.py Show resolved Hide resolved
tests/farmer_harvester/test_farmer.py Show resolved Hide resolved
tests/farmer_harvester/test_farmer.py Show resolved Hide resolved
@emlowe
Copy link
Contributor

emlowe commented Jul 17, 2023

Coverage exemption accepted - there are long standing gaps in testing pool protocol errors and in FarmerRpcApi. This might be a good target for code coverage tech debt

@wallentx wallentx merged commit e772e4f into main Jul 17, 2023
187 of 188 checks passed
@wallentx wallentx deleted the cmj.plot2-2 branch July 17, 2023 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arvidn arvidn arvidn left review comments

@emlowe emlowe emlowe approved these changes

@xdustinface xdustinface Awaiting requested review from xdustinface

@jack60612 jack60612 Awaiting requested review from jack60612

Assignees

@ChiaMineJP ChiaMineJP

Labels
Changed Required label for PR that categorizes merge commit message as "Changed" for changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ChiaMineJP @arvidn @emlowe @wallentx