Refactor recording client management #20175
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted filessee 22 files with indirect coverage changes @@ Coverage Diff @@
## main #20175 +/- ##
==========================================
- Coverage 69.27% 69.20% -0.08%
==========================================
Files 413 413
Lines 34364 34367 +3
==========================================
- Hits 23805 23783 -22
- Misses 9160 9180 +20
- Partials 1399 1404 +5 Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
nwnt
force-pushed
the
nwnt/refactor-recording-client-mgmt
branch
from
06331aa to
a9821d2
Compare
nwnt
force-pushed
the
nwnt/refactor-recording-client-mgmt
branch
from
a9821d2 to
df159e9
Compare
nwnt
force-pushed
the
nwnt/refactor-recording-client-mgmt
branch
from
df159e9 to
f0731f2
Compare
nwnt
force-pushed
the
nwnt/refactor-recording-client-mgmt
branch
from
f0731f2 to
76c980e
Compare
|
@serathius sorry was away for a few days. How does the latest commit look? |
nwnt
force-pushed
the
nwnt/refactor-recording-client-mgmt
branch
from
76c980e to
e2dba97
Compare
tests/robustness/client/client.go
Outdated
| type ClientSet struct { | ||
| mux sync.Mutex | ||
| idProvider identity.Provider | ||
| baseTime time.Time | ||
|
|
||
| closed bool | ||
| clients []*RecordingClient | ||
| reports []report.ClientReport | ||
| } |
There was a problem hiding this comment.
| type ClientSet struct { | |
| mux sync.Mutex | |
| idProvider identity.Provider | |
| baseTime time.Time | |
| closed bool | |
| clients []*RecordingClient | |
| reports []report.ClientReport | |
| } | |
| type ClientSet struct { | |
| idProvider identity.Provider | |
| baseTime time.Time | |
| mux sync.Mutex | |
| closed bool | |
| clients []*RecordingClient | |
| reports []report.ClientReport | |
| } |
It good to have mutex over the variables it's meant to protect. We don't need to protect idProvider or baseTime as they are never changed.
| } | ||
|
|
||
| func (cs *ClientSet) NewClient(endpoints []string) (*RecordingClient, error) { | ||
| if cs.closed { |
There was a problem hiding this comment.
Checking cs.closed should be under mutex.
tests/robustness/client/client.go
Outdated
| } | ||
|
|
||
| func (cs *ClientSet) Reports() []report.ClientReport { | ||
| if !cs.closed { |
There was a problem hiding this comment.
Checking closed and and closing should be under mutex.
There was a problem hiding this comment.
Instead of this check, I made the method always call Close. The same check is done there. If we leave the function here and cover it with mutex, it will run into a deadlock from acquiring lock at Reports and acquiring lock again at Close.
| return cs.reports | ||
| } | ||
|
|
||
| func (cs *ClientSet) Close() { |
| @@ -210,8 +195,8 @@ func runDefragLoop(ctx context.Context, c *client.RecordingClient, period time.D | |||
| } | |||
| } | |||
|
|
|||
| func connect(endpoints []string, ids identity.Provider, baseTime time.Time) *client.RecordingClient { | |||
There was a problem hiding this comment.
For followup, we should remove this function.
nwnt
force-pushed
the
nwnt/refactor-recording-client-mgmt
branch
3 times, most recently
from
4b73c1f to
0d81538
Compare
|
/retest |
nwnt
force-pushed
the
nwnt/refactor-recording-client-mgmt
branch
from
0d81538 to
1be5691
Compare
Signed-off-by: Nont <nont@duck.com>
nwnt
force-pushed
the
nwnt/refactor-recording-client-mgmt
branch
from
1be5691 to
ae8a57f
Compare
|
This should fix all of them. Can I bother you to review again @serathius? |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nwnt, serathius The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
Fix #19893