You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "hlteoh37 (via GitHub)" <gi...@apache.org> on 2024/10/18 17:23:57 UTC
[PR] [FLINK-36571][web] Fix issue to allow Busy and Backpressure numbers o… [flink]
hlteoh37 opened a new pull request, #25548:
URL: https://github.com/apache/flink/pull/25548
…n Flink dashboard
## What is the purpose of the change
Fix bug so that non-zero numbers of Busy / Backpressure can be shown on Flink dashboard
## Brief change log
- Add check for `!!value` to convert value to boolean
- Add check before setting `dataSkewPercentage`
## Verifying this change
- This change is a trivial rework / code cleanup without any test coverage.
- Manually checked on a job running locally, that non-zero Busy % can be seen.
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): (yes / no)
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (yes / no)
- The serializers: (yes / no / don't know)
- The runtime per-record code paths (performance sensitive): (yes / no / don't know)
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (yes / no / don't know)
- The S3 file system connector: (yes / no / don't know)
## Documentation
- Does this pull request introduce a new feature? (yes / no)
- If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36571][web] Fix issue to allow Busy and Backpressure numbers o… [flink]
Posted by "flinkbot (via GitHub)" <gi...@apache.org>.
flinkbot commented on PR #25548:
URL: https://github.com/apache/flink/pull/25548#issuecomment-2422968536
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "58b62ff0a4d0e719fe900ca09ebaef3710b07118",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "58b62ff0a4d0e719fe900ca09ebaef3710b07118",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 58b62ff0a4d0e719fe900ca09ebaef3710b07118 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36571][web] Fix issue to allow Busy and Backpressure numbers o… [flink]
Posted by "pnowojski (via GitHub)" <gi...@apache.org>.
pnowojski commented on PR #25548:
URL: https://github.com/apache/flink/pull/25548#issuecomment-2431122684
Thanks for fixing it! I was wondering what has broken that.
@xintongsong @Laffery , can one of you take a look at this? It was caused by a change that you committed a while back and I don't know enough about JS/TS to do a proper review :(
@hlteoh37 if this won't get reviewed soon, please remind me I will merge it as is trusting your manual tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36571][web] Fix issue to allow Busy and Backpressure numbers o… [flink]
Posted by "hlteoh37 (via GitHub)" <gi...@apache.org>.
hlteoh37 commented on PR #25548:
URL: https://github.com/apache/flink/pull/25548#issuecomment-2437121774
@flinkbot run azure
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36571][web] Fix issue to allow Busy and Backpressure numbers o… [flink]
Posted by "hlteoh37 (via GitHub)" <gi...@apache.org>.
hlteoh37 commented on PR #25548:
URL: https://github.com/apache/flink/pull/25548#issuecomment-2437902849
@flinkbot run azure
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36571][web] Fix issue to allow Busy and Backpressure numbers o… [flink]
Posted by "pnowojski (via GitHub)" <gi...@apache.org>.
pnowojski merged PR #25548:
URL: https://github.com/apache/flink/pull/25548
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36571][web] Fix issue to allow Busy and Backpressure numbers o… [flink]
Posted by "dannycranmer (via GitHub)" <gi...@apache.org>.
dannycranmer commented on PR #25548:
URL: https://github.com/apache/flink/pull/25548#issuecomment-2432049223
I hear @Airblader is a dab hand at TS
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36571][web] Fix issue to allow Busy and Backpressure numbers o… [flink]
Posted by "pnowojski (via GitHub)" <gi...@apache.org>.
pnowojski commented on PR #25548:
URL: https://github.com/apache/flink/pull/25548#issuecomment-2431111290
@flinkbot run azure
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36571][web] Fix issue to allow Busy and Backpressure numbers o… [flink]
Posted by "Laffery (via GitHub)" <gi...@apache.org>.
Laffery commented on code in PR #25548:
URL: https://github.com/apache/flink/pull/25548#discussion_r1818406256
##########
flink-runtime-web/web-dashboard/src/app/components/dagre/components/node/node.component.ts:
##########
@@ -82,7 +84,7 @@ export class NodeComponent {
}
isValid = (value?: number): boolean => {
- return value === undefined || value === 0 || isNaN(value);
+ return !!value || value === undefined || value === 0 || isNaN(value);
Review Comment:
I checked the earliest commits, and the original logic considered `undefined` invalid
```diff
isValid = (value?: number): boolean => {
- return !!value || value === 0 || value === NaN;
+ return value === undefined || value === 0 || isNaN(value);
}
```
So here's what I think should be changed:
```diff
isValid = (value?: number): boolean => {
- return value === undefined || value === 0 || isNaN(value);
+ return !!value || value === 0 || isNaN(value);
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36571][web] Fix issue to allow Busy and Backpressure numbers o… [flink]
Posted by "hlteoh37 (via GitHub)" <gi...@apache.org>.
hlteoh37 commented on code in PR #25548:
URL: https://github.com/apache/flink/pull/25548#discussion_r1819083958
##########
flink-runtime-web/web-dashboard/src/app/components/dagre/components/node/node.component.ts:
##########
@@ -82,7 +84,7 @@ export class NodeComponent {
}
isValid = (value?: number): boolean => {
- return value === undefined || value === 0 || isNaN(value);
+ return !!value || value === undefined || value === 0 || isNaN(value);
Review Comment:
We need to check for undefined, as there has been changes in TS to make it such that we cannot pass `undefined` into `isNaN`.
[CI link](https://dev.azure.com/apache-flink/apache-flink/_build/results?buildId=63355&view=logs&j=52b61abe-a3cc-5bde-cc35-1bbe89bb7df5&t=54421a62-0c80-5aad-3319-094ff69180bb)
```
13:14:46.202 [INFO] Error: src/app/components/dagre/components/node/node.component.ts:87:44 - error TS2345: Argument of type 'number | undefined' is not assignable to parameter of type 'number'.
13:14:46.202 [INFO] Type 'undefined' is not assignable to type 'number'.
13:14:46.202 [INFO]
13:14:46.202 [INFO] 87 return !!value || value === 0 || isNaN(value);
```
We should be ok with `undefined` being valid as we handle it in the `prettyPrint()`
https://github.com/apache/flink/blob/master/flink-runtime-web/web-dashboard/src/app/components/dagre/components/node/node.component.ts#L166-L172
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36571][web] Fix issue to allow Busy and Backpressure numbers o… [flink]
Posted by "pnowojski (via GitHub)" <gi...@apache.org>.
pnowojski commented on PR #25548:
URL: https://github.com/apache/flink/pull/25548#issuecomment-2449322691
> CI seems to be failing because of editing a wiki -.- 99.9% sure it's not related to this change!
That has been fixed on master, so rebasing would have fixed it. But anyway, I'm merging it as it is :) Thanks for the fix and thanks @Laffery for the review!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36571][web] Fix issue to allow Busy and Backpressure numbers o… [flink]
Posted by "hlteoh37 (via GitHub)" <gi...@apache.org>.
hlteoh37 commented on PR #25548:
URL: https://github.com/apache/flink/pull/25548#issuecomment-2437865377
CI seems to be failing because of editing a wiki -.- 99.9% sure it's not related to this change!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org