add timestamps to evals #5881
add timestamps to evals #5881
Conversation
|
So the tests on my branch pass (travis-ci/push), but not the ones merged w/ master (travis-ci/pr). I thought it was a transient issue on master, so I'm rerunning and looking into it if its not. UPDATE: yes, it was transient. build is passing now. |
|
Working on the description, which will include a walkthrough of the method call tracing and where is the correct place to set eval timestamps |
| @@ -235,6 +235,7 @@ func (a *Alloc) Stop(args *structs.AllocStopRequest, reply *structs.AllocStopRes | |||
| return fmt.Errorf(structs.ErrUnknownAllocationPrefix) | |||
| } | |||
|
|
|||
| now := time.Now().UTC().UnixNano() | |||
| eval := &structs.Evaluation{ | |||
There was a problem hiding this comment.
Someday a helper func for making evals might be nice to ensure we always set the time properly:
func NewEval(namespace, type string) *Evaluation {
// get time.now, generate uuid, and set basic fields here
}Not a blocker for this PR.
There was a problem hiding this comment.
Agreed. This was discussed but I opted not to since a big refactor on all eval creations seemed risky and outside of the scope of this PR.
There was a problem hiding this comment.
Too bad Go doesn't have constructors! 🐣
| // Format the evaluation data | ||
| basic := []string{ | ||
| fmt.Sprintf("ID|%s", limit(eval.ID, length)), | ||
| fmt.Sprintf("Create Time|%s", formattedCreateTime), | ||
| fmt.Sprintf("Modify Time|%s", formattedModifyTime), |
There was a problem hiding this comment.
Pointing out that for old evals this will print an empty string, which may be ok but odd UX.
There was a problem hiding this comment.
Agreed it's not the greatest UX. This behavior is consistent with nomad alloc status, although it's unlikely at this point that there are old allocs w/o a CreateTime. Going to think about it, will most likely leave as is but add a note in the docs.
There was a problem hiding this comment.
Leaving it as is sounds good to me if there's prior art already that does this.
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Overview
Added timestamps to evaluations upon creation/modification.
Behavior
Implementation
The key here was to add timestamps to evals in the correct place, which would be where the eval is first registered as part of a raft transaction.
TODO