engine: Add tests for initialization failure #2238
Conversation
Codecov Report
@@ Coverage Diff @@
## support/v0.35 #2238 +/- ##
=================================================
+ Coverage 30.88% 30.99% +0.10%
=================================================
Files 383 383
Lines 28395 28419 +24
=================================================
+ Hits 8770 8808 +38
+ Misses 18878 18870 -8
+ Partials 747 741 -6
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| c.log.Info("shard attached to engine", zap.Stringer("id", id)) | ||
| } | ||
| } | ||
| if shardsAttached == 0 { |
There was a problem hiding this comment.
is it a discussed behavior? i mean could a node starting with just an error in the logs but without some planned shards be expected by an admin?
There was a problem hiding this comment.
Currently we are still dropping shards in Init , so this behaviour is not new.
In our model shard is an (almost) independent domain of failure. I am mostly thinking about automatic node restart after hardware failures. We can discuss this in future, I am not sure what the expected behaviour here is: for 11 shards repetitve config manipulations is a laborious and error-prone task.
There was a problem hiding this comment.
Or do you mean that we could start it in a degraded mode if we cannot write ID to the metabase?
There was a problem hiding this comment.
The biggest problem of this approach is that you can lose shards (and potentially data) on benign misconfiguration. This will happen.
There was a problem hiding this comment.
so this behaviour is not new
but we had fatalOnErr(err) previously if a shard has not been attached. as i understand, that was very different. losing shards without some huge "you are losing shards" banner bothers me but i dont have any good ideas of how to solve that right now
There was a problem hiding this comment.
In our model shard is an (almost) independent domain of failure.
also, can't fully agree with this. one shard could contain Locks, Tombstones that protect/remove data from another so losing one shard is not just losing some objects. but that is our general problem
| e.mtx.RLock() | ||
| defer e.mtx.RUnlock() | ||
| e.mtx.Lock() | ||
| defer e.mtx.Unlock() |
There was a problem hiding this comment.
Why do you duplicate this much code from Init()?
There was a problem hiding this comment.
Could you elaborate?
This line is here because we can remove shards in Open after this PR.
There was a problem hiding this comment.
It's not about this particular line, I just had to attach this comment somewhere. There is quite some duplication between open and Init now it seems.
There was a problem hiding this comment.
Oh, I see.
I've thought about it here, though, I am not sure it will be simpler with some ad-hoc parrallelizing function which accepts functions as arguments.
| c.log.Info("shard attached to engine", zap.Stringer("id", id)) | ||
| } | ||
| } | ||
| if shardsAttached == 0 { |
There was a problem hiding this comment.
The biggest problem of this approach is that you can lose shards (and potentially data) on benign misconfiguration. This will happen.
I agree. But the alternative here is lose all vs lose some data. |
1. Both could initialize shards in parallel. 2. Both should close shards after an error. Signed-off-by: Evgenii Stratonikov <e.stratonikov@yadro.com>
Signed-off-by: Evgenii Stratonikov <e.stratonikov@yadro.com>
…rrors Signed-off-by: Evgenii Stratonikov <e.stratonikov@yadro.com>
Signed-off-by: Evgenii Stratonikov <e.stratonikov@yadro.com>
Fix bugs along the way.
Invalid mode on a directory is an approximation of missing media.
We expect any
Open/Initerrors to be logged and a shard to be disabled.