YUNIKORN-42 (Work in progress changes for recorder) #118
Conversation
|
Just added a WIP change for recorder, which includes:
Pending:
@wilfred-s , @TaoYang526 , @yangwwei please add your suggestions. |
There was a problem hiding this comment.
I read the discussion in the jira. Though I agree with @wilfred-s regarding the event publishing, I think for existing K8s users the kubectl describe is a must - this is the most handful tool a kubernetes admin to look at. IMO we should seek a way to only emit K8s relevant scheduling events and if the user wants some more granular scheduler diagnostics, should try to look for the YuniKorn UI or REST API.
Regarding the actual recording message, those could be submitted through another PR.
| timer.SetNanoTimeNow(newTime) | ||
| if timer.NanoTimeNow()-after > time.Second.Nanoseconds() { | ||
| t.Errorf("SetNanoTime for regular timer should not take effect") | ||
| } |
There was a problem hiding this comment.
I think this test case is wrong. If the regularTimer's SetNanoTimeNow took effect, the test would pass.
| timer common.Timer | ||
|
|
||
| // Nano-second interval of record for same key | ||
| minimumRecordIntervalForSameKeyInNS int64 |
| return true | ||
| } | ||
|
|
||
| func (de *DiagEventsRecorder) RecordResourceRequestEvent(level Level, requestId string, appId string, queueName string, nodeId string, message string) { |
There was a problem hiding this comment.
A technical question: just by looking at the datastructure I assume we only allow one event per request. Is this intended? Isn't it a bit too strict? I can imagine scenarios where for a request we can emit multiple event records.
| idx := 0 | ||
| for _, v := range de.eventMap { | ||
| ret[idx] = v | ||
| idx ++ | ||
| } |
There was a problem hiding this comment.
| idx := 0 | |
| for _, v := range de.eventMap { | |
| ret[idx] = v | |
| idx ++ | |
| } | |
| for idx, v := range de.eventMap { | |
| ret[idx] = v | |
| } |
No description provided.