shouldRollGeneration should execute under read lock

[dnhatn]

Translog#shouldRollGeneration should execute under the read lock for it accesses the current writer.

[elasticmachine]

Pinging @elastic/es-distributed

[henningandersen]

LGTM.

I you know ways in which this could manifest itself, it would be nice to include into the commit message. I think this should in reality never show, but is easier to reason about with the lock check included.

[jasontedor]

I am not sure, is this actually a problem? There are issues here which this doesn't protect against. Namely the check and roll are not atomic, so it could be that a thread checks if a roll is needed, and then a roll occurs, and then this thread returns back and performs a roll itself. This change doesn't protect against that.

[dnhatn]

@henningandersen @jasontedor Thanks for looking.

I spotted this while I was investigating a case where more than 140 empty translog files in a single shard. For sure this is not the reason for those empty translog files. That shard has more than 280 primary failovers, and we roll a new translog on each primary promotion.

Although this should never happen in practice, I think this method should be called with lock. I marked this PR as a non-issue.

[jasontedor]

I agree it's more correct and should be integrated, my point though is that I don't think it matters in practice yet it still leaves behind real issues. 🤷‍♀

[dnhatn]

@henningandersen @jasontedor Thanks for reviewing!

[dnhatn]

process was found dead while waiting for ports files, node{:modules:mapper-extras:integTest-1}

run elasticsearch-ci/1