Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CAS not working with @Version [DATACOUCH-623] #934

Closed
spring-projects-issues opened this issue Oct 5, 2020 · 8 comments
Closed

CAS not working with @Version [DATACOUCH-623] #934

spring-projects-issues opened this issue Oct 5, 2020 · 8 comments

Comments

@spring-projects-issues
Copy link

@spring-projects-issues spring-projects-issues commented Oct 5, 2020

David Prevost opened DATACOUCH-623 and commented

I wanted to use @Version (with long version) to use optimistic locking and kick the CAS of couchbase like mentioned in the documentation.

However, after doing an integrated test, I found out that neither the CASMismatchException or the OptimisticLocking exception are thrown. The problem seems pretty much similar to https://jira.spring.io/browse/DATACOUCH-212.

After searching a bit, it seems that if we want CAS to work, we must use couchbaseOperations.replace... However, the current implement of repositories seems to be using upsert. I was not able to test this theory though.

My integrate test was basically doing:

  • Create a User
  • Update the token to add one token then save
  • Build a new User object with a previous version and a new token and the same id
  • Save
  • Expecting Optimitics lock error but it is save without error

 

    public void givenExistingUser_whenChangeByTwoPersonsAndSaved_thenOptimisticError() {

        // Given
        final User savedUser = givenExistingUser();

        // When
        // First person
        savedUser.setTokenIds(Set.of(UUID.randomUUID()));
        userSessionDao.save(savedUser);

        // Second person
        final User savedUser2 =
            UserBuilder.builder().userUuid(savedUser.getUserUuid()).build();
        savedUser2.setVersion(savedUser.getVersion() - 1);
        savedUser2.settokenIds(Set.of(UUID.randomUUID()));

        // Then
        final Throwable throwable = catchThrowable(() -> userSessionDao.save(savedUser2));

        assertThat(throwable).isExactlyInstanceOf(CasMismatchException.class);
    }

 

@Document
public class UserEntity {

    @Id
    private UUID userUuid;

    @Version
    private long version;

    @Field
    @NotNull
    private Set<UUID> tokenIds;

 


Affects: 4.0.4 (Neumann SR4)

Referenced from: pull request #268

Backported to: 4.0.5 (Neumann SR5)

1 votes, 4 watchers

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Oct 5, 2020

Michael Reiche commented

plum117 - I'm not done looking at this yet, but there was an issue with CAS being ignored.  DATACOUCH-595.  It's merged into master, but nowhere else yet

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Oct 5, 2020

David Prevost commented

Oh okok... when release will test this fix

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Oct 5, 2020

Michael Reiche commented

plum117 - just investigated this further - 595 does not fix it.  The issue is that the save() method of the repository uses upsert()  so that it works for both inserts and update.  And we don't have upsert() doing anything with CAS - there was an internal discussion about this - to the effect that if you know you are replacing an existing document, that replace() should be used. 
Ok - this is exactly what you already stated - "if we want CAS to work, we must use couchbaseOperations.replace... However, the current implement of repositories seems to be using upsert."

replace() does honor CAS - but there is no hook to replace() from the repository.  When (if) that is added, your code would get CasMisMatch wrapped in DataIntegrityViolation like this:

org.springframework.dao.DataIntegrityViolationException: Document has been concurrently modified on the server; nested exception is com.couchbase.client.core.error.CasMismatchException: Document has been concurrently modified on the server {"completed":true,"coreId":"0x7a8fc29100000001","idempotent":false,"lastChannelId":"7A8FC29100000001/0000000052F6AA4F","lastDispatchedFrom":"127.0.0.1:49808","lastDispatchedTo":"127.0.0.1:11210","requestId":11,"requestType":"ReplaceRequest","retried":0,"service":

{"bucket":"70b51006-f5a7-4874-b3dc-64e824dd76e8","collection":"_default","documentId":"1","opaque":"0x14","scope":"_default","type":"kv"}

,"status":"EXISTS","timeoutMs":2500,"timings":{"dispatchMicros":347,"encodingMicros":237,"totalMicros":3881,"serverMicros":0}}

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Oct 6, 2020

jpringle commented

Given that optimistic locking completely depends upon using replace() vs upsert() in the couchbase SDK (there's not even a way to provide a CAS value on an upsert operation, and lack of optimistic lock support is a MAJOR issue, it seems the only reasonable path forward is to add an "update()" operation to the CouchbaseRepository interface.

At any rate, in the meantime the documentation should highlight that the only way to get optimistic locking support is to drop back to CouchbaseTemplate.replaceById()

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Oct 6, 2020

David Prevost commented

I agree with jpringle, there seem to be a major flaw here mostly from what it is documented since from the doc Optimistic locking is supposed to work out of the box just by using @Version 

Either the doc or the code need major review!

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Oct 6, 2020

Michael Reiche commented

jpringle plum117

"it seems the only reasonable path forward is to add an "update()" operation to the CouchbaseRepository interface"

That's the change I made yesterday ( I named it replace() instead of update() ).   Please pull the branch indicated in the pull request ( datacouch_623_add_replace_to_repository_for_cas_usage ) to try it out

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Oct 7, 2020

Mark Paluch commented

From what I understand, the difference in behavior originates from calling replace vs. upsert. It should be the responsibility of the Spring Data infrastructure to determine which method to use instead of propagating this responsibility to the caller. 

Spring Data repositories aim to be technology-agnostic so introducing technology-specific methods are violating that concept.

Instead, the implementation should check whether the entity is versioned and call upon save/saveAll the appropriate Template API method (or the Template API should handle this aspect internally)

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Oct 8, 2020

David Prevost commented

Michael Reiche let me know if you change your implementation following last comment before I test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants