Skip to content

UNOMI-371 add optional support for optimistic concurrency control (if_seq_no)#200

Closed
lyogev wants to merge 1 commit intoapache:masterfrom
YotpoLtd:optimistic_cuncurrency
Closed

UNOMI-371 add optional support for optimistic concurrency control (if_seq_no)#200
lyogev wants to merge 1 commit intoapache:masterfrom
YotpoLtd:optimistic_cuncurrency

Conversation

@lyogev
Copy link
Copy Markdown
Contributor

@lyogev lyogev commented Sep 21, 2020

When profiles are written at the same time (multiple events on the same profile for example, or segment updates during other segment updates) sometimes we are getting inconsistencies with the final profile.
This PR adds an optional flag to start using elasticsearch optimistic concurrency mechanism
This, in conjunction with the throwExceptions flag allows the client to be aware of such a case and retry if needed.

@lyogev
Copy link
Copy Markdown
Contributor Author

lyogev commented Sep 24, 2020

@sergehuber I'm thinking that maybe to make this a more robust change we need to get the PersistenceService to return the new saved object whenever a save is triggered (changing the signature for all the save functions), WDYT?

Copy link
Copy Markdown
Contributor

@sergehuber sergehuber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. I'm requesting a change on the name of a field because I'm afraid it's going to add confusion. Unfortunately I don't have a better name to provide at this point. Options maybe ?

protected String itemType;
protected String scope;
protected Long version;
protected Map<String, Object> metadata = new HashMap<>();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having a problem with this name. We already have a MetadataItem class that has Metadata object associated with it. I'm afraid this name might cause confusion. I understand why it was introduced but I'm wondering if we could find another name for it ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lyogev have you seen my previous message about the metadata name ? Once this is fixed we could merge this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @sergehuber yes I've seen the comment and I will update, but we are adding more code here soon as we are already using this mode, and we discovered some more issues. will update soon

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok thanks for the feedback. Will probably merge this after 1.5.3 then.

@sergehuber
Copy link
Copy Markdown
Contributor

I just realized I forgot to answer your question about the save methods. That's ok with me as long as we don't change any REST or GraphQL API endpoints.

@lyogev lyogev force-pushed the optimistic_cuncurrency branch from 44003de to 52a0005 Compare October 27, 2020 10:19
@sergehuber
Copy link
Copy Markdown
Contributor

sergehuber commented Nov 19, 2020

Hello @lyogev , due to other merges this PR now conflicts with the master. Could you fix it please ?

Also for the metadata I could suggest we rename it to systemMetadata ?

Thanks for your help and contribution.

Regards,
Serge...

@giladw
Copy link
Copy Markdown
Contributor

giladw commented Dec 2, 2020

Hi @sergehuber ,
im opening a new PR with the requested changes. and closing this one.
i hope its ok :)

@lyogev
Copy link
Copy Markdown
Contributor Author

lyogev commented Dec 2, 2020

Closing this PR and moving to #223

@lyogev lyogev closed this Dec 2, 2020
@giladw giladw deleted the optimistic_cuncurrency branch January 19, 2021 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants