Extend histogram support #256
Conversation
| first_time = True | ||
| histsum: List[int] = []; | ||
| for msp in metaslabs: | ||
| if msp.ms_sm == sdb.get_typed_null(msp.ms_sm.type_): |
There was a problem hiding this comment.
It looks like this is the canonical way of doing this, but it seems pretty tricky just to check for null. As an orthogonal cleanup, maybe we should change this helper function. It looks like all the callers of get_typed_null(type) could be changed to a simpler-to-use helper, something like:
def is_null(obj: drgn.Object) -> bool:
return obj == drgn.NULL(prog, obj.type_)There was a problem hiding this comment.
Yup, I will file a new bug for this.
sdimitro
left a comment
sdimitro
left a comment
There was a problem hiding this comment.
There are some issues with pylint, yapf, and mypy as you can see in the automation. If you want to deal with those locally you can install these utilities through pip the same way we do it in the automation (ref: https://github.com/delphix/sdb/blob/master/.github/workflows/main.yml) - keep in mind that for certain utilities like mypy we use a very specific version (i.e. not the latest).
Furthermore, now that you added support for spa -mH it would be cool to add a regression test case for it too.
Finally, Matt's idea of changing the null-check wrapper is a good one but could span multiple files that are unrelated to this change. If you feel like picking it up you can do this as a separate commit, otherwise feel free to file a bug and assign it to me.
Codecov Report
@@ Coverage Diff @@
## master #256 +/- ##
==========================================
+ Coverage 87.41% 87.51% +0.09%
==========================================
Files 60 60
Lines 2496 2524 +28
==========================================
+ Hits 2182 2209 +27
- Misses 314 315 +1
Continue to review full report at Codecov.
|
sdimitro
left a comment
sdimitro
left a comment
There was a problem hiding this comment.
LGTM
make sure to squash you commits before merging
This commit removes the need to provide the -v flag for per-metaslab histograms. They can now be obtained with only the -mH flags. Additionally, this commit adds "rolled up" per-vdev histogram data when the -v flag is provided with the -H flag.
4 participants
This commit removes the need to provide the -v flag for per-metaslab
output. For example, histograms can now be obtained with only the -mH flags.
Additionally, this commit adds "rolled up" per-vdev histogram data
when the -v flag is provided with the -H flag.
Pull request checklist
Please check if your PR fulfills the following requirements:
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
Historgram data (-H) is only available at the pool level or metaslab level. There is no available histogram data at the vdev level.
The -m flag (request metaslab data) has no impact at the pool level unless requested with the -v flag. Requesting metaslab data at the pool level should implicitly request a vdev traversal.
Issue Number: N/A
What is the new behavior?
Does this introduce a breaking change?
Other information