Add garbage collection support to the Prometheus metrics adjuster #613
Conversation
dinooliva
requested review from
pjanotti,
songy23,
tigrannajaryan and
a team
as code owners
Codecov Report
@@ Coverage Diff @@
## master #613 +/- ##
=========================================
Coverage ? 69.16%
=========================================
Files ? 92
Lines ? 6110
Branches ? 0
=========================================
Hits ? 4226
Misses ? 1664
Partials ? 220
Continue to review full report at Codecov.
|
|
@rghetia - ptal |
| } | ||
| jm.lastGC = time.Now() | ||
| } | ||
| jm.Unlock() |
There was a problem hiding this comment.
jm.Unlock can be defer jm.Unlock right after jm.Lock
I would like to put tsm.Lock() inside tsm.gc() but I am not sure that would work. The question is will anothe goroutine will have tsm while jm is Locked above?
There was a problem hiding this comment.
Yes, I can move the tsm.lock() to tsm.gc().
It is is possible that the metricsadjuster could mark the tsm and it will be removed from the JobsMap when it didn't need to be but that would happen even with locking (in a slightly different order).
| jm.Lock() | ||
| defer jm.Unlock() | ||
| // recheck lastGC to make sure gc is necessary | ||
| if current.Sub(jm.lastGC) > jm.gcInterval { |
There was a problem hiding this comment.
There was a problem hiding this comment.
Yes, the previous check is done before locking jm - this check is to verify that no other gc() has happened in the interim before we grab the lock. I'll add a comment to clarify what's going on.
No description provided.