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

#8970 Minion tab in Pinot UI #8978

Merged
merged 20 commits into from
Jul 12, 2022
Merged

#8970 Minion tab in Pinot UI #8978

merged 20 commits into from
Jul 12, 2022

Conversation

satishwaghela
Copy link
Contributor

@satishwaghela satishwaghela commented Jun 27, 2022 •
edited by npawar
Loading

Closes: #8970

Release notes:
Minion UI tab

@npawar
Copy link
Contributor

npawar commented Jul 3, 2022

Thanks @satishwaghela ! Just played around with this and it's pretty neat! Here's some suggestions:

  1. On Homepage, Move MINIONS after SERVERS (Order of these tables should be Controllers, Brokers, Servers, Minions)

Screen Shot 2022-07-03 at 12 15 41 AM

  1. Is this validation coming from UI or backend? If UI, no need for this restriction in tags for Minions, it can be anything.

Screen Shot 2022-07-03 at 12 18 12 AM

  1. After editing the tag for Minion, I see this table appear below, which should not be there

Screen Shot 2022-07-03 at 12 18 31 AM

  1. Replace Execution Time with “Elapsed Time”. Calculate the value using System.currentTimeMillis() - startTime. Show value in minutes if < 60. In hours + minutes if > 60

Screen Shot 2022-07-03 at 12 27 12 AM

  1. After clicking Schedule Now, show a modal with message “Are you sure you want to schedule this task?” If user clicks YES, then show the Task ID from the response as “ scheduled” and give user an option to dismiss the message

Screen Shot 2022-07-03 at 1 02 11 AM

@npawar
Copy link
Contributor

npawar commented Jul 3, 2022

  1. These buttons have different tooltip text provided in the design doc
  2. Show every unique table name only once (in this case githubEvents_REALTIME should appear only once)
  3. On this page, when clicking “Resume All” show similar modal to the one as used in “Stop All”. Same for Cleanup All.
  4. After clicking “Stop All” immediately call the API to update “Task Queue Status” field (it should become STOPPED as per the API). Same for “Resume All”, call API to get updated status
  5. After calling Stop or Resume, sometimes going back to homepage just keeps spinning

Screen Shot 2022-07-03 at 12 32 42 AM

@npawar
Copy link
Contributor

npawar commented Jul 3, 2022

@joshigaurava @shahsank3t please help review from code perspective. Thanks!

@npawar
Copy link
Contributor

npawar commented Jul 6, 2022

Thanks @satishwaghela . I have last couple of comments

  1. After scheduling task using “Schedule Now”, show different message as per response. If response contains null, show message “Could not schedule task for ”. If response contains a task id, show the Task ID from the response as “ scheduled” and give user an option to dismiss the message (dont auto disappear)
  2. From this page (http://localhost:9000/#/task-queue/MergeRollupTask/tables/githubEvents_OFFLINE) if i click refresh on browser, I see it takes 10-15 seconds to load.
  3. The Start Time is empty for on UI for a task with status IN_PROGRESS, but if I inspect the response, I do see the startTime field.
    Screen Shot 2022-07-05 at 6 42 04 PM

joshigaurava
joshigaurava reviewed Jul 6, 2022
View reviewed changes
pinot-controller/src/main/resources/app/pages/InstanceDetails.tsx Outdated
@@ -68,9 +68,16 @@ type Props = {
const InstanceDetails = ({ match }: RouteComponentProps<Props>) => {
const classes = useStyles();
const {instanceName} = match.params;
let instanceType;
if (instanceName.toLowerCase().startsWith('broker')) {
instanceType = 'BROKER';
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it feasible to declare these as enums/constants?

@npawar
Copy link
Contributor

npawar commented Jul 7, 2022

Still seeing high load time when I refresh this page. Seems to be coming from "info" call?
Screen Shot 2022-07-07 at 11 49 40 AM

@npawar
Copy link
Contributor

npawar commented Jul 7, 2022

also, please take a look at why the Pinot Integration Test fails in the Github Actions. Seems to be throwing some exceptions from ui code

@npawar
Copy link
Contributor

npawar commented Jul 12, 2022 •
edited
Loading

also, please take a look at why the Pinot Integration Test fails in the Github Actions. Seems to be throwing some exceptions from ui code

Seeing this : https://github.com/apache/pinot/runs/7293911715?check_suite_focus=true

 ERROR in /home/runner/work/pinot/pinot/pinot-controller/src/main/resources/app/pages/TaskQueueTable.tsx
[INFO] ./app/pages/TaskQueueTable.tsx
[INFO] [tsl] ERROR in /home/runner/work/pinot/pinot/pinot-controller/src/main/resources/app/pages/TaskQueueTable.tsx(31,31)
[INFO]       TS2307: Cannot find module '../components/useMinionMetadata'.
[INFO] Child HtmlWebpackCompiler:
[INFO]      1 asset
[INFO]     Entrypoint HtmlWebpackPlugin_0 = __child-HtmlWebpackPlugin_0
[INFO]     [0] ./node_modules/html-webpack-plugin/lib/loader.js!./app/index.html 1.43 KiB {0} [built]
[INFO] npm ERR! code ELIFECYCLE
[INFO] npm ERR! errno 2
[INFO] npm ERR! pinot-controller-ui@1.0.0 build: `webpack --mode production`
[INFO] npm ERR! Exit status 2
[INFO] npm ERR! 
[INFO] npm ERR! Failed at the pinot-controller-ui@1.0.0 build script.
[INFO] npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

@npawar npawar marked this pull request as ready for review July 12, 2022 04:32
@codecov-commenter
Copy link

codecov-commenter commented Jul 12, 2022 •
edited
Loading

Codecov Report

Merging #8978 (486dd34) into master (c802786) will increase coverage by 0.20%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #8978      +/-   ##
============================================
+ Coverage     69.78%   69.99%   +0.20%     
- Complexity     4679     4736      +57     
============================================
  Files          1808     1831      +23     
  Lines         94235    96239    +2004     
  Branches      14052    14381     +329     
============================================
+ Hits          65765    67362    +1597     
- Misses        23908    24225     +317     
- Partials       4562     4652      +90
Flag Coverage Δ
integration1 26.41% <ø> (-0.29%) ⬇️
integration2 24.63% <ø> (-0.25%) ⬇️
unittests1 66.85% <ø> (+0.45%) ⬆️
unittests2 15.40% <ø> (+0.51%) ⬆️

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

Impacted Files Coverage Δ
...data/manager/realtime/DefaultSegmentCommitter.java 0.00% <0.00%> (-80.00%) ⬇️
...lanner/partitioning/FieldSelectionKeySelector.java 45.83% <0.00%> (-39.89%) ⬇️
...che/pinot/plugin/stream/kinesis/KinesisConfig.java 65.62% <0.00%> (-34.38%) ⬇️
...core/operator/filter/predicate/PredicateUtils.java 74.07% <0.00%> (-25.93%) ⬇️
.../readers/forward/FixedBitMVForwardIndexReader.java 73.49% <0.00%> (-24.34%) ⬇️
...e/impl/forward/FixedByteMVMutableForwardIndex.java 70.78% <0.00%> (-23.16%) ⬇️
...t/segment/spi/index/reader/ForwardIndexReader.java 46.86% <0.00%> (-22.74%) ⬇️
...ller/helix/core/minion/TaskTypeMetricsUpdater.java 80.00% <0.00%> (-20.00%) ⬇️
...er/api/resources/LLCSegmentCompletionHandlers.java 43.56% <0.00%> (-18.82%) ⬇️
...in/java/org/apache/pinot/spi/utils/StringUtil.java 81.81% <0.00%> (-18.19%) ⬇️
... and 257 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 c802786...486dd34. Read the comment docs.

@joshigaurava
Copy link
Contributor

Looks good!

npawar
npawar approved these changes Jul 12, 2022
View reviewed changes
Copy link
Contributor

@npawar npawar left a comment

Choose a reason for hiding this comment

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

Lgtm functionally!

@npawar
Copy link
Contributor

npawar commented Jul 12, 2022

@klsince i'll merge this for now. if you do have any feedback after you play with it, we can address those separately

@npawar npawar merged commit 25ead8d into apache:master Jul 12, 2022
@npawar npawar added release-notes Referenced by PRs that need attention when compiling the next release notes ui UI related issue labels Jul 12, 2022
@npawar npawar mentioned this pull request Jul 12, 2022
Closed
@jadami10 jadami10 mentioned this pull request Aug 6, 2022
Merged
@Jackie-Jiang Jackie-Jiang mentioned this pull request Apr 7, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@npawar npawar npawar approved these changes

@joshigaurava joshigaurava joshigaurava approved these changes

Assignees
No one assigned
Labels
release-notes Referenced by PRs that need attention when compiling the next release notes ui UI related issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Minion tab in Pinot UI
4 participants
@satishwaghela @npawar @codecov-commenter @joshigaurava