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

UI: show partial index in reload status #11913

Merged

Conversation

jayeshchoudhary
Copy link
Contributor

@jayeshchoudhary jayeshchoudhary commented Oct 31, 2023

Description

  • There was a missing case in reload status UI where partial index where not shown
  • The API returns total segments and the segments that index is applied
  • if the index is partial then show a yellow icon which when hovered will show a text shown in the image below.
image

@codecov-commenter
Copy link

codecov-commenter commented Oct 31, 2023

Codecov Report

Merging #11913 (c850208) into master (2e37f2c) will increase coverage by 0.00%.
Report is 6 commits behind head on master.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #11913   +/-   ##
=========================================
  Coverage     61.43%   61.44%           
- Complexity     1146     1147    +1     
=========================================
  Files          2376     2378    +2     
  Lines        128773   128844   +71     
  Branches      19906    19925   +19     
=========================================
+ Hits          79111    79163   +52     
- Misses        43951    43952    +1     
- Partials       5711     5729   +18     
Flag Coverage Δ
custom-integration1 <0.01% <ø> (ø)
integration <0.01% <ø> (ø)
integration1 <0.01% <ø> (ø)
integration2 0.00% <ø> (ø)
java-11 61.39% <ø> (+<0.01%) ⬆️
java-21 61.30% <ø> (+<0.01%) ⬆️
skip-bytebuffers-false 61.41% <ø> (+<0.01%) ⬆️
skip-bytebuffers-true 61.28% <ø> (+<0.01%) ⬆️
temurin 61.44% <ø> (+<0.01%) ⬆️
unittests 61.43% <ø> (+<0.01%) ⬆️
unittests1 46.61% <ø> (-0.05%) ⬇️
unittests2 27.67% <ø> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 34 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jayeshchoudhary
Copy link
Contributor Author

@saurabhd336 @Jackie-Jiang

@Jackie-Jiang Jackie-Jiang added the ui UI related issue label Oct 31, 2023
@Jackie-Jiang
Copy link
Contributor

The change itself looks good. My concern is that we are pulling column level index info for every single segments, which can freeze the browser, or even crash the controller for large tables. IMO we don't need to show the index info when checking the reload status, just the segment count should be good enough.
We can add a separate button to check the index info, which shows the desired index info from table config, and optionally pull some segments index info from servers (we need to limit segments pulled).
Do you know which UI action will pull the column level index info for all segments? We should remove all of them.
cc @walterddr I remember you opened an issue for this problem?

@jayeshchoudhary
Copy link
Contributor Author

jayeshchoudhary commented Oct 31, 2023

@Jackie-Jiang we already replaced that expensive API call with this one
#11793
#11576

@Jackie-Jiang
Copy link
Contributor

Oh nice, so it only returns the count!

@Jackie-Jiang Jackie-Jiang merged commit 9069fe2 into apache:master Oct 31, 2023
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ui UI related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants