You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by "pbacsko (via GitHub)" <gi...@apache.org> on 2023/06/19 20:22:40 UTC
[GitHub] [yunikorn-core] pbacsko opened a new pull request, #573: [YUNIKORN-1802] Add wrapper for node events
pbacsko opened a new pull request, #573:
URL: https://github.com/apache/yunikorn-core/pull/573
### What is this PR for?
Create wrapper type for node events and integrate it with the existing codebase.
### What type of PR is it?
* [ ] - Bug Fix
* [ ] - Improvement
* [x] - Feature
* [ ] - Documentation
* [ ] - Hot Fix
* [ ] - Refactoring
### Todos
* [ ] - Task
### What is the Jira issue?
https://issues.apache.org/jira/browse/YUNIKORN-1802
### How should this be tested?
### Screenshots (if appropriate)
### Questions:
* [ ] - The licenses files need update.
* [ ] - There is breaking changes for older versions.
* [ ] - It needs documentation.
--
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: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] wilfred-s commented on pull request #573: [YUNIKORN-1802] Add node events
Posted by "wilfred-s (via GitHub)" <gi...@apache.org>.
wilfred-s commented on PR #573:
URL: https://github.com/apache/yunikorn-core/pull/573#issuecomment-1602715508
PR needs a rebase after other event changes have been committed.
--
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: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] pbacsko commented on a diff in pull request #573: [YUNIKORN-1802] Add node events
Posted by "pbacsko (via GitHub)" <gi...@apache.org>.
pbacsko commented on code in PR #573:
URL: https://github.com/apache/yunikorn-core/pull/573#discussion_r1246342542
##########
pkg/scheduler/objects/node.go:
##########
@@ -584,4 +592,13 @@ func (sn *Node) SetReady(ready bool) {
sn.Lock()
defer sn.Unlock()
sn.ready = ready
+ sn.nodeEvents.sendNodeReadyChangedEvent(sn.ready)
+}
+
+func (sn *Node) SendNodeAddedEvent() {
+ sn.nodeEvents.sendNodeAddedEvent()
Review Comment:
There's a direct reference to `n.node.NodeID` (which doesn't change) and `n.node.GetCapacity()` which uses locking, so it's fine.
--
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: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] wilfred-s commented on a diff in pull request #573: [YUNIKORN-1802] Add node events
Posted by "wilfred-s (via GitHub)" <gi...@apache.org>.
wilfred-s commented on code in PR #573:
URL: https://github.com/apache/yunikorn-core/pull/573#discussion_r1246228817
##########
pkg/scheduler/objects/node.go:
##########
@@ -584,4 +592,13 @@ func (sn *Node) SetReady(ready bool) {
sn.Lock()
defer sn.Unlock()
sn.ready = ready
+ sn.nodeEvents.sendNodeReadyChangedEvent(sn.ready)
+}
+
+func (sn *Node) SendNodeAddedEvent() {
+ sn.nodeEvents.sendNodeAddedEvent()
Review Comment:
Should this not happen under a read lock? (same for remove)
--
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: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] pbacsko commented on a diff in pull request #573: [YUNIKORN-1802] Add node events
Posted by "pbacsko (via GitHub)" <gi...@apache.org>.
pbacsko commented on code in PR #573:
URL: https://github.com/apache/yunikorn-core/pull/573#discussion_r1239571433
##########
pkg/scheduler/partition_test.go:
##########
@@ -355,15 +355,15 @@ func TestRemoveNodeWithAllocations(t *testing.T) {
// wait for events to be processed
err = common.WaitFor(10*time.Millisecond, time.Second, func() bool {
fmt.Printf("checking event length: %d\n", eventSystem.Store.CountStoredEvents())
- return eventSystem.Store.CountStoredEvents() == 1
+ return eventSystem.Store.CountStoredEvents() == 4 // 3 node alloc + 1 app event
Review Comment:
Created a follow-up to address this: https://issues.apache.org/jira/browse/YUNIKORN-1833.
The tests are becoming unmaintainable, this cannot be tested here.
--
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: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] pbacsko commented on a diff in pull request #573: [YUNIKORN-1802] Add node events
Posted by "pbacsko (via GitHub)" <gi...@apache.org>.
pbacsko commented on code in PR #573:
URL: https://github.com/apache/yunikorn-core/pull/573#discussion_r1239586753
##########
pkg/scheduler/objects/node.go:
##########
@@ -27,6 +27,7 @@ import (
"go.uber.org/zap"
Review Comment:
Created the follow-up.
--
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: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] pbacsko commented on a diff in pull request #573: [YUNIKORN-1802] Add node events
Posted by "pbacsko (via GitHub)" <gi...@apache.org>.
pbacsko commented on code in PR #573:
URL: https://github.com/apache/yunikorn-core/pull/573#discussion_r1239571433
##########
pkg/scheduler/partition_test.go:
##########
@@ -355,15 +355,15 @@ func TestRemoveNodeWithAllocations(t *testing.T) {
// wait for events to be processed
err = common.WaitFor(10*time.Millisecond, time.Second, func() bool {
fmt.Printf("checking event length: %d\n", eventSystem.Store.CountStoredEvents())
- return eventSystem.Store.CountStoredEvents() == 1
+ return eventSystem.Store.CountStoredEvents() == 4 // 3 node alloc + 1 app event
Review Comment:
Created a follow-up to address this: https://issues.apache.org/jira/browse/YUNIKORN-1833.
The tests are becoming unmaintainable, this cannot be verified here.
--
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: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] pbacsko closed pull request #573: [YUNIKORN-1802] Add node events
Posted by "pbacsko (via GitHub)" <gi...@apache.org>.
pbacsko closed pull request #573: [YUNIKORN-1802] Add node events
URL: https://github.com/apache/yunikorn-core/pull/573
--
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: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] pbacsko commented on a diff in pull request #573: [YUNIKORN-1802] Add wrapper for node events
Posted by "pbacsko (via GitHub)" <gi...@apache.org>.
pbacsko commented on code in PR #573:
URL: https://github.com/apache/yunikorn-core/pull/573#discussion_r1234464610
##########
pkg/scheduler/objects/node.go:
##########
@@ -27,6 +27,7 @@ import (
"go.uber.org/zap"
Review Comment:
General comment: we don't have event change details for placeholder replacement and reservation/unreservation. I don't know if we need those or not, I assume the replace would be useful.
--
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: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] codecov[bot] commented on pull request #573: [YUNIKORN-1802] Add wrapper for node events
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #573:
URL: https://github.com/apache/yunikorn-core/pull/573#issuecomment-1597728589
## [Codecov](https://app.codecov.io/gh/apache/yunikorn-core/pull/573?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#573](https://app.codecov.io/gh/apache/yunikorn-core/pull/573?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (6062dc6) into [master](https://app.codecov.io/gh/apache/yunikorn-core/commit/a6a90bb0408a3004592f06d8a79335ac326dcfe4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (a6a90bb) will **increase** coverage by `0.18%`.
> The diff coverage is `96.66%`.
> :exclamation: Current head 6062dc6 differs from pull request most recent head b4bf21e. Consider uploading reports for the commit b4bf21e to get more accurate results
```diff
@@ Coverage Diff @@
## master #573 +/- ##
==========================================
+ Coverage 75.45% 75.64% +0.18%
==========================================
Files 73 74 +1
Lines 12057 12145 +88
==========================================
+ Hits 9098 9187 +89
+ Misses 2638 2635 -3
- Partials 321 323 +2
```
| [Impacted Files](https://app.codecov.io/gh/apache/yunikorn-core/pull/573?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [pkg/events/events.go](https://app.codecov.io/gh/apache/yunikorn-core/pull/573?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL2V2ZW50cy9ldmVudHMuZ28=) | `100.00% <ø> (ø)` | |
| [pkg/scheduler/objects/node\_events.go](https://app.codecov.io/gh/apache/yunikorn-core/pull/573?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL25vZGVfZXZlbnRzLmdv) | `95.94% <95.94%> (ø)` | |
| [pkg/scheduler/context.go](https://app.codecov.io/gh/apache/yunikorn-core/pull/573?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL3NjaGVkdWxlci9jb250ZXh0Lmdv) | `30.63% <100.00%> (+0.20%)` | :arrow_up: |
| [pkg/scheduler/objects/node.go](https://app.codecov.io/gh/apache/yunikorn-core/pull/573?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL25vZGUuZ28=) | `84.16% <100.00%> (+1.60%)` | :arrow_up: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] pbacsko commented on pull request #573: [YUNIKORN-1802] Add wrapper for node events
Posted by "pbacsko (via GitHub)" <gi...@apache.org>.
pbacsko commented on PR #573:
URL: https://github.com/apache/yunikorn-core/pull/573#issuecomment-1597734367
cc @FrankYang0529 you might need some stuff from this PR, like `MockEventSystem`. If this gets merged, you can rebase your PR on this.
--
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: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] wilfred-s commented on a diff in pull request #573: [YUNIKORN-1802] Add node events
Posted by "wilfred-s (via GitHub)" <gi...@apache.org>.
wilfred-s commented on code in PR #573:
URL: https://github.com/apache/yunikorn-core/pull/573#discussion_r1238552626
##########
pkg/scheduler/objects/node.go:
##########
@@ -27,6 +27,7 @@ import (
"go.uber.org/zap"
Review Comment:
Placeholders: no that is just another allocation for the node. Nothing special from that side.
Reservation: file a follow up jira to look at that. We use a reservation in different ways specially in preemption. We need to think where we report it (app or node) and how we report it.
##########
pkg/events/events.go:
##########
@@ -25,6 +25,8 @@ import (
"github.com/apache/yunikorn-scheduler-interface/lib/go/si"
)
+const Empty = ""
+
Review Comment:
Move to the common package
--
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: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org