Support optimistic locking for full update scenario using seq_no + primary_term [DATAES-799]

[spring-projects-issues]

Roman Puchkovskiy opened DATAES-799 and commented

Elasticsearch currently recommends using seq_no + primary_term combination to implement optimistic locking: https://www.elastic.co/guide/en/elasticsearch/reference/current/optimistic-concurrency-control.html

Consider the following scenario:

1. Repository.findById() is called
2. The returned entity is edited by a user
3. Repository.save() is called to save the entity

To make optimistic locking work in this scenario, on step 1 the returned entity must return seq_no + primary_term, and on step 3 they must be passed in the index request. But currently, if I'm not mistaken, the returned seq_no + primary_term are not mapped to the entity instance when the entity is materialized, and no way exists to pass them back to save() methods.

I suggest the following:

1. Add @SequenceNumber and @PrimaryTerm annotations in org.springframework.data.elasticsearch.annotations; they will be used to mark Long entity fields
2. When materializing an entity from GetResult, SearchHit, or any other possible DTO coming from 'getting' Elasticsearch response, fill fields mapped with these new annotations with seq_no and primary_term values from the response
3. When creating an IndexRequest during indexing/saving, call setIfSeqNo() and setIfPrimaryTerm() on the constructed request with values from the annotated fields

An alternative to have 2 separate fields and 2 annotations is to invent a special value object combining them both, like SequencePosition, encapsulating both seq_no and primary_term, and add just one annotation, @SequenceNumber, to annotate a field of type SequencePosition.

 

Does this make sense?

---

Affects: 4.0 RC2 (Neumann)

Issue Links:

• DATAES-876 Add seqno and primary term to entity on initial save

Referenced from: pull request #441

[spring-projects-issues]

sothawo commented

I would go for a dedicated value object like SeqNoPrimaryTerm.

There is no need for an annotation. On setting up the ElasticsearchPersistentEntity we simply check if there is a property of this type and keep this info and use it on building the requests and reading the results. The property then would be omitted in the normal converted data - like an implicit @Transient annotation.

This shouldn't be too much work, looks like an interesting task

[spring-projects-issues]

Roman Puchkovskiy commented

I will try to make a prototype

[spring-projects-issues]

sothawo commented

I changed to state of this issue to in progress to make it clear, that this is worked upon

[jjbowman2]

This seems to cover the JPA use case, but there doesn't seem to be support for getting the seq_no/primary_term using the SearchOperations/ElasticRestTemplate. There does seem to be methods in the UpdateQueryBuilder for withIfSeqNo or withIfPrimaryTerm, but there's no way of getting those from the search, so you cannot fully complete this flow without relying on repositories.

[sothawo]

This is implemented on entity level and so works with operations and repositories. Check the classes used in the tests (https://github.com/spring-projects/spring-data-elasticsearch/blob/main/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplateTests.java#L4351-L4430) and how they are used in the test cases.

[jjbowman2]

Ah okay. Thank you for pointing this out. In our particular use case, we have been using a generic JSONObject as the return instead of entities (mostly due to frequent changes to the data schema), so I was hoping for a way to include the seq no in the response. However, I think we can utilize an entity based work-around when we need this optimistic concurrency solution.