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

[#1629] fix(operator): ShuffleServer cannot be deleted even though there are no more application. #1630

Merged
merged 4 commits into from
Apr 16, 2024
Merged

[#1629] fix(operator): ShuffleServer cannot be deleted even though there are no more application. #1630

merged 4 commits into from
Apr 16, 2024

Conversation

zhengchenyu
Copy link
Collaborator

What changes were proposed in this pull request?

When parsing json, handle special cases where the value might be NaN

Why are the changes needed?

Fix: #1629

Does this PR introduce any user-facing change?

No.

How was this patch tested?

test in real cluster and unit test

@zhengchenyu zhengchenyu changed the title [#1629] fix(operator): ShuffleSever cannot be deleted even though the… [#1629] fix(operator): ShuffleSever cannot be deleted even though there are no more application. Apr 9, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 9, 2024 •
edited
Loading

Test Results

 2 363 files  ±0   2 363 suites  ±0   4h 30m 41s ⏱️ -42s
   912 tests ±0     911 ✅ ±0   1 💤 ±0  0 ❌ ±0 
10 585 runs  ±0  10 571 ✅ ±0  14 💤 ±0  0 ❌ ±0 

Results for commit 6f8d9c5. ± Comparison against base commit 3ea3aaa.

♻️ This comment has been updated with latest results.

@jerqi jerqi changed the title [#1629] fix(operator): ShuffleSever cannot be deleted even though there are no more application. [#1629] fix(operator): ShuffleServer cannot be deleted even though there are no more application. Apr 9, 2024
@jerqi jerqi requested review from advancedxy and wangao1236 April 9, 2024 10:29
@jerqi
Copy link
Contributor

jerqi commented Apr 9, 2024

@advancedxy @wangao1236 Could you take a look at this pull request?

@zhengchenyu
Copy link
Collaborator Author

In fact, both prom and json metrics may produce NaN. I'm only describing a situation where the webhook cannot handle NaN when parsing json metrics.

We have two way to solve it.
(1) When we parse metrics, we handle the special case. This is current PR's method. Then fetch metrics tool need change.
(2) We can change NaN to 0. This eliminates the need to modify the code for parsing metrics, but 0 does not mean not a number. (The same goes for inf)

@advancedxy
Copy link
Contributor

In fact, both prom and json metrics may produce NaN.

Hmm, in which case, a NaN metrics value is produced rather than other valid values?

advancedxy
advancedxy reviewed Apr 9, 2024
View reviewed changes
deploy/kubernetes/operator/pkg/webhook/util/util.go Outdated
@@ -163,7 +164,7 @@ type MetricItem struct {
Name string `json:"name"`
LabelNames []string `json:"labelNames"`
LabelValues []string `json:"labelValues"`
Value float32 `json:"value"`
Value interface{} `json:"value"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should use interface{} here. You can define a special type to custom json serialization and deserialization, such as https://stackoverflow.com/a/74179639/1730663

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea!

@zhengchenyu
Copy link
Collaborator Author

zhengchenyu commented Apr 9, 2024 •
edited
Loading

In fact, both prom and json metrics may produce NaN.

Hmm, in which case, a NaN metrics value is produced rather than other valid values?

In general, NaN is returned when there are not enough samples!

@advancedxy
Copy link
Contributor

In general, NaN is returned when there are not enough samples!

OK, good to know.

@rickyma
Copy link
Contributor

rickyma commented Apr 15, 2024

How is the progress here? @zhengchenyu

@rickyma
Copy link
Contributor

rickyma commented Apr 15, 2024

It is OK for review? We are eager to apply this patch. @zhengchenyu @advancedxy

@zuston zuston requested a review from advancedxy April 15, 2024 08:24
@advancedxy
Copy link
Contributor

I will take a look tonight(in Beijing time).

@rickyma
Copy link
Contributor

rickyma commented Apr 15, 2024

Do we support Dashboard?

advancedxy
advancedxy reviewed Apr 15, 2024
View reviewed changes
Copy link
Contributor

@advancedxy advancedxy left a comment

Choose a reason for hiding this comment

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

Generally lgtm, except one minor comment.

deploy/kubernetes/operator/pkg/webhook/util/util.go
type JSONFloat float64

// UnmarshalJSON return the parsed JSONFloat
func (j *JSONFloat) UnmarshalJSON(v []byte) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

To be symmetric, would you mind to add the MarshalJSON method as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, MarshalJSON is not used here. The metric is generated by shuffle server. Even if I unify the format, the output format of shuffle server is not affected.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I know MarshalJson is not used here right now. But it might be used in the operator in the future for various reasons. It would be surprise or cause response failure in the operator when potentially serializing float with NaN. Since we already support deserializing float with NaN, I think it would nice to add serialize support to avoid any potential gochas.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK! Done!

advancedxy
advancedxy approved these changes Apr 16, 2024
View reviewed changes
Copy link
Contributor

@advancedxy advancedxy left a comment

Choose a reason for hiding this comment

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

LGTM, pending CI passes.

Thanks @zhengchenyu .

@advancedxy advancedxy merged commit b4c92b8 into apache:master Apr 16, 2024
42 checks passed
@advancedxy
Copy link
Contributor

Merged, thank you @zhengchenyu.

jerqi pushed a commit that referenced this pull request Apr 30, 2024
@zhengchenyu
[#1629] fix(operator): Support parsing NaN float value in metrics (#1630
92d984b 
)

### What changes were proposed in this pull request?
When parsing json, handle special cases where the value might be NaN

### Why are the changes needed?
Fix: #1629 

### Does this PR introduce _any_ user-facing change?
No.

### How was this patch tested?
test in real cluster and add unit test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wangao1236 wangao1236 wangao1236 approved these changes

@advancedxy advancedxy advancedxy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug] [Operator] ShuffleSever cannot be deleted even though there are no more application.
5 participants
@zhengchenyu @jerqi @advancedxy @rickyma @wangao1236