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

Bug 1677451 - Implement Memory Distribution in the RLB #1376

Merged
merged 1 commit into from Dec 10, 2020

Conversation

chutten
Copy link
Contributor

@chutten chutten commented Dec 9, 2020

Also, while I was here I added test_get_num_recorded_errors because any metric that records errors should expose this method (heck, maybe all metrics should expose this method for consistency's sake)

Copy link
Contributor

@Dexterp37 Dexterp37 left a comment

Choose a reason for hiding this comment

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

Thank you for doing this! It looks really good overall. There's a couple of things that we might simplify, I left a few comments above.

Would you kindly also add:

  • a changelog entry;
  • docs here?

glean-core/rlb/src/private/memory_distribution.rs Outdated Show resolved Hide resolved
glean-core/rlb/src/private/memory_distribution.rs Outdated Show resolved Hide resolved
glean-core/rlb/src/private/memory_distribution.rs Outdated Show resolved Hide resolved
@chutten chutten force-pushed the bug1677451-memdistRLB branch 5 times, most recently from 5119f94 to f7c41b2 Compare December 9, 2020 15:09
@chutten
Copy link
Contributor Author

chutten commented Dec 9, 2020

RFAL

Copy link
Contributor

@mdboom mdboom left a comment

Choose a reason for hiding this comment

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

r+, wc

```

</div>

Copy link
Contributor

Choose a reason for hiding this comment

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

Also add a link to the Rust API docs in the "Reference" section below.

@Dexterp37 Dexterp37 merged commit 142c0ca into mozilla:main Dec 10, 2020
@chutten chutten deleted the bug1677451-memdistRLB branch December 10, 2020 16:59
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.

None yet

4 participants