Return empty CommitID from ShadowEngine#flush #11554
Conversation
|
here is the CI link http://build-us-00.elastic.co/job/es_core_16_strong/37/consoleFull |
| } | ||
| // we have to read this in a loop since there is a potential race-condition between reading | ||
| // the segment info and reading the commit ID/ | ||
| } |
There was a problem hiding this comment.
while(true) doesn't seem right. Should this be done in a FindSegmentsFile block to remove the race?
There was a problem hiding this comment.
I think doing it in a FindSegmentsFile block would be better, but there is still going to be a race condition on actually reading the segments file, so it will still need to be in a loop?
|
@s1monw that sounds fine to me, if we don't need the CommitID why even read it at all then? Why not always return an empty one? |
|
+1 on an empty commit ID all the time. The only use case for it now is the synced flush which isn't supported anyway on ShadowEngines. |
s1monw
changed the title
| @@ -155,7 +158,9 @@ public CommitId flush(boolean force, boolean waitIfOngoing) throws EngineExcepti | |||
| } finally { | |||
| store.decRef(); | |||
| } | |||
| return id; | |||
| // We can just return an empty commit ID since this is a read only engine that | |||
| // doesn't modify anything so the content of this ID doesn't really matter | |||
There was a problem hiding this comment.
can we add a little note about avoiding race conditions between read and writes on a shared FS ? I think this will people we not see why we opted to EMPTY_COMMIT_ID
|
LGTM. Left one trivial comment |
s1monw
force-pushed
the
make_commit_id_mandatory
branch
2 times, most recently
from
9aba2e2
to
72acf7b
Compare
This change removes CommitID reading for 1.x indices to prevent raceconditions on shared FS for shadow engines where null is returned if a race happens. We now simply return an empty commit ID which is not needed on shadow engines anyway.
this change removes CommitID reading for 1.x indices to prevent raceconditions on shared FS for shadow engines where null is returned if a race happens. We now simply return an empty commit ID which is not needed on shadow engines anyway.