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

add tooltip and modal for table status #7899

Merged
merged 3 commits into from
Dec 21, 2021
Merged

add tooltip and modal for table status #7899

merged 3 commits into from
Dec 21, 2021

Conversation

jadami10
Copy link
Contributor

@jadami10 jadami10 commented Dec 14, 2021

Description

It's always bothered me how difficult it is to tell why a table with hundreds of segments, even when they're all "good" shows up as "bad". So this adds a tooltip and a modal that explains why.

This is a bit of a big change for what it's doing, so i'm happy for feedback on a better approach.

I need to test this on a bigger cluster internally, but let me know if this is desirable before I do that.
image
image

Upgrade Notes

Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)

  • Yes (Please label as backward-incompat, and complete the section below on Release Notes)

Does this PR fix a zero-downtime upgrade introduced earlier?

  • Yes (Please label this as backward-incompat, and complete the section below on Release Notes)

Does this PR otherwise need attention when creating release notes? Things to consider:

  • New configuration options
  • Deprecation of configurations
  • Signature changes to public methods/interfaces
  • New plugins added or old plugins removed
  • Yes (Please label this PR as release-notes and complete the section on Release Notes)

Release Notes

Documentation

@codecov-commenter
Copy link

codecov-commenter commented Dec 14, 2021

Codecov Report

Merging #7899 (637de11) into master (9c57b4d) will decrease coverage by 2.90%.
The diff coverage is 82.01%.

❗ Current head 637de11 differs from pull request most recent head c2af0e5. Consider uploading reports for the commit c2af0e5 to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##             master    #7899      +/-   ##
============================================
- Coverage     71.31%   68.41%   -2.91%     
+ Complexity     4087     4034      -53     
============================================
  Files          1589     1198     -391     
  Lines         82109    59621   -22488     
  Branches      12267     9176    -3091     
============================================
- Hits          58559    40789   -17770     
+ Misses        19584    15954    -3630     
+ Partials       3966     2878    -1088     
Flag Coverage Δ
integration1 ?
integration2 ?
unittests1 68.41% <82.01%> (+0.11%) ⬆️
unittests2 ?

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

Impacted Files Coverage Δ
...rg/apache/pinot/core/minion/RawIndexConverter.java 0.00% <0.00%> (-56.61%) ⬇️
...ava/org/apache/pinot/spi/plugin/PluginManager.java 55.41% <44.68%> (+6.86%) ⬆️
...ment/creator/impl/DefaultIndexCreatorProvider.java 64.93% <64.93%> (ø)
...t/index/loader/invertedindex/JsonIndexHandler.java 73.33% <66.66%> (+0.45%) ⬆️
...ment/index/readers/DefaultIndexReaderProvider.java 73.68% <73.68%> (ø)
...manager/realtime/LLRealtimeSegmentDataManager.java 47.07% <76.74%> (-24.87%) ⬇️
.../index/loader/invertedindex/RangeIndexHandler.java 44.44% <85.71%> (+3.44%) ⬆️
...ment/creator/impl/SegmentColumnarIndexCreator.java 87.26% <89.74%> (+2.87%) ⬆️
...ent/index/column/PhysicalColumnIndexContainer.java 91.66% <90.90%> (+6.73%) ⬆️
...che/pinot/segment/spi/index/IndexingOverrides.java 96.36% <96.36%> (ø)
... and 631 more

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 9c57b4d...c2af0e5. Read the comment docs.

@Jackie-Jiang
Copy link
Contributor

This is super cool!

I would suggest always putting the whole segment -> instance state map into the tooltip. With in the screenshot I still cannot tell which segment has inconsistent EV vs IS.
E.g.

{
  "segment-0": {
    "server-0": "ONLINE",
    "server-1": "ERROR"
  }
}

Also, when segment count does not match in EV vs IS, it would be helpful to show the mismatching segments in the tooltip

@jadami10
Copy link
Contributor Author

I've updated it to show full diffs.

Here's what it looks like now when they're not equal
image

Here's what it looks like when segments are missing
image
image

I changed the meetupRsvp table to do 2 rows per segment so i could get a bunch of segments, then took the server offline. Here's what it looks like with 258 bad segments in the diff. The lagginess is because mov -> gif was too big, I had to lower it to 3 fps. It's very fast.
bad_segments

@Jackie-Jiang
Copy link
Contributor

This is awesome! @shahsank3t Can you please help review this? The example looks good to me

Copy link
Contributor

@shahsank3t shahsank3t left a comment

Choose a reason for hiding this comment

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

awesome work on this.
+1 Looks good to me!

@Jackie-Jiang Jackie-Jiang merged commit 8c50f8a into apache:master Dec 21, 2021
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.

4 participants