[FLINK-38901][runtime-web] Introduce the Rescales/Configuration sub-page for streaming jobs with the adaptive scheduler enabled#27826
Conversation
|
Hi, @RocMarshal! |
RocMarshal
left a comment
There was a problem hiding this comment.
Good job! @och5351
Left a few of comments. PTAL in your free time.
| const checkpointIndex = this.checkpointIndexOfNav(); | ||
| if (data.plan.type == 'STREAMING' && checkpointIndex == -1) { | ||
| this.listOfNavigation.splice(this.checkpointIndexOfNavigation, 0, { | ||
| path: 'checkpoints', | ||
| title: 'Checkpoints' | ||
| }); | ||
| } else if (data.plan.type == 'BATCH' && index > -1) { | ||
| this.listOfNavigation.splice(index, 1); | ||
| } else if (data.plan.type == 'BATCH' && checkpointIndex > -1) { | ||
| this.listOfNavigation.splice(checkpointIndex, 1); | ||
| } |
There was a problem hiding this comment.
The change could be committed as a alone commit.
There was a problem hiding this comment.
I wasn't sure how to split the commit message appropriately, so I created a sub-issue [FLINK-39325] under [FLIP-487] for the rescales tab logic and committed it separately with the new ticket number.
There was a problem hiding this comment.
@och5351 It should be sufficient to use a separate commit with the [hotfix] prefix here, since this is just a minor variable naming issue, right?
BTW, the change of the hotfix commit should contain the checkpointIndex->checkpointNavIndex related naming only.
There was a problem hiding this comment.
Hi, @RocMarshal !
Thank you for your review.
I have applied the fix for[hotfix] Dynamically show/hide rescales tab.
Could you please take a look and review it again?
flink-runtime-web/web-dashboard/src/app/pages/job/job-detail/status/job-status.component.ts
Outdated
Show resolved
Hide resolved
flink-runtime-web/web-dashboard/src/app/pages/job/job-detail/status/job-status.component.ts
Outdated
Show resolved
Hide resolved
| disabledInterval = 0x7fffffffffffffff; | ||
|
|
||
| public rescalesConfig?: RescalesConfig; | ||
| public jobDetail: JobDetailCorrect; |
There was a problem hiding this comment.
Could the JobDetailCorrect be JobDetail?
There was a problem hiding this comment.
Hi, @och5351 If I understand correctly, this class is used here only to obtain information such as the job ID, job type, and scheduler type.
However, JobDetailCorrect provides a much broader range of information than this scope. In addition, when it comes to displaying the detailed information for each rescale in the future, reusing JobDetailCorrect does not seem to be an appropriate choice.
Please let me know that's your opinon.
There was a problem hiding this comment.
Hi, @RocMarshal
You're right that currently we only use job ID, type, and scheduler type from the jobDetail. However, I used JobDetailCorrect because:
- [1]JobLocalService.jobDetailChanges() returns Observable, so using JobDetail would cause a type mismatch
- The job-checkpoints.component.ts follows the same pattern for consistency
- The jobDetail here serves as job metadata (scheduler, type, etc.), while future rescale-specific details would come from RescalesConfig
Would you prefer changing JobLocalService to return JobDetail instead, or keeping the current approach for consistency with other components?
[1] flink-runtime-web/web-dashboard/src/app/pages/job/job-local.service.ts
There was a problem hiding this comment.
Thanks @och5351 for the clarification.
I had a try on JobDetailCorrect -> JobDetail, It worked.
If the code logic does not rely on the extensions made to the plan property in JobDetailCorrect(i.e., it does not use plan.streamNodes, plan.streamLinks, or plan.nodesas NodesItemCorrect[]), then this usage is type-safe and appropriate.
Pls correct me if wrong.
There was a problem hiding this comment.
Thank you @RocMarshal!
You're absolutely right. I tested the JobDetailCorrect → JobDetail change and it works correctly.
I really appreciate your code analysis - it helped me understand the type hierarchy better and I learned a lot from your insights.
I've made the changes and rebased the commits. Could you please take another look?
e3c665e to
00e4b62
Compare
| const checkpointNavIndex = this.checkpointIndexOfNav(); | ||
| if (data.plan.type == 'STREAMING' && checkpointNavIndex == -1) { |
There was a problem hiding this comment.
Sorry for the lack of clarity in my previous message.
What I meant is that the current changes are primarily aimed at adding the rescale navigation button and the rescales/configuration subpage.
However, it is now apparent that this also includes renaming checkpoint-related variables.
I prefer that each patch has changes that are as single-purpose or have a single responsibility as possible.
If you also prefer this approach, we should split the renaming of checkpoint-related variables into a separate hotfix like [hotfix][runtime-web] Polish the checkpoint related variables naming.
The other changes can remain in the commit for FLINK-38901.
| disabledInterval = 0x7fffffffffffffff; | ||
|
|
||
| public rescalesConfig?: RescalesConfig; | ||
| public jobDetail: JobDetailCorrect; |
There was a problem hiding this comment.
Thanks @och5351 for the clarification.
I had a try on JobDetailCorrect -> JobDetail, It worked.
If the code logic does not rely on the extensions made to the plan property in JobDetailCorrect(i.e., it does not use plan.streamNodes, plan.streamLinks, or plan.nodesas NodesItemCorrect[]), then this usage is type-safe and appropriate.
Pls correct me if wrong.
| <td *ngIf="rescalesConfig['rescaleHistoryMax'] === -1"></td> | ||
| <td *ngIf="rescalesConfig['rescaleHistoryMax'] !== -1"> | ||
| {{ rescalesConfig['rescaleHistoryMax'] }} |
There was a problem hiding this comment.
It would be better to show the raw value from the response~
Because there will be a default value for the rescaleHistoryMax.
There was a problem hiding this comment.
Thanks, I've applied this change in the last commit.
a571231 to
973a0ee
Compare
…age for streaming jobs with the adaptive scheduler enabled
9ef4694 to
f96e323
Compare



What is the purpose of the change
Brief change log
Adds the 'Rescale' tab and 'Configuration' subpage in relation to
[FLINK-38897][Runtime/REST] Introduce /jobs/:jobid/rescales/config endpoint to REST API#27580.Verifying this change
Environmental preparation
CASE 1. Streaming Job & Adaptive scheduler
CASE 2. Batch Job
# Run batch example ./build-target/bin/flink run -t remote -m localhost:8081 ./build-target/examples/table/WordCountSQLExample.jarCASE 3. Only Streaming job
Does this pull request potentially affect one of the following parts:
@Public(Evolving): (yes / no)Documentation