Show healthy / desired allocs in job summary #17 #19
Conversation
There was a problem hiding this comment.
Hey @jawahars16, I love the solution with the Emojis 👏 I hope in 2021 there is no issues anymore displaying them on every Terminal :D
The PR looks great! I left a few comments. I think now with the fact that the job view requires to get updates for two topics (Job + Alloc) a subscriber should be able to subscribe to multiple topics at a time.
| row := []string{ | ||
| job.ID, | ||
| job.Name, | ||
| job.Type, | ||
| job.Namespace, | ||
| job.Status, | ||
| fmt.Sprintf("%d/%d", job.StatusSummary.Running, job.StatusSummary.Total), | ||
| reaadyStatus, |
There was a problem hiding this comment.
What do you think about adding a short variable name here, like rdyStatus or just status. I first thought it was a typo, before I realized this is to avoid the naming crash of the helper function.
| @@ -35,6 +35,7 @@ func TestJobTable_Happy(t *testing.T) { | |||
| Status: "running", | |||
| StatusDescription: "fine", | |||
| StatusSummary: models.Summary{Total: 2, Running: 2}, | |||
There was a problem hiding this comment.
The StatusSummary field isn't used anymore, do you mind removing it?
| @@ -47,6 +47,7 @@ func (v *View) Jobs() { | |||
| } | |||
|
|
|||
| v.Watcher.Subscribe(api.TopicJob, update) | |||
| v.Watcher.Subscribe(api.TopicAllocation, update) | |||
There was a problem hiding this comment.
In Damon, there is always a single subscriber at a time. Here we overwrite the subscription to topic Job with Allocation. This means that the first subscription can be removed.
I understand the reason for this is that the jobs view now requires information when allocations change. I think the easiest way to fix this would be to allow a subscriber to subscribe to multiple topics at a time. So the the Subscribe method should take an slice of Topics rather then a single topic:
v.Watcher.Subscribe([]api.Topic{api.Topic.Job, api.TopicAllocation}, update)Does that make sense?
There was a problem hiding this comment.
@hcjulz Sorry a little busy on other works. Just looking at the comments now. 😄
This make sense for views. To update jobs view, we need to subscribe to the changes for both Jobs and Allocations.
| w.updateAllocations() | ||
| { | ||
| w.updateAllocations() | ||
| w.updateJobs() // * Need this update, since we show allocation count in job view. |
There was a problem hiding this comment.
Related to my previous comment, I think if a subscriber can subscribe to multiple topics we can undo this?
There was a problem hiding this comment.
I am not sure about this one. This is happening in watcher. Updating the state of jobs and allocations happening directly in watcher. It is based on the messages coming from the event channel. If the event channel gives AllocationTopic, we should also update the Job's state. Sending multiple topics to subscribe method cannot solve this.
Your thoughts !!
There was a problem hiding this comment.
Hey @jawahars16, I'm not sure either right now. My thought was: If we change that a view can subscribe to multiple topics the watcher code will have to change to always notify all subscribed topics. I'll have a closer look into this this weekend.
There was a problem hiding this comment.
@jawahars16 I was able to confirm my theory. When a view can subscribe to multiple topics and it gets notified whenever one of these topics generates an event the view gets updated. I also tried it on top of your PR.
I did the code change to allow multiple subscriptions: #24
If you pull in the changes to your PR here, you only have to subscribe in view/jobs.go in the Jobs() func additionally to the Allocations topic. The Subscribe method slightly changed, the update func comes first:
watcher.Subscribe(notify func(), topics ...api.Topic)Give it a try. I hope it helps.
There was a problem hiding this comment.
Sure @hcjulz I will give it a try. Thanks.
|
Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement Learn more about why HashiCorp requires a CLA and what the CLA includes Jawahar seems not to be a GitHub user. Have you signed the CLA already but the status is still pending? Recheck it. |
What is this about?
Show healthy and desired allocs count in job summary along with a status indicator. (Follow up on #17)
Example : 2/3 ( 2 allocs are healthy out of 3 desired.)
The status indicator represents the below,
Preview