stantheman0128 opened a new pull request, #1125:
URL: https://github.com/apache/yunikorn-core/pull/1125
### Description
`tryPreemption` built a `Preemptor` and then called `CheckPreconditions` on
it. The checks read
only the ask and the preemption delay, so the preemptor was being allocated
even for asks that were
about to be rejected.
This turns `CheckPreconditions` into a package level function taking the ask
and the delay, and moves
`NewPreemptor` below the check. Asks that fail the checks no longer allocate
a preemptor to find that
out.
Generated by Claude Code (Claude Opus 5).
Two notes:
- `NewPreemptor` is a plain struct literal with no side effects, so moving
it after the check does not
change behaviour. All six checks read `ask` and `preemptionDelay` only,
which is what makes the
extraction possible in the first place.
- `CheckPreconditions` changes from a method on `*Preemptor` to a package
level function. It stays
exported so the identifier is still reachable, but it is only used inside
this package and its
tests, so I am happy to make it unexported if you would rather narrow the
surface.
On effect: the allocation avoided is one 112 byte `Preemptor` per call that
fails the checks. That
path is not rare, because `preemptAttemptFrequency` gates a given ask to one
real attempt every 15
seconds, so repeated scheduling cycles over a pending unschedulable ask
return at the frequency check.
I did not run a macro benchmark for this. `BenchmarkScheduling` drives asks
that get allocated, so it
does not walk the preemption path, and I would rather report nothing than
report a number that does
not measure this change.
### Type of change
- [x] Improvement
- [x] Refactoring
### Jira issue
Jira ID : https://issues.apache.org/jira/browse/YUNIKORN-3331
- [ ] I have created a Jira issue for this pull request.
- [x] The Jira ID is part of the title of this pull request.
### AI Tooling
- [x] The PR includes the phrase "Generated by Claude Code (Claude Opus 5)",
where Claude Code is the name of the AI tool used.
- [x] My use of AI contributions follows the ASF legal policy.
Check https://www.apache.org/legal/generative-tooling.html for details.
### How has this been tested?
- [ ] New unit tests were added to cover new or changed code paths.
- [x] `make test_all` was run, and no failures reported.
- [ ] A pull request will be opened for new e2e tests
(apache/yunikorn-k8shim repository).
No new tests. `TestCheckPreconditions` already covered every branch of these
checks, and it now calls
the function directly instead of through a preemptor, which is the point of
the change.
`make test_all` on Linux, all green:
running shellcheck
checking license headers:
all OK
running golangci-lint
0 issues.
running unit tests
ok github.com/apache/yunikorn-core/pkg/scheduler 23.283s
coverage: 75.6% of statements
ok github.com/apache/yunikorn-core/pkg/scheduler/objects 11.851s
coverage: 89.6% of statements
ok github.com/apache/yunikorn-core/pkg/scheduler/tests 52.107s
(every other package ok, output trimmed)
Also run locally, not part of CI:
go test ./pkg/scheduler/objects/ -race -run
'TestCheckPreconditions|Preempt' -count=50
ok github.com/apache/yunikorn-core/pkg/scheduler/objects 389.748s
`gofmt -l pkg/` is clean and `go vet` passes.
### Questions:
- The change does not need documentation.
- There are no breaking changes.
- The license files do not need to be updated.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
