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

ISPN-14654 MSETNX #10944

Merged
merged 2 commits into from
Sep 13, 2023
Merged

ISPN-14654 MSETNX #10944

merged 2 commits into from
Sep 13, 2023

Conversation

rigazilla
Copy link
Contributor

@tristantarrant
Copy link
Member

Please add a row to the table in documentation/src/main/asciidoc/topics/ref_redis_commands.adoc

entries.put(arguments.get(i), arguments.get(++i));
}
// Change to loop?
var existingEntries = handler.cache().getAll(entries.keySet());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the current implementation:

  1. entries = getAll(keys)
  2. if entries.size()==0 then putAll(newEntries)

if an concurrent put(conflictingEntry) operation happens btween 1 and 2, there's no way to keep the semantic of the operation, I mean:

  • order (msetnx then set) must result in conflictEntry to be present, but it has been actually overwritten
  • order (set then msetnx) must result in msetnx not be executed since concEntry is present.

Replacing putAll with a loop of putIfAbsent results in a consistent story (msetnx then set), the cost is to replace an atomic operation with a loop.

I can't see any better solution without involving transactions.

any preference/though?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think there is any way to do this without transactions or some type of 2 step operation when in a cluster. We can do simpler things with a single node, but in a cluster we really need something like a distributed transaction, a new command won't remedy that sadly.

@tristantarrant
Copy link
Member

Rebase please

@rigazilla
Copy link
Contributor Author

rebased

@tristantarrant
Copy link
Member

@rigazilla needs rebase. @karesti @jabolina can you review?

Copy link
Member

@wburns wburns left a comment

Choose a reason for hiding this comment

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

Seems fine to me, if anyone else wants to double check.

@jabolina jabolina merged commit 680b1cd into infinispan:main Sep 13, 2023
3 of 4 checks passed
@jabolina
Copy link
Member

Thanks, @rigazilla!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants