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.

Sign up for GitHub

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

Jump to bottom

ISPN-14863 Implement RESP "cluster nodes" command #11025

Merged
merged 1 commit into from
Jun 30, 2023
Merged

ISPN-14863 Implement RESP "cluster nodes" command #11025

merged 1 commit into from
Jun 30, 2023

Conversation

jabolina
Copy link
Member

@jabolina jabolina commented Jun 6, 2023

wburns
wburns reviewed Jun 6, 2023
View reviewed changes
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, just some minor comments. I also am not familiar with the actual REDIS output, but I assume you double checked the validity.

server/resp/src/main/java/org/infinispan/server/resp/commands/cluster/NODES.java Outdated
}

private static void serializeSegments(StringBuilder response, IntSet ranges) {
BitSet bitSet = BitSet.valueOf(ranges.toBitSet());
Copy link
Member

Choose a reason for hiding this comment

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

This should be able to just use the ranges.iterator instead. But this change is not required as it is a pretty small optimization I suspect.

Copy link
Member

Choose a reason for hiding this comment

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

Another option is adding nextSetBit method to IntSet which sounds like a good idea to me. It should be pretty trivial to add the method to the various implementations.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added the nextSetBit method, and updated here and the SHARDS command.

server/resp/src/main/java/org/infinispan/server/resp/commands/cluster/NODES.java Outdated
StringBuilder sb = new StringBuilder();
sb.append(name).append(' ');
sb.append(addressString).append(':').append(port).append('@').append(port);
sb.append(",,shard-id=").append(name).append(' ');
Copy link
Member

Choose a reason for hiding this comment

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

Is the space at the end always needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, in this format, they use space as a separator.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I was thinking it was a bit of a waste if it was always there, even for the last node. But if that is how they want it :)

@jabolina
Copy link
Member Author

jabolina commented Jun 6, 2023

Updated with suggestions.

@jabolina
Copy link
Member Author

jabolina commented Jun 6, 2023

I need to find another way to check this beyond using the Lettuce client to validate the response.

@jabolina
Copy link
Member Author

jabolina commented Jun 6, 2023

Doing a comparison by running with echo 'cluster-enabled yes' | redis-server -, my server is version Redis 7.0.10 (00000000/0) 64 bit and it is assigned a few slots automatically. Running the command with redis-cli:

127.0.0.1:6379> CLUSTER NODES
308345ab50e60cdf538e30d2fd5e3de671abfb0c :6379@16379 myself,master - 0 0 0 connected 238 5282 5798 9755 12291 12539
127.0.0.1:6379>

Running with ISPN:

127.0.0.1:11222> CLUSTER NODES
jbolina-64043 172.17.0.1:7800@7800,,shard-id=jbolina-64043 myself,master - 0 1686081424 1 connected 0-255
127.0.0.1:11222>

The ISPN output includes the shard-id in the auxiliary field. Per the doc, this field is always included in version >= 7.2.0.
I need to update to return the correct address in the protocol server.

@jabolina
Copy link
Member Author

jabolina commented Jun 7, 2023

Doing a test with three servers (server-1, server-2, server-3):

127.0.0.1:11222> cluster nodes
server-1 127.0.0.1:11222@60836,,shard-id=server-1 myself,master - 0 1686141297 9 connected 0-255
server-2 127.0.0.1:11232@60836,,shard-id=server-2 master - 0 1686141297 9 connected 0-255
server-3 127.0.0.1:11322@60836,,shard-id=server-3 master - 0 1686141297 9 connected 0-255
127.0.0.1:11222>

I think that should be it, going to squash the commits. I guess a point of attention is the way I did to retrieve the resp server.

@tristantarrant
Copy link
Member

I need to find another way to check this beyond using the Lettuce client to validate the response.

Maybe introduce a Vert.x Redis client test ?

@jabolina
Copy link
Member Author

jabolina commented Jun 9, 2023

I think adding the Vert.x might be worth it, especially since we're missing more throughout coverage in cluster mode, checking the CRC16 implementation. Although, the client seems to use the CLUSTER SLOTS command (link).

The CLUSTER SLOTS is next on my list. I'll add the client then.

@jabolina
Copy link
Member Author

Pushed a small change. I was not retrieving the segments owned (that's why all nodes had 0-255). Now we have:

jbolina-22538 127.0.0.1:11222@54392,,shard-id=jbolina-22538 myself,master - 0 1686572258 5 connected 74-136 142-145 157-168 181-181 187-210 212-217 219-219 225-226 230-247
jbolina-61882 127.0.0.1:11322@54392,,shard-id=jbolina-61882 master - 0 1686572258 5 connected 0-73 137-141 146-156 169-180 182-186 211-211 218-218 220-224 227-229 248-255

@jabolina jabolina force-pushed the ISPN-14963 branch 2 times, most recently from cdb78bf to 7c33b78 Compare June 13, 2023 16:15
@karesti
Copy link
Contributor

karesti commented Jun 14, 2023

@tristantarrant @wburns something else ?

wburns
wburns reviewed Jun 14, 2023
View reviewed changes
server/resp/src/main/java/org/infinispan/server/resp/commands/cluster/NODES.java Outdated
hash = currentCH;
}
EmbeddedCacheManager ecm = SecurityActions.getEmbeddedCacheManager(respCache);
cs = requestPerOwner.computeIfAbsent(ecm.getAddress(), ignore -> requestClusterInformation(handler, ctx, ecm, topology));
Copy link
Member

Choose a reason for hiding this comment

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

Isn't ecm.getAddress() always the same thing irrespective of the consistent hash? Not sure I understand the requestPerOwner map usage.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not always, it depends on the server handling the request. The result is slightly different and includes a myself in the returned nodes list. We have a test covering this, too. With two servers, each client connects to one server.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, totally missed the point. Updated now.

@wburns wburns merged commit 1b51e7d into infinispan:main Jun 30, 2023
3 of 4 checks passed
@wburns
Copy link
Member

wburns commented Jun 30, 2023

Integrated into main, thanks @jabolina !

@jabolina jabolina deleted the ISPN-14963 branch June 30, 2023 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wburns wburns wburns left review comments

Assignees
No one assigned
Labels
Question
Projects
None yet
Milestone
No milestone
4 participants
@jabolina @tristantarrant @karesti @wburns