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

[FLINK-31471] Allow setting JobResourceRequirements through WEB UI. #22350

Merged
merged 1 commit into from
Apr 14, 2023

Conversation

dmvk
Copy link
Member

@dmvk dmvk commented Apr 4, 2023

@dmvk dmvk requested a review from zentol April 4, 2023 15:29
@flinkbot
Copy link
Collaborator

flinkbot commented Apr 4, 2023

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

@dmvk
Copy link
Member Author

dmvk commented Apr 5, 2023

Thanks for the review Chesnay, I've addressed the comments and scheduled some follow-ups. PTAL

Copy link
Contributor

@zentol zentol left a comment

Choose a reason for hiding this comment

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

There are some usability issues here:

  1. errors about exceeding max parallelism are not reported to the user or handled by the UI
  2. Scale column should be closer to the parallelism column since it modifies that
  3. +1/-1 buttons are neat for demos, but annoying if you go even beyond a few slots
    3.a) Maybe the parallelism column could be changed to a textbox where a user can enter a parallelism
  4. Until a restart we show the user-configured target parallelism in the parallelism column, but after a restart this is gone and goes back to the actual parallelism.

@dmvk
Copy link
Member Author

dmvk commented Apr 5, 2023

For the UX comments: I'm not a UX person, and this is good enough for the demo purpose (it was not meant for serious usage, that's what REST API is for). We touched on this a few weeks back with @knaufk, and the plan is to reach out to the ML with the demo and see if any UX-proficient person volunteers to improve this.

WDYT? I can, of course, try to pull something off, but there is certainly a limit on how good I can make this.

errors about exceeding max parallelism are not reported to the user or handled by the UI

👍 I'll look into that.

@zentol
Copy link
Contributor

zentol commented Apr 6, 2023

WDYT? I can, of course, try to pull something off, but there is certainly a limit on how good I can make this.

That plan sounds fine to me. Let's file tickets for the issues I've listed.

@dmvk
Copy link
Member Author

dmvk commented Apr 12, 2023

@zentol The error handling is now fixed if you want to try it out.

Screenshot 2023-04-12 at 10 42 13

Copy link
Contributor

@zentol zentol left a comment

Choose a reason for hiding this comment

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

Nice, error messages are now shown properly.

(well, if "properly" includes a giant pointless stack trace 🤷)

flink-runtime-web/web-dashboard/src/app/app.interceptor.ts Outdated Show resolved Hide resolved
@dmvk
Copy link
Member Author

dmvk commented Apr 12, 2023

(well, if "properly" includes a giant pointless stack trace 🤷)

yeah, it's not optimal 😢 that's a different issue to address though

@dmvk dmvk merged commit 40d2cfb into apache:master Apr 14, 2023
@@ -96,13 +105,19 @@ public static RestHandlerConfiguration fromConfiguration(Configuration configura

final boolean webSubmitEnabled = configuration.getBoolean(WebOptions.SUBMIT_ENABLE);
final boolean webCancelEnabled = configuration.getBoolean(WebOptions.CANCEL_ENABLE);
final boolean webRescaleSupported =
ClusterOptions.isAdaptiveSchedulerEnabled(configuration)
&& !ClusterOptions.isReactiveModeEnabled(configuration);
Copy link

Choose a reason for hiding this comment

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

Why cannot change the parallelism of a job running in reactive mode

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants