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.

Sign up for GitHub

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

Jump to bottom

Fix N+1 Issue in Namespace Search and Enhance Tree View #153

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix N+1 Issue in Namespace Search and Enhance Tree View #153

wants to merge 3 commits into from

Conversation

012xx
Copy link

@012xx 012xx commented Feb 17, 2025 •
edited
Loading

Overview

  • Resolved the N+1 problem by optimizing namespace retrieval for faster searches.
  • Fixed tree display indentation using tree symbols.

Fixes

  • Optimized namespace hierarchy retrieval to eliminate redundant queries.
  • Improved tree view formatting by using tree symbols and removing unnecessary indent variables.
  • Sorted namespaces in ascending alphabetical order.

Rationale

  • The current implementation suffers from an N+1 problem when searching namespaces, leading to poor performance.
  • This update improves user experience by significantly speeding up namespace lookups.

@012xx 012xx self-assigned this Feb 17, 2025
@012xx 012xx force-pushed the update-accutrate-list branch from 8d19e3b to 8fda699 Compare February 17, 2025 01:48
takahiro-yamada
takahiro-yamada requested changes Feb 17, 2025
View reviewed changes
@012xx 012xx marked this pull request as draft February 17, 2025 07:30
@012xx 012xx force-pushed the update-accutrate-list branch from 4644dcb to 8fda699 Compare February 17, 2025 07:35
@012xx 012xx marked this pull request as ready for review February 17, 2025 10:41
@012xx 012xx changed the title refactor: optimize namespace listing and tree display logic Fix N+1 Issue in Namespace Search and Enhance Tree View Feb 17, 2025
@012xx 012xx force-pushed the update-accutrate-list branch from 51cac3d to 9809d0a Compare February 17, 2025 12:02
takahiro-yamada
takahiro-yamada previously approved these changes Feb 20, 2025
View reviewed changes
Copy link

@takahiro-yamada takahiro-yamada left a comment

Choose a reason for hiding this comment

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

LGTM

@zoetrope
Could you review this PR as a core maintainer please?

@012xx 012xx requested a review from zoetrope February 20, 2025 05:07
@012xx 012xx force-pushed the update-accutrate-list branch from 907b41e to 15b242d Compare February 21, 2025 09:58
zoetrope
zoetrope reviewed Feb 21, 2025
View reviewed changes
cmd/kubectl-accurate/sub/template_list.go Outdated
if err := o.showNS(ctx, ns.Name, 0); err != nil {
return err
for _, children := range childMap {
sort.Slice(children, func(i, j int) bool {
Copy link
Member

Choose a reason for hiding this comment

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

You are sorting too many times.
allNamespaces.Items is already sorted, there is no need to sort children here.

@012xx 012xx requested a review from zoetrope February 24, 2025 22:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@takahiro-yamada takahiro-yamada takahiro-yamada left review comments

@zoetrope zoetrope Awaiting requested review from zoetrope

At least 1 approving review is required to merge this pull request.

Assignees

@012xx 012xx

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@012xx @zoetrope @takahiro-yamada