I thought we were going to rename ensureOpen's confusing boolean param?
Right, but for some reason I thought that you're going to do that . I'll do it in the next patch.
IW.setCommitData should be sync'd I think, eg to ensure visibility across threads of the changes to sis.userData?
Hmm ... I think there's a thread hazard here, during commit
I think you're right. Not sure how practical, because I believe that usually the commit thread will also be the one that calls setCommitData, but it is possible.
I agree that calling that setCommitData in finishCommit is redundant, but perhaps we can solve it more elegantly by either:
- Not storing the setCommitData in infos, but rather in a private IW member. Then in startCommit set it on the cloned infos. It's essentially how it's done today, only now the commit data will be copied from a member.
- Stick w/ current API commit(commitData) and prepareCommit(commitData), and just make sure that commit goes through even if changeCount == previousChangeCount, but commitUserData != null.
Option #2 means that there's no API break, no synchronization is needed on setCommitData and practically everything remains the same. We can still remove the redundant .setCommitData in finishCommit regadless.
should we add an IW.getCommitData?
I think that that'd be great ! Today the only way to do it is if you refresh a reader (expensive). I think that the code in finishCommit ensures that we can always pull the commitData from segmentInfos?