You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2020/07/24 16:32:55 UTC
[GitHub] [cassandra] jonmeredith opened a new pull request #692: CASSANDRA-15937 JMX output inconsistencies from CASSANDRA-7544 storage-port-configurable-per-node
jonmeredith opened a new pull request #692:
URL: https://github.com/apache/cassandra/pull/692
See [CASSANDRA-15937](https://issues.apache.org/jira/browse/CASSANDRA-15937)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] jonmeredith commented on a change in pull request #692: CASSANDRA-15937 JMX output inconsistencies from CASSANDRA-7544 storage-port-configurable-per-node
Posted by GitBox <gi...@apache.org>.
jonmeredith commented on a change in pull request #692:
URL: https://github.com/apache/cassandra/pull/692#discussion_r471016996
##########
File path: test/unit/org/apache/cassandra/gms/EndpointStateTest.java
##########
@@ -114,11 +117,11 @@ public void testMultiThreadWriteConsistency() throws InterruptedException
/**
* Test that two threads can update the state map concurrently.
*/
- private void innerTestMultiThreadWriteConsistency() throws InterruptedException
+ private void innerTestMultiThreadWriteConsistency() throws InterruptedException, UnknownHostException
{
final Token token = DatabaseDescriptor.getPartitioner().getRandomToken();
final List<Token> tokens = Collections.singletonList(token);
- final String ip = "127.0.0.1";
+ final InetAddress ip = InetAddress.getByName("127.0.0.1");
Review comment:
ack - much better, thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] jonmeredith commented on pull request #692: CASSANDRA-15937 JMX output inconsistencies from CASSANDRA-7544 storage-port-configurable-per-node
Posted by GitBox <gi...@apache.org>.
jonmeredith commented on pull request #692:
URL: https://github.com/apache/cassandra/pull/692#issuecomment-681185827
Merged as a062ff5f9f0d3221deaaaaf8fccbcd21e933e7cd
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] jonmeredith closed pull request #692: CASSANDRA-15937 JMX output inconsistencies from CASSANDRA-7544 storage-port-configurable-per-node
Posted by GitBox <gi...@apache.org>.
jonmeredith closed pull request #692:
URL: https://github.com/apache/cassandra/pull/692
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] jonmeredith commented on a change in pull request #692: CASSANDRA-15937 JMX output inconsistencies from CASSANDRA-7544 storage-port-configurable-per-node
Posted by GitBox <gi...@apache.org>.
jonmeredith commented on a change in pull request #692:
URL: https://github.com/apache/cassandra/pull/692#discussion_r471001246
##########
File path: src/java/org/apache/cassandra/locator/InetAddressAndPort.java
##########
@@ -119,11 +121,22 @@ public int compareTo(InetAddressAndPort o)
return Integer.compare(port, o.port);
}
+ public String getHostAddressAndPort()
+ {
+ return getHostAddress(true);
+ }
+
+ static final Pattern jmxIncompatibleChars = Pattern.compile("[\\[\\]:]");
Review comment:
ack
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] jonmeredith commented on a change in pull request #692: CASSANDRA-15937 JMX output inconsistencies from CASSANDRA-7544 storage-port-configurable-per-node
Posted by GitBox <gi...@apache.org>.
jonmeredith commented on a change in pull request #692:
URL: https://github.com/apache/cassandra/pull/692#discussion_r471016978
##########
File path: src/java/org/apache/cassandra/metrics/ClientMetrics.java
##########
@@ -136,7 +136,7 @@ private int countConnectedClients()
for (Server server : servers)
for (ClientStat stat : server.recentClientStats())
- stats.add(stat.asMap());
+ stats.add(new HashMap(stat.asMap()));
Review comment:
ack
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] jonmeredith commented on a change in pull request #692: CASSANDRA-15937 JMX output inconsistencies from CASSANDRA-7544 storage-port-configurable-per-node
Posted by GitBox <gi...@apache.org>.
jonmeredith commented on a change in pull request #692:
URL: https://github.com/apache/cassandra/pull/692#discussion_r471015576
##########
File path: src/java/org/apache/cassandra/locator/InetAddressAndPort.java
##########
@@ -151,7 +164,33 @@ public String toString(boolean withPort)
public static String toString(InetAddress address, int port)
{
- return HostAndPort.fromParts(address.getHostAddress(), port).toString();
+ String hostName = Objects.toString(address.getHostName(), "");
Review comment:
Updated and looked a little closer. Ended up rewriting to preserve the `InetAddress.toString` resolver behavior.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] dcapwell commented on a change in pull request #692: CASSANDRA-15937 JMX output inconsistencies from CASSANDRA-7544 storage-port-configurable-per-node
Posted by GitBox <gi...@apache.org>.
dcapwell commented on a change in pull request #692:
URL: https://github.com/apache/cassandra/pull/692#discussion_r464629070
##########
File path: src/java/org/apache/cassandra/locator/InetAddressAndPort.java
##########
@@ -119,11 +121,22 @@ public int compareTo(InetAddressAndPort o)
return Integer.compare(port, o.port);
}
+ public String getHostAddressAndPort()
+ {
+ return getHostAddress(true);
+ }
+
+ static final Pattern jmxIncompatibleChars = Pattern.compile("[\\[\\]:]");
Review comment:
should be private
##########
File path: src/java/org/apache/cassandra/locator/InetAddressAndPort.java
##########
@@ -151,7 +164,33 @@ public String toString(boolean withPort)
public static String toString(InetAddress address, int port)
{
- return HostAndPort.fromParts(address.getHostAddress(), port).toString();
+ String hostName = Objects.toString(address.getHostName(), "");
Review comment:
Docs would be good, so the caller knows what to expect and how this related to getHostAddressAndPort() and getHostAddressAndPortForJMX()
##########
File path: test/unit/org/apache/cassandra/gms/EndpointStateTest.java
##########
@@ -114,11 +117,11 @@ public void testMultiThreadWriteConsistency() throws InterruptedException
/**
* Test that two threads can update the state map concurrently.
*/
- private void innerTestMultiThreadWriteConsistency() throws InterruptedException
+ private void innerTestMultiThreadWriteConsistency() throws InterruptedException, UnknownHostException
{
final Token token = DatabaseDescriptor.getPartitioner().getRandomToken();
final List<Token> tokens = Collections.singletonList(token);
- final String ip = "127.0.0.1";
+ final InetAddress ip = InetAddress.getByName("127.0.0.1");
Review comment:
sadly this will try to resolve, since this should be loopback its mostly fine, but would be good to use `java.net.InetAddress#getByAddress(java.lang.String, byte[])`
```
InetAddress.getByAddress(null, new byte[] { 127, 0, 0, 1})
```
this throws the same exception, but doesn't try to resolve
##########
File path: src/java/org/apache/cassandra/locator/InetAddressAndPort.java
##########
@@ -119,11 +121,22 @@ public int compareTo(InetAddressAndPort o)
return Integer.compare(port, o.port);
}
+ public String getHostAddressAndPort()
+ {
+ return getHostAddress(true);
+ }
+
+ static final Pattern jmxIncompatibleChars = Pattern.compile("[\\[\\]:]");
Review comment:
should be `JMX_INCOMPATIBLE_CHARS`
##########
File path: src/java/org/apache/cassandra/metrics/ClientMetrics.java
##########
@@ -136,7 +136,7 @@ private int countConnectedClients()
for (Server server : servers)
for (ClientStat stat : server.recentClientStats())
- stats.add(stat.asMap());
+ stats.add(new HashMap(stat.asMap()));
Review comment:
for maintenance, would be good to document that this is because `asMap` returns guava, so need to convert to java for jmx
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] dcapwell commented on a change in pull request #692: CASSANDRA-15937 JMX output inconsistencies from CASSANDRA-7544 storage-port-configurable-per-node
Posted by GitBox <gi...@apache.org>.
dcapwell commented on a change in pull request #692:
URL: https://github.com/apache/cassandra/pull/692#discussion_r473368193
##########
File path: src/java/org/apache/cassandra/locator/InetAddressAndPort.java
##########
@@ -149,9 +170,42 @@ public String toString(boolean withPort)
}
}
+ /** Format an InetAddressAndPort in the same style as InetAddress.toString.
+ * The string returned is of the form: hostname / literal IP address : port
+ * (without the whitespace). Literal IPv6 addresses will be wrapped with [ ]
+ * to make the port number clear.
+ *
+ * If the host name is unresolved, no reverse name service lookup
+ * is performed. The hostname part will be represented by an empty string.
+ *
+ * @param address InetAddress to convert String
+ * @param port Port number to convert to String
+ * @return String representation of the IP address and port
+ */
public static String toString(InetAddress address, int port)
{
- return HostAndPort.fromParts(address.getHostAddress(), port).toString();
+ String addressToString = address.toString(); // cannot use getHostName as it resolves
+ int nameLength = addressToString.lastIndexOf('/'); // use last index to prevent ambiguity if host name contains /
+ assert nameLength >= 0 : "InetAddress.toString format may have changed, expecting /";
Review comment:
should include the string `addressToString` so debugging actually knows what caused the failure
##########
File path: test/distributed/org/apache/cassandra/distributed/test/GossipSettlesTest.java
##########
@@ -18,27 +18,143 @@
package org.apache.cassandra.distributed.test;
+import java.net.InetAddress;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Map;
+import java.util.regex.Pattern;
+import java.util.stream.Collectors;
+
+import org.junit.Assert;
import org.junit.Test;
-import org.apache.cassandra.distributed.api.ICluster;
+import org.apache.cassandra.config.DatabaseDescriptor;
+import org.apache.cassandra.distributed.Cluster;
+import org.apache.cassandra.gms.FailureDetector;
+import org.apache.cassandra.gms.Gossiper;
+import org.apache.cassandra.net.MessagingService;
+import org.apache.cassandra.repair.SystemDistributedKeyspace;
+import org.apache.cassandra.schema.SchemaConstants;
+import org.apache.cassandra.service.StorageProxy;
+import org.apache.cassandra.service.StorageService;
+import org.apache.cassandra.utils.ByteBufferUtil;
import static org.apache.cassandra.distributed.api.Feature.GOSSIP;
import static org.apache.cassandra.distributed.api.Feature.NETWORK;
// TODO: this test should be removed after running in-jvm dtests is set up via the shared API repository
public class GossipSettlesTest extends TestBaseImpl
{
-
@Test
public void testGossipSettles() throws Throwable
{
/* Use withSubnet(1) to prove seed provider is set correctly - without the fix to pass a seed provider, this test fails */
- try (ICluster cluster = builder().withNodes(3)
- .withConfig(config -> config.with(GOSSIP).with(NETWORK))
- .withSubnet(1)
- .start())
+ try (Cluster cluster = builder().withNodes(3)
+ .withConfig(config -> config.with(GOSSIP).with(NETWORK))
+ .withSubnet(1)
+ .start())
{
+ // Verify the 4.0 WithPort versions of status reporting methods match their InetAddress
+ // counterparts. Disable Gossip first to prevent any bump in heartbeats that would
+ // invalidate the comparison. Compare the newer WithPort versions by adding the
+ // storage port to IP addresses in keys/values/strings as appropriate.
+ cluster.forEach(i -> i.runOnInstance(() -> { Gossiper.instance.stop(); }));
+ cluster.get(1).runOnInstance(() -> {
+
+ // First prove that the storage port is added
+ Assert.assertEquals("stuff 127.0.0.1:7012 morestuff 127.0.0.2:7012", addStoragePortToIP("stuff 127.0.0.1 morestuff 127.0.0.2"));
+
+ FailureDetector fd = ((FailureDetector) FailureDetector.instance);
+ Assert.assertEquals(addStoragePortToInstanceName(fd.getAllEndpointStates(false)),
+ fd.getAllEndpointStates(true));
+ Assert.assertEquals(addPortToKeys(fd.getSimpleStates()), fd.getSimpleStatesWithPort());
+
+ StorageProxy sp = StorageProxy.instance;
+ Assert.assertEquals(addPortToSchemaVersions(sp.getSchemaVersions()), sp.getSchemaVersionsWithPort());
+
+ StorageService ss = StorageService.instance;
+ Assert.assertEquals(addPortToValues(ss.getTokenToEndpointMap()), ss.getTokenToEndpointWithPortMap());
+ Assert.assertEquals(addPortToKeys(ss.getEndpointToHostId()), ss.getEndpointWithPortToHostId());
+ Assert.assertEquals(addPortToValues(ss.getHostIdToEndpoint()), ss.getHostIdToEndpointWithPort());
+ Assert.assertEquals(addPortToKeys(ss.getLoadMap()), ss.getLoadMapWithPort());
+ Assert.assertEquals(addPortToList(ss.getLiveNodes()), ss.getLiveNodesWithPort());
+ List<String> naturalEndpointsAddedPort = ss.getNaturalEndpoints(SchemaConstants.DISTRIBUTED_KEYSPACE_NAME,
+ SystemDistributedKeyspace.VIEW_BUILD_STATUS, "dummy").stream()
+ .map(e -> addStoragePortToIP(e.getHostAddress())).collect(Collectors.toList());
+ Assert.assertEquals(naturalEndpointsAddedPort,
+ ss.getNaturalEndpointsWithPort(SchemaConstants.DISTRIBUTED_KEYSPACE_NAME,
+ SystemDistributedKeyspace.VIEW_BUILD_STATUS, "dummy"));
+ naturalEndpointsAddedPort = ss.getNaturalEndpoints(SchemaConstants.DISTRIBUTED_KEYSPACE_NAME, ByteBufferUtil.EMPTY_BYTE_BUFFER).stream()
+ .map(e -> addStoragePortToIP(e.getHostAddress())).collect(Collectors.toList());
+ Assert.assertEquals(naturalEndpointsAddedPort,
+ ss.getNaturalEndpointsWithPort(SchemaConstants.DISTRIBUTED_KEYSPACE_NAME, ByteBufferUtil.EMPTY_BYTE_BUFFER));
+
+
+ // Difference in key type... convert to String and add the port to the older format
+ Map<String, Float> getOwnershipKeyAddedPort = ss.getOwnership().entrySet().stream()
+ .collect(Collectors.<Map.Entry<InetAddress, Float>, String, Float>toMap(
+ e -> addStoragePortToIP(e.getKey().toString()),
+ Map.Entry::getValue));
+ Assert.assertEquals(getOwnershipKeyAddedPort, ss.getOwnershipWithPort());
+
+ MessagingService ms = MessagingService.instance();
+ Assert.assertEquals(addPortToKeys(ms.getTimeoutsPerHost()), ms.getTimeoutsPerHostWithPort());
+ Assert.assertEquals(addPortToKeys(ms.getLargeMessagePendingTasks()), ms.getLargeMessagePendingTasksWithPort());
+ Assert.assertEquals(addPortToKeys(ms.getLargeMessageCompletedTasks()), ms.getLargeMessageCompletedTasksWithPort());
+ Assert.assertEquals(addPortToKeys(ms.getLargeMessageDroppedTasks()), ms.getLargeMessageDroppedTasksWithPort());
+ Assert.assertEquals(addPortToKeys(ms.getSmallMessagePendingTasks()), ms.getSmallMessagePendingTasksWithPort());
+ Assert.assertEquals(addPortToKeys(ms.getSmallMessageCompletedTasks()), ms.getSmallMessageCompletedTasksWithPort());
+ Assert.assertEquals(addPortToKeys(ms.getSmallMessageDroppedTasks()), ms.getSmallMessageDroppedTasksWithPort());
+ Assert.assertEquals(addPortToKeys(ms.getGossipMessagePendingTasks()), ms.getGossipMessagePendingTasksWithPort());
+ Assert.assertEquals(addPortToKeys(ms.getGossipMessageCompletedTasks()), ms.getGossipMessageCompletedTasksWithPort());
+ Assert.assertEquals(addPortToKeys(ms.getGossipMessageDroppedTasks()), ms.getGossipMessageDroppedTasksWithPort());
+ });
}
}
+
+ final static Pattern IP4_ADDRESS = Pattern.compile("(127\\.0\\.\\d{1,3}\\.\\d{1,3})");
+
+ static String addStoragePortToIP(String s)
+ {
+ return IP4_ADDRESS.matcher(s).replaceAll("$1:" + DatabaseDescriptor.getStoragePort());
+ }
+
+ static String addStoragePortToInstanceName(String s)
+ {
+ return Arrays.stream(s.split("\n")).map(line -> {
+ if (line.startsWith(" "))
Review comment:
nit: `return line.startsWith(" ") ? line : addStoragePortToIP(line);`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org