Call 'GetAsksById' once per DoWork

[Enkidu93]

Potential fix for #492. @johnml1135, I'll leave potentially updating the polling interval in the deployments to you if you deem it necessary.

Also, I think that there was a bug here where tasks would have been counted in both queues. There were no reports of this (to my knowledge) and there isn't testing that covers the logic in this class. In any case, I fixed that as well.

---

This change is 

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 4.08163% with 47 lines in your changes missing coverage. Please review.

Project coverage is 56.97%. Comparing base (52dc1ba) to head (c27c8fb).

Files with missing lines	Patch %	Lines
...c/Serval.Machine.Shared/Services/ClearMLService.cs	6.25%	30 Missing ⚠️
...l.Machine.Shared/Services/ClearMLMonitorService.cs	0.00%	17 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #504      +/-   ##
==========================================
- Coverage   57.06%   56.97%   -0.10%     
==========================================
  Files         277      277              
  Lines       14168    14194      +26     
  Branches     1902     1907       +5     
==========================================
+ Hits         8085     8087       +2     
- Misses       5495     5519      +24     
  Partials      588      588              

Flag	Coverage Δ
	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[johnml1135]

src/Machine/src/Serval.Machine.Shared/Services/ClearMLMonitorService.cs line 55 at r1 (raw file):

            Dictionary<string, ClearMLTask> tasks = (
                await _clearMLService.GetTasksByIdAsync(

Getting tasks by ID only takes a few milliseconds. It's the queue task that takes 3-5 seconds. I pushed an update to bring down the time to < 100ms per queue call.

[johnml1135]

src/Machine/src/Serval.Machine.Shared/Services/ClearMLMonitorService.cs line 65 at r1 (raw file):

            {
                var tasksPerEngineType = tasks
                    .Where(kvp =>

I'm a little confused what the extra "where" and "select" does...

[johnml1135]

Reviewed 1 of 1 files at r1.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @ddaspit and @Enkidu93)

[ddaspit]

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Enkidu93)

---

src/Machine/src/Serval.Machine.Shared/Services/ClearMLService.cs line 149 at r2 (raw file):

    )
    {
        await PopulateQueueNamesToIdsAsync(cancellationToken);

If we return the dictionary from this method call, then we know that it will never be null.

---

src/Machine/src/Serval.Machine.Shared/Services/ClearMLService.cs line 151 at r2 (raw file):

        await PopulateQueueNamesToIdsAsync(cancellationToken);

        var body = new JsonObject { ["queue"] = _queueNamesToIds![queue] };

We should check if the queue name exists in the dictionary. If not, we should try repopulating the dictionary.

---

src/Machine/src/Serval.Machine.Shared/Services/ClearMLService.cs line 160 at r2 (raw file):

    private async Task PopulateQueueNamesToIdsAsync(CancellationToken cancellationToken = default)
    {
        if (_queueNamesToIds != null)

We should synchronize access to _queueNamesToIds, since it is a mutable field in a singleton service.

---

src/Machine/src/Serval.Machine.Shared/Services/ClearMLService.cs line 182 at r2 (raw file):

    )
    {
        if (!ids.Any())

We should convert ids to an array before performing this check. Generally, you want to avoid iterating over an IEnumerable multiple times.

[Enkidu93]

I'll let you, @johnml1135, address the changes that @ddaspit requested on your commit - unless you'd like me to go ahead and address them.

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @johnml1135)

---

src/Machine/src/Serval.Machine.Shared/Services/ClearMLMonitorService.cs line 55 at r1 (raw file):

Previously, johnml1135 (John Lambert) wrote…

Getting tasks by ID only takes a few milliseconds. It's the queue task that takes 3-5 seconds. I pushed an update to bring down the time to < 100ms per queue call.

OK, cool, that's great!

---

src/Machine/src/Serval.Machine.Shared/Services/ClearMLMonitorService.cs line 65 at r1 (raw file):

Previously, johnml1135 (John Lambert) wrote…

I'm a little confused what the extra "where" and "select" does...

Unless I'm mistaken (which is surely possible 😆), this variable was grabbing all tasks, not just the ones of that loop's engine type which would throw the numbers off.

[Enkidu93]

Reviewable status: 1 of 2 files reviewed, 6 unresolved discussions (waiting on @ddaspit and @johnml1135)

---

src/Machine/src/Serval.Machine.Shared/Services/ClearMLService.cs line 149 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

If we return the dictionary from this method call, then we know that it will never be null.

Done.

---

src/Machine/src/Serval.Machine.Shared/Services/ClearMLService.cs line 151 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

We should check if the queue name exists in the dictionary. If not, we should try repopulating the dictionary.

Done. Do we want to throw an exception if it's still not in there after repopulating?

---

src/Machine/src/Serval.Machine.Shared/Services/ClearMLService.cs line 160 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

We should synchronize access to _queueNamesToIds, since it is a mutable field in a singleton service.

Done.

---

src/Machine/src/Serval.Machine.Shared/Services/ClearMLService.cs line 182 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

We should convert ids to an array before performing this check. Generally, you want to avoid iterating over an IEnumerable multiple times.

Done.

[ddaspit]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Enkidu93 and @johnml1135)

---

src/Machine/src/Serval.Machine.Shared/Services/ClearMLService.cs line 149 at r2 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

Done.

If _queueNamesToIds is no longer nullable, then we don't need to return it from PopulateQueueNamesToIdsAsync anymore.

---

src/Machine/src/Serval.Machine.Shared/Services/ClearMLService.cs line 151 at r2 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

Done. Do we want to throw an exception if it's still not in there after repopulating?

Yes, we should throw if it is still not there after repopulating.

---

src/Machine/src/Serval.Machine.Shared/Services/ClearMLService.cs line 160 at r2 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

Done.

This code could result in the case where _queueNamesToIds contains queues that no longer exist. If we use an AsyncLock coupled with an ImmutableDictionary, then the code should be thread-safe.

[Enkidu93]

Reviewable status: 1 of 3 files reviewed, 5 unresolved discussions (waiting on @ddaspit and @johnml1135)

---

src/Machine/src/Serval.Machine.Shared/Services/ClearMLService.cs line 149 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

If _queueNamesToIds is no longer nullable, then we don't need to return it from PopulateQueueNamesToIdsAsync anymore.

It's nullable again haha

---

src/Machine/src/Serval.Machine.Shared/Services/ClearMLService.cs line 151 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Yes, we should throw if it is still not there after repopulating.

Done.

---

src/Machine/src/Serval.Machine.Shared/Services/ClearMLService.cs line 160 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

This code could result in the case where _queueNamesToIds contains queues that no longer exist. If we use an AsyncLock coupled with an ImmutableDictionary, then the code should be thread-safe.

Done. Let me know if you think the scope of the locking is appropriate.

[ddaspit]

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @johnml1135)

[johnml1135]

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Enkidu93)

[johnml1135]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Enkidu93)