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-14907 Implement RESP "cluster shards" command #10930

Merged
merged 1 commit into from Jun 5, 2023

Conversation

jabolina
Copy link
Member

https://issues.redhat.com/browse/ISPN-14229

  • Implements the CLUSTER SHARDS command.

This is part 1, do not close the JIRA yet.

@jabolina
Copy link
Member Author

Let me cover this a bit more with tests for single nodes, too

@karesti
Copy link
Contributor

karesti commented May 11, 2023

@jabolina open a new jira if you have two parts

@jabolina
Copy link
Member Author

I'll put an on-hold on this momentarily while I work on some fixes first.

@tristantarrant
Copy link
Member

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

@jabolina jabolina changed the title ISPN-14229 Implement RESP "cluster" commands ISPN-14907 Implement RESP "cluster shards" command May 25, 2023
@jabolina
Copy link
Member Author

@karesti Created a JIRA and updated this PR.

@jabolina jabolina force-pushed the ISPN-14229-shards branch 2 times, most recently from a86a01d to 50b40a0 Compare May 31, 2023 21:02
byte[] possibleBytes = bytes;
if (length == possibleBytes.length) {
boolean matches = true;
for (int i = 0; i < possibleBytes.length; ++i) {
Copy link
Member

Choose a reason for hiding this comment

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

Technically this is adding an additional case insensitive char comparison, but I guess that is okay.

@jabolina jabolina force-pushed the ISPN-14229-shards branch 2 times, most recently from 8534dae to 927678f Compare June 1, 2023 16:44
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.

Changes look good, just a couple more things.

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.

Another thought is we could retry or wait for a topology update to complete. But this is probably too complex for this with little benefit.

I am mainly just concerned about using a null owner and ignoring some backups when they aren't named... I think having something is better than just not including them.

@jabolina jabolina force-pushed the ISPN-14229-shards branch 2 times, most recently from c07032f to 475062d Compare June 2, 2023 12:06
@jabolina
Copy link
Member Author

jabolina commented Jun 2, 2023

Yeah, we could create all the required fields using the address.toString() as a placeholder for name and address. And we can say the node's health, so I put it to use loading in these cases, meaning the node might serve requests in the future.

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.

LGTM, but there are CI failures.

@jabolina
Copy link
Member Author

jabolina commented Jun 2, 2023

Fixed the test.

@wburns wburns merged commit b8187f8 into infinispan:main Jun 5, 2023
3 of 4 checks passed
@wburns
Copy link
Member

wburns commented Jun 5, 2023

Integrated into main, thanks @jabolina !

@jabolina jabolina deleted the ISPN-14229-shards branch June 5, 2023 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants