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

feat: kafka Rebalance list #1070

Merged
merged 6 commits into from
Oct 7, 2024
Merged

feat: kafka Rebalance list #1070

merged 6 commits into from
Oct 7, 2024

Conversation

hemahg
Copy link
Contributor

@hemahg hemahg commented Sep 24, 2024 •
edited
Loading

feat: kafka Rebalance list

Kafka rebalance list
Screenshot 2024-10-01 at 11 37 42 AM

optimization Result modal
Screenshot 2024-10-01 at 11 38 14 AM

@hemahg hemahg marked this pull request as draft September 24, 2024 07:38
@hemahg hemahg added the ui label Sep 24, 2024
@MikeEdgar MikeEdgar added this to the 0.4.0 milestone Sep 24, 2024
@hemahg hemahg force-pushed the Rebalancing branch 3 times, most recently from e32e90f to a5ce0e4 Compare September 30, 2024 20:25
@hemahg hemahg marked this pull request as ready for review October 1, 2024 10:44
@hemahg hemahg force-pushed the Rebalancing branch 2 times, most recently from c9d1c71 to b3feddd Compare October 1, 2024 13:00
MikeEdgar
MikeEdgar reviewed Oct 2, 2024
View reviewed changes
Copy link
Member

@MikeEdgar MikeEdgar left a comment

Choose a reason for hiding this comment

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

Can the headers/values for the status counts be centered? The ones for the total and stopped seem to be in a weird position.
image

Similarly for the mode/auto-approval/brokers data:
image

I think if there are no brokers returned by the API (which should be the case when the mode=full), we can display "N/A" or something.

When entries in the proposal report are missing values like the "excluded" labels here, should we put "None" or "-" as a placeholder?
image

Actions when state is "ProposalReady" should be "Approve" and "Refresh". I'll look through the code and see if I can spot why it's not correct.
image

MikeEdgar
MikeEdgar requested changes Oct 2, 2024
View reviewed changes
Copy link
Member

@MikeEdgar MikeEdgar left a comment

Choose a reason for hiding this comment

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

A few suggestions for the options menu

ui/app/[locale]/(authorized)/kafka/[kafkaId]/nodes/rebalances/RebalanceTable.tsx Outdated Show resolved Hide resolved
ui/app/[locale]/(authorized)/kafka/[kafkaId]/nodes/rebalances/RebalanceTable.tsx Show resolved Hide resolved
ui/app/[locale]/(authorized)/kafka/[kafkaId]/nodes/rebalances/RebalanceTable.tsx Outdated Show resolved Hide resolved
MikeEdgar
MikeEdgar requested changes Oct 2, 2024
View reviewed changes
Copy link
Member

@MikeEdgar MikeEdgar left a comment

Choose a reason for hiding this comment

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

When filtering the list and nothing matches the filter, the empty state is shown without a way to clear the filter. Can this view incorporate the search box also?

image

MikeEdgar
MikeEdgar reviewed Oct 2, 2024
View reviewed changes
Copy link
Member

@MikeEdgar MikeEdgar left a comment

Choose a reason for hiding this comment

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

Refreshing the page on the rebalance tab, the "Brokers" header and "Overview"/"Rebalance" tabs are lost:
image

hemahg added 3 commits October 4, 2024 00:32
@hemahg
Rebalance list
465f013 
Signed-off-by: hemahg <[email protected]>
@hemahg
Add refresh modal
ca88cd2 
Signed-off-by: hemahg <[email protected]>
@hemahg
Incorporate review comments
0e6b29a 
Signed-off-by: hemahg <[email protected]>
@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Oct 4, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

MikeEdgar
MikeEdgar approved these changes Oct 4, 2024
View reviewed changes
Copy link
Member

@MikeEdgar MikeEdgar left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @hemahg

@hemahg hemahg merged commit 570345f into main Oct 7, 2024
9 checks passed
@hemahg hemahg deleted the Rebalancing branch October 7, 2024 06:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MikeEdgar MikeEdgar MikeEdgar approved these changes

@riccardo-forina riccardo-forina Awaiting requested review from riccardo-forina

Assignees
No one assigned
Labels
ui
Projects
None yet
Milestone
0.4.0
Development

Successfully merging this pull request may close these issues.
2 participants
@hemahg @MikeEdgar