CASSANDRA-15319 against 2.2 #354
Conversation
The current test relies on the order of nodes returned by the snitch to include node3, which SimpleSnitch does. With support for other snitches coming then the test should be able to handle any order of nodes - so make sure all nodes are present.
And can now simplify the constructor.
Resolves issue with trying to call getMessagingVersion on nodes that are not started, will test if it causes problems with mixed version upgrades once all branches are updated.
| this.thread = thread; | ||
| } | ||
|
|
||
| static TraceEntry fromObjects(Object[] objects) |
There was a problem hiding this comment.
The method name doesn't clearly convey the use of the method. Could we call this something different? This method converts a row result (list of deserialized objects) to a trace entry. Perhaps call this fromRowResultObjects()? I'm open to other name suggestions too.
There was a problem hiding this comment.
Renamed to fromResultObjects are requested.
There was a problem hiding this comment.
Thank you for the patch, and sorry for the longer turnaround.
I've reviewed and it looks good. I only wanted to suggest, since we're using consecutive numbers anyways, to switch from using a map to the array, which slightly simplifies the code. To make sure this suggestion is right, I've tried to implement in myself in a branch [1], but please feel free to ignore my implementation.
There is a couple more minor things: imports, error message in the assert, and a removing the extra initialisation code in one of the tests.
| if (nodeIdTopology.isEmpty()) | ||
| throw new IllegalStateException("Topology is empty. It must have an entry for every nodeId."); | ||
|
|
||
| IntStream.rangeClosed(1, nodeIdTopology.size()).forEach(nodeId -> { |
There was a problem hiding this comment.
Not that this really matters here, but why should we use stream here instead of a simple for loop? I really see no advantages.
There was a problem hiding this comment.
It just seemed more concise than the for loop in somewhere that wasn't performance sensitive, but I'm still finding my way with appropriate use of streams.
| throw new IllegalStateException("Cluster must have at least one node"); | ||
|
|
||
| if (nodeIdTopology == null) | ||
| nodeIdTopology = IntStream.rangeClosed(1, nodeCount).boxed() |
There was a problem hiding this comment.
Same here. I might be just thinking a bit old-school)
There was a problem hiding this comment.
Same reasoning as above.
|
Thanks @ifesdjeen, I do like that the change reduces a possible source of user error of leaving gaps in the nodeId sequence, but I guard against that with an explicit runtime check. The reason I use the map approach is so that if a user wants, they can specify exactly where each node is, so they know where |
|
All right. My thinking was that since we have a stream API for filtering / finding specific nodes in rack/datacenter, people would probably use those, so there is no need to expose internal identifiers. However, I'm not insisting, just elaborating on my thinking when I was suggesting this. |
|
Committed as 58a5ce1 |
See CASSANDRA-15319