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

[YUNIKORN-2856] Remove redundant conditional #964

Closed
wants to merge 2 commits into from
Closed

[YUNIKORN-2856] Remove redundant conditional #964

wants to merge 2 commits into from

Conversation

400Ping
Copy link
Contributor

@400Ping 400Ping commented Sep 6, 2024

What is this PR for?

Remove redundant of if function.

What type of PR is it?

  • - Bug Fix
  • - Improvement
  • - Feature
  • - Documentation
  • - Hot Fix
  • - Refactoring

What is the Jira issue?

https://issues.apache.org/jira/browse/YUNIKORN-2856

@ryankert01
Copy link
Contributor

Hi, thanks for your PR. Please resolve syntax error.
Suggestion: you can run ci before you send a PR.

Comment on lines 858 to 861
if len(unescapedQueueName) == 0 {
app = partitionContext.GetApplication(application)
} else {
else {
queueErr := validateQueue(unescapedQueueName)
if queueErr != nil {
buildJSONErrorResponse(w, queueErr.Error(), http.StatusBadRequest)
Copy link
Contributor

Choose a reason for hiding this comment

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

please also refactor, and add test to prove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you mean by this

running unit tests
"go" clean -testcache
"go" test ./... -race -tags deadlock -coverprofile=build/coverage.txt -covermode=atomic
        github.com/apache/yunikorn-core/cmd/schedulerclient             coverage: 0.0% of statements
        github.com/apache/yunikorn-core/cmd/queueconfigchecker          coverage: 0.0% of statements
        github.com/apache/yunikorn-core/cmd/simplescheduler             coverage: 0.0% of statements
ok      github.com/apache/yunikorn-core/pkg/common      3.427s  coverage: 74.1% of statements
ok      github.com/apache/yunikorn-core/pkg/common/configs      3.376s  coverage: 97.8% of statements
ok      github.com/apache/yunikorn-core/pkg/common/resources    5.427s  coverage: 100.0% of statements
?       github.com/apache/yunikorn-core/pkg/handler     [no test files]
        github.com/apache/yunikorn-core/pkg/examples            coverage: 0.0% of statements
        github.com/apache/yunikorn-core/pkg/events/mock         coverage: 0.0% of statements
ok      github.com/apache/yunikorn-core/pkg/common/security     8.745s  coverage: 96.8% of statements
ok      github.com/apache/yunikorn-core/pkg/entrypoint  2.559s  coverage: 82.1% of statements
        github.com/apache/yunikorn-core/pkg/mock                coverage: 0.0% of statements
ok      github.com/apache/yunikorn-core/pkg/events      3.868s  coverage: 97.4% of statements
?       github.com/apache/yunikorn-core/pkg/rmproxy/rmevent     [no test files]
        github.com/apache/yunikorn-core/pkg/rmproxy             coverage: 0.0% of statements
ok      github.com/apache/yunikorn-core/pkg/locking     5.329s  coverage: 51.1% of statements
?       github.com/apache/yunikorn-core/pkg/scheduler/placement/types   [no test files]
        github.com/apache/yunikorn-core/pkg/webservice/dao              coverage: 0.0% of statements
ok      github.com/apache/yunikorn-core/pkg/log 25.208s coverage: 76.3% of statements
ok      github.com/apache/yunikorn-core/pkg/metrics     8.137s  coverage: 69.5% of statements
ok      github.com/apache/yunikorn-core/pkg/metrics/history     4.552s  coverage: 100.0% of statements
ok      github.com/apache/yunikorn-core/pkg/plugins     5.033s  coverage: 100.0% of statements
ok      github.com/apache/yunikorn-core/pkg/scheduler   8.046s  coverage: 68.7% of statements
ok      github.com/apache/yunikorn-core/pkg/scheduler/objects   4.299s  coverage: 84.9% of statements
ok      github.com/apache/yunikorn-core/pkg/scheduler/objects/events    6.734s  coverage: 100.0% of statements
ok      github.com/apache/yunikorn-core/pkg/scheduler/objects/template  5.430s  coverage: 100.0% of statements
ok      github.com/apache/yunikorn-core/pkg/scheduler/placement 4.549s  coverage: 100.0% of statements
ok      github.com/apache/yunikorn-core/pkg/scheduler/policies  7.382s  coverage: 100.0% of statements
ok      github.com/apache/yunikorn-core/pkg/scheduler/tests     48.139s coverage: [no statements]
ok      github.com/apache/yunikorn-core/pkg/scheduler/ugm       3.840s  coverage: 94.5% of statements
ok      github.com/apache/yunikorn-core/pkg/webservice  5.892s  coverage: 87.0% of statements
"go" vet github.com/apache/yunikorn-core/pkg...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes please do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I fix the syntax by changing to this(aebbea2)
the results in the make test is this:

running unit tests
"go" clean -testcache
"go" test ./... -race -tags deadlock -coverprofile=build/coverage.txt -covermode=atomic
        github.com/apache/yunikorn-core/cmd/queueconfigchecker          coverage: 0.0% of statements
        github.com/apache/yunikorn-core/cmd/schedulerclient             coverage: 0.0% of statements
        github.com/apache/yunikorn-core/cmd/simplescheduler             coverage: 0.0% of statements
        github.com/apache/yunikorn-core/pkg/events/mock         coverage: 0.0% of statements
        github.com/apache/yunikorn-core/pkg/examples            coverage: 0.0% of statements
?       github.com/apache/yunikorn-core/pkg/handler     [no test files]
ok      github.com/apache/yunikorn-core/pkg/common      3.070s  coverage: 74.1% of statements
ok      github.com/apache/yunikorn-core/pkg/common/configs      3.123s  coverage: 97.8% of statements
ok      github.com/apache/yunikorn-core/pkg/common/resources    4.936s  coverage: 100.0% of statements
        github.com/apache/yunikorn-core/pkg/mock                coverage: 0.0% of statements
        github.com/apache/yunikorn-core/pkg/rmproxy             coverage: 0.0% of statements
?       github.com/apache/yunikorn-core/pkg/rmproxy/rmevent     [no test files]
ok      github.com/apache/yunikorn-core/pkg/common/security     5.959s  coverage: 96.8% of statements
ok      github.com/apache/yunikorn-core/pkg/entrypoint  7.105s  coverage: 82.1% of statements
ok      github.com/apache/yunikorn-core/pkg/events      5.834s  coverage: 97.4% of statements
ok      github.com/apache/yunikorn-core/pkg/locking     8.267s  coverage: 51.1% of statements
?       github.com/apache/yunikorn-core/pkg/scheduler/placement/types   [no test files]
        github.com/apache/yunikorn-core/pkg/webservice/dao              coverage: 0.0% of statements
ok      github.com/apache/yunikorn-core/pkg/log 25.608s coverage: 76.3% of statements
ok      github.com/apache/yunikorn-core/pkg/metrics     9.365s  coverage: 69.5% of statements
ok      github.com/apache/yunikorn-core/pkg/metrics/history     6.755s  coverage: 100.0% of statements
ok      github.com/apache/yunikorn-core/pkg/plugins     5.666s  coverage: 100.0% of statements
ok      github.com/apache/yunikorn-core/pkg/scheduler   10.554s coverage: 68.7% of statements
ok      github.com/apache/yunikorn-core/pkg/scheduler/objects   7.591s  coverage: 84.9% of statements
ok      github.com/apache/yunikorn-core/pkg/scheduler/objects/events    6.293s  coverage: 100.0% of statements
ok      github.com/apache/yunikorn-core/pkg/scheduler/objects/template  6.005s  coverage: 100.0% of statements
ok      github.com/apache/yunikorn-core/pkg/scheduler/placement 5.608s  coverage: 100.0% of statements
ok      github.com/apache/yunikorn-core/pkg/scheduler/policies  5.793s  coverage: 100.0% of statements
ok      github.com/apache/yunikorn-core/pkg/scheduler/tests     47.177s coverage: [no statements]
ok      github.com/apache/yunikorn-core/pkg/scheduler/ugm       4.489s  coverage: 94.5% of statements
ok      github.com/apache/yunikorn-core/pkg/webservice  5.945s  coverage: 87.0% of statements
"go" vet github.com/apache/yunikorn-core/pkg...

Copy link
Contributor

@chenyulin0719 chenyulin0719 left a comment

Choose a reason for hiding this comment

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

-1 for this change. The condition is current used for the REST API:

This change will break it.
I'm surprised that this change doesn't cause the CI to fail. We should improve the test coverage.

image

@ryankert01
Copy link
Contributor

ryankert01 commented Sep 7, 2024

I see, there's two routes using the same handler

@400Ping 400Ping closed this Sep 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants