Apply optimistic locking for MongoDB repository

[quaff]

No description provided.

[fmbenhassine]

@quaff Any reason you closed this PR?

[quaff]

@quaff Any reason you closed this PR?

I found MongoDB DAO implementations doesn't persist version field, so this PR won't works as expected.

[fmbenhassine]

I found MongoDB DAO implementations doesn't persist version field

I did not encounter any major issues with the mongo repository since its introduction, and since it's not using this version field, I am questioning this optimistic locking strategy altogether and if we really need it.. Do you agree?

As a side note, I just wanted to thank you for all your time and effort with contributions to Spring Batch! REALLY appreciated and I won't hesitate to drop a word about that in the GA of v6 announcement.

[quaff]

I did not encounter any major issues with the mongo repository since its introduction, and since it's not using this version field, I am questioning this optimistic locking strategy altogether and if we really need it.. Do you agree?

Since the optimistic locking strategy is required by JDBC implementation for concurrency safety, MongoDB implementation should align to JDBC implementation, why not?

As a side note, I just wanted to thank you for all your time and effort with contributions to Spring Batch! REALLY appreciated and I won't hesitate to drop a word about that in the GA of v6 announcement.

You are welcome, it's my pleasure.

[fmbenhassine]

Since the optimistic locking strategy is required by JDBC implementation for concurrency safety, MongoDB implementation should align to JDBC implementation, why not?

"required": That's what I am questioning here. My thinking is that since the mongo job repository one does not use it and there are (apparently) no issues, it might be that optimistic locking is not required on the JDBC side as well. I don't exclude concurrency issues with the mongo implementation, we might just need more concurrency related tests to validate that. If that is the case, the mongo DAOs must be updated to handle the version field and this PR will be relevant.

In all cases, JDBC and MongoDB implementations should be consistent.

[quaff]

"required": That's what I am questioning here. My thinking is that since the mongo job repository one does not use it and there are (apparently) no issues, it might be that optimistic locking is not required on the JDBC side as well.

I don't think so, optimistic locking failure occurs when multi-threaded step is not handled correctly, it's necessary to let developer know something is wrong.

[fmbenhassine]

That's true for the previous generation, where a Tasklet could be executed concurrently by multiple threads (hence the need for optimistic locking, especially for ChunkOrientedTasklet). In v6, the chunk-oriented processing model is not based on the Tasklet / TaskletStep APIs, and the new concurrency model uses a single thread to execute the step, and only multiple threads for processing items (the interaction with the job repository is single threaded). That is what I meant when saying the optimistic locking strategy might not be needed anymore.

[quaff]

That's true for the previous generation, where a Tasklet could be executed concurrently by multiple threads (hence the need for optimistic locking, especially for ChunkOrientedTasklet). In v6, the chunk-oriented processing model is not based on the Tasklet / TaskletStep APIs, and the new concurrency model uses a single thread to execute the step, and only multiple threads for processing items (the interaction with the job repository is single threaded). That is what I meant when saying the optimistic locking strategy might not be needed anymore.

Glad to hear that, I'm not sure about this.

[quaff]

That's true for the previous generation, where a Tasklet could be executed concurrently by multiple threads (hence the need for optimistic locking, especially for ChunkOrientedTasklet). In v6, the chunk-oriented processing model is not based on the Tasklet / TaskletStep APIs, and the new concurrency model uses a single thread to execute the step, and only multiple threads for processing items (the interaction with the job repository is single threaded). That is what I meant when saying the optimistic locking strategy might not be needed anymore.

@fmbenhassine I noticed you introduce ReentrantLock for concurrent update, I don't think it's safe enough, in practice there may be multi-process executing partition step at the same time.

[fmbenhassine]

@quaff Thanks! Please open an issue and we can introduce a more robust locking mechanism if needed.