-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
channelz: major cleanup / reorganization #6969
Conversation
pluralize EntryPerPage
remove nil panic potential cleanups
reorganize remove ref type pt1 remove some of findEntry simplifications
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #6969 +/- ##
==========================================
+ Coverage 82.44% 82.45% +0.01%
==========================================
Files 296 299 +3
Lines 31475 31317 -158
==========================================
- Hits 25948 25822 -126
+ Misses 4471 4450 -21
+ Partials 1056 1045 -11
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made a first pass -- which was basically a file by file review. However I might need more iterations here as all of this is new to me.
I believe this needs to be a change not visible to the user. How do we verify that UX doesnt change for channelz here? A channelz example might have been useful to detect the change possibly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like everything LGTM modulo a few comments. PTAL
I discussed with @dfawley offline. The general refactor and the changes look good to me. There is however still more scope of cleanup - which can be categorized as future improvements to channelz in general. LGTM, you can ignore the nit formatting comments from above. Also the branch requires a rebase. |
I've resolved the convos for nits. Could you take a look at the rest and see if makes sense? |
Thanks for the review! I also merged from master ( 🤯) to fix the conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
🤞 |
This reverts commit 55cd7a6.
Several major changes are included:
To be done:
RELEASE NOTES: none