Patch Envelopes: remove envelopesToUpdate
#3806
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3806 +/- ##
==========================================
- Coverage 19.71% 19.67% -0.04%
==========================================
Files 235 235
Lines 51757 51723 -34
==========================================
- Hits 10204 10178 -26
+ Misses 40815 40807 -8
Partials 738 738 ☔ View full report in Codecov by Sentry. |
|
LGTM - Thx for your time. |
|
Did we include PE support in 11.0? If so we should label this stable and backport to 11.0-stable. |
|
@eriknordmark , yep. I'll put stable on it and backport |
uncleDecart
force-pushed
the
iterate-over-all-envelopes
branch
from
abfcb73
to
a495836
Compare
|
@eriknordmark we include PE partially in 11.0-stable (only inline objects, no volume reference), so backporting this test doesn't make sense. Should I backport all of the patches to 11.0-stable? |
Previous patch envelope unit test flakiness revealed that using `envelopesToUpdate` boolean map creates a a situation, where in some cases patch envelope is not getting updates. This happens occasionally (around 2 out of 200 runs). In spirit of KIS (Keep It Simple) this patch removes `envelopesToUpdate` and iterates over every attached patch envelope when updating state. Since number of patch envelopes attached to Edge App is small in real life scenarios, it won't affect performance. Morevover, one could argue that without the need to determine which patch envelope is affected when `VolumeInstance` is changed of `contentTree` appears, such approach is more suitable. In addition, this patch introduces `RWMutex` in `PatchEnvelopes` struct, because in `updateEnvelopes` function assigns map to new value, which can cause a race condition. Note that queue update queue still won't grow, since update channel is still buffered to one. Signed-off-by: Pavel Abramov <uncle.decart@gmail.com>
uncleDecart
force-pushed
the
iterate-over-all-envelopes
branch
from
a495836
to
88d34b4
Compare
If this isn't needed in 11.0-stable then no need. Do we have an API Capability defined for the PE support? If so we should make sure it isn't set in 11.0-stable. |
We didn't define capability for the PE support. We should introduce it, I'll take a look |
Previous patch envelope unit test flakiness revealed that using
envelopesToUpdateboolean map creates a a situation, where in some cases patch envelope is not getting updates. This happens occasionally (around 2 out of 200 runs).In spirit of KIS (Keep It Simple) this patch removes
envelopesToUpdateand iterates over every attached patch envelope when updating state. Since number of patch envelopes attached to Edge App is small in real life scenarios, it won't affect performance. Morevover, one could argue that without the need to determine which patch envelope is affected whenVolumeInstanceis changed ofcontentTreeappears, such approach is more suitable.Note that queue update queue still won't grow, since update channel is still buffered to one.
Thanks @christoph-zededa for finding the problem!