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-14597 SINTER resp command #11035

Merged
merged 2 commits into from
Jul 5, 2023
Merged

Conversation

rigazilla
Copy link
Contributor

@rigazilla rigazilla commented Jun 14, 2023

A bunch of commands for sets.
Code was almost the same, so this PR refers to 3 jiras.

https://issues.redhat.com/browse/ISPN-14597
SINTER SINTERCARD SINTERSTORE

https://issues.redhat.com/browse/ISPN-14598
SMEMBERS

https://issues.redhat.com/browse/ISPN-14595
SCARD

@rigazilla rigazilla force-pushed the SINTER-14597 branch 2 times, most recently from 62734d7 to fcc32c0 Compare June 16, 2023 07:46
@rigazilla rigazilla marked this pull request as ready for review June 16, 2023 09:51
@rigazilla rigazilla force-pushed the SINTER-14597 branch 3 times, most recently from 62c1284 to 1365923 Compare June 19, 2023 13:03
@rigazilla rigazilla mentioned this pull request Jun 20, 2023
Copy link
Contributor

@karesti karesti left a comment

Choose a reason for hiding this comment

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

just a couple of changes for now, I will do a deeper review after
@jabolina as well

Copy link
Member

@jabolina jabolina left a comment

Choose a reason for hiding this comment

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

A few things. Also, I missed some tests for the embedded set in multimap.

Copy link
Member

@jabolina jabolina left a comment

Choose a reason for hiding this comment

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

Just some small things, and we're good. We should document the behavior of these commands with other concurrent operations over the same sets. As I guess we don't have the same guarantees as in Redis.

@rigazilla
Copy link
Contributor Author

Just some small things, and we're good. We should document the behavior of these commands with other concurrent operations over the same sets. As I guess we don't have the same guarantees as in Redis.

I changed the implementation to use atomic getAll, this way the behaviour should be the same as Redis. Looking at the docs I can't find anything special for the intersect commands implemented here.

please squash the commits if all good. getAll is a separate commit

Copy link
Member

@jabolina jabolina left a comment

Choose a reason for hiding this comment

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

A few comments, but we can continue.

Comment on lines +87 to +88
// await(set.add(NAMES_KEY, ELAIA));
// assertValuesAndOwnership(NAMES_KEY, ELAIA);
Copy link
Member

Choose a reason for hiding this comment

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

Do we have an issue with these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, it's a mistake. I restored these lines in #10906. Thanks!

* @param keys, collection of keys to be get
* @return {@link CompletionStage} containing a {@link }
*/
public CompletableFuture<Map<K, SetBucket<V>>> getAll(Set<K> keys) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe in future changes, we update to avoid returning the bucket as it is just an abstraction.


public static Set<WrappedByteArray> checkTypesAndReturnEmpty(Collection<SetBucket<WrappedByteArray>> buckets) {
var iter = buckets.iterator();
// access all items to check for error
Copy link
Member

Choose a reason for hiding this comment

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

Class cast errors?

@karesti karesti merged commit 3300c46 into infinispan:main Jul 5, 2023
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants