Skip to content

Conversation

@mmaybee
Copy link
Contributor

@mmaybee mmaybee commented Feb 25, 2021

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build was run locally and any changes were pushed
  • Lint has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

This PR includes two changes:

  1. The current per-vdev histogram output is generated by "rolling up" the vdev's per-metaslab historgram data. This is unnecessary since the rolled up data is already available in vdev.vdev_mg.mg_histogram.
  2. The current canonical way to check for a null drgn.Object pointer is to use sdb.get_typed_null():

if msp.ms_sm == sdb_get_typed_null(msp.ms_sm.type_):

This is both awkward and confusing.

Issue Number: 258

What is the new behavior?

  1. The per-vdev histogram output is now generated directly from the data found in vdev.vdev_mg.mg_histogram
  2. All drgn.Object null pointer checks have been changed to use the new sdb.is_null() interface:

if sdb.is_null(msp.ms_sm):

Does this introduce a breaking change?

  • Yes
  • No

Streamline the per-vdev histogram reporting by directly using the
data in vdev_mg->mg_histogram rather than "rolling up" the data from
individual metaslab histogram data.
@mmaybee mmaybee requested review from ahrens and sdimitro February 25, 2021 20:33
Addresses issue delphix#258

The function used for a null pointer check is sdb.get_typed_null()
e.g.
	if msp.ms_sm == sdb_get_typed_null(msp.ms_sm.type_):

This is awkward and confusing. The new sdb.is_null() iterface
simplifies the null check:

	if sdb.is_null(msp.ms_sm):
@mmaybee mmaybee force-pushed the cleanup_vdev_histogram branch from f48f694 to 47a9f8a Compare February 25, 2021 22:16
@codecov-io
Copy link

codecov-io commented Feb 25, 2021

Codecov Report

Merging #261 (515f9d2) into master (bdd67b8) will decrease coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #261      +/-   ##
==========================================
- Coverage   87.60%   87.51%   -0.09%     
==========================================
  Files          62       62              
  Lines        2541     2523      -18     
==========================================
- Hits         2226     2208      -18     
  Misses        315      315              
Impacted Files Coverage Δ
sdb/__init__.py 100.00% <100.00%> (ø)
sdb/commands/spl/avl.py 100.00% <100.00%> (ø)
sdb/commands/zfs/dbuf.py 86.11% <100.00%> (ø)
sdb/commands/zfs/histograms.py 98.55% <100.00%> (ø)
sdb/commands/zfs/metaslab.py 95.55% <100.00%> (+0.10%) ⬆️
sdb/commands/zfs/spa.py 97.77% <100.00%> (ø)
sdb/commands/zfs/vdev.py 90.32% <100.00%> (-2.37%) ⬇️
sdb/target.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bdd67b8...515f9d2. Read the comment docs.

W0221: Parameters differ from overridden 'pretty_print' method (arguments-differ)

Occurring at:
sdb/commands/zfs/vdev.py:72
sdb/commands/zfs/metaslab.py:163
sdb/commands/zfs/spa.py:68
sdb/commands/zfs/dbuf.py:84

When overriding methods the arguments must match exactly. Introduced
print_indented() method for Metaslab and Vdev classes.
@mmaybee mmaybee force-pushed the cleanup_vdev_histogram branch from 515f9d2 to 92532a3 Compare February 26, 2021 15:49
@mmaybee mmaybee merged commit 139a925 into delphix:master Feb 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants