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

std::fmt::Debug for Layer implementations #3536

Merged
merged 3 commits into from
Feb 6, 2023
Merged

std::fmt::Debug for Layer implementations #3536

merged 3 commits into from
Feb 6, 2023

Conversation

koivunej
Copy link
Member

@koivunej koivunej commented Feb 3, 2023

Follow-up to #3513.

This removes the old blanket std::fmt::Debug impl on dyn Layer which did not seem to be used from anywhere (no compilation errors after removing).

Adds std::fmt::Debug requirement and implementations for trait Layer implementors:

  • LayerDescriptor (derived)
  • RemoteLayer (manual)
  • DeltaLayer (manual)
  • ImageLayer (manual)

Adds and adjusts some doc comments to be more rustdoc alike.

In follow-up #3544: Use this std::fmt::Debug when logging a LayerMap::replace failure for which we currently have no tests but speculation (see #3387 comments).

Alternatives for adding the std::fmt::Debug:

  • add a custom Layer::get_type_name(&self) -> &'static str
    • would give no additional information to the place of use except the type name
  • add other custom std::fmt::Debug alike functionality to the trait
    • std::fmt::Debug is the way to debug format objects

@koivunej koivunej requested review from a team as code owners February 3, 2023 13:42
@koivunej koivunej requested review from lubennikovaav and LizardWizzard and removed request for a team February 3, 2023 13:42
Copy link
Contributor

@problame problame left a comment

Choose a reason for hiding this comment

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

Nice job!

@koivunej
Copy link
Member Author

koivunej commented Feb 3, 2023

This should probably wait eviction related stats because we might want to include those.

@koivunej
Copy link
Member Author

koivunej commented Feb 6, 2023

After looking a bit how to use this on the new evict_layer endpoint, I am thinking I'll just separate that to different PR altogether (with the c8b3139).

Copy link
Contributor

@SomeoneToIgnore SomeoneToIgnore left a comment

Choose a reason for hiding this comment

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

I'm a bit sad that it makes it harder (at least, a bigger patch) to go away from the "&dyn Trait as an interface" approach, but otherwise looks good.

@koivunej
Copy link
Member Author

koivunej commented Feb 6, 2023

@SomeoneToIgnore please elaborate how will this make it more difficult? I think the main point is that we should have Debug for Layer impls, surely this is as easy to do with more typed values than not?

@SomeoneToIgnore
Copy link
Contributor

Maybe I'm wrong, just imagined that now every place with Arc<dyn Layer> could be debugged, and for the later swap, more code changes could be required in either of such place.
But it's still noting big, having Debug seems to be a good thing in general.

@koivunej
Copy link
Member Author

koivunej commented Feb 6, 2023

more code changes could be required in either of such place.

Oki now I understood. I think in future it's just important to use #[derive(Debug)] regardless of the shape. Here it cannot yet be used because we could dump a lot of indices for InMemoryLayer and few of those extra fields, perhaps EphmeralFile would had had quite long Debug output as well (the full hashmap of files). EDIT: Forgot there were the tenantid and timelineid as well, and conf. They are everywhere :)

I don't want to drop fields (above discussion on RemoteLayer I'm keeping open) or add newtypes which would suppress any long output (InMemoryLayer's index) to move towards that on this PR but having Debug impls in general will be a good thing.

@koivunej koivunej merged commit 678fe06 into main Feb 6, 2023
@koivunej koivunej deleted the layer-debug branch February 6, 2023 12:21
koivunej added a commit that referenced this pull request Feb 7, 2023
Follow-up to #3536, to actually use the new `Debug` in replacing the
layers, and use replacement with manual eviction endpoint.

Turns out the two paths share a lot of handling of `Replacement` but
didn't unify the two (need 3). There are also upcoming refactorings
from other PRs to this.
koivunej added a commit that referenced this pull request Feb 9, 2023
#3536 added the custom Debug implementations but it using derived Debug
on Key lead to too verbose output. Instead of making `Key`'s `Debug`
unconditionally or conditionally do the `Display` variant (for table
space'd keys), opted to build a newtype to provide `Debug` for
`Range<Key>` via `Display` which seemed to work unconditionally.

Also orders Key to have: 1. comment, 2. derive, 3. `struct Key`.
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.

3 participants