You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by aweisberg <gi...@git.apache.org> on 2018/02/08 17:46:36 UTC
[GitHub] cassandra pull request #191: 13993
GitHub user aweisberg opened a pull request:
https://github.com/apache/cassandra/pull/191
13993
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jasobrown/cassandra 13993
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cassandra/pull/191.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #191
----
commit 106ba8046bab1932953b726bfc2b22b1c69d618f
Author: Jason Brown <ja...@...>
Date: 2017-09-29T18:53:35Z
Implement block-for functionality
commit d1493c9976637e281a1a2bfd2b0d7871da201084
Author: Jason Brown <ja...@...>
Date: 2018-01-30T12:40:53Z
renamed a few variables
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra pull request #191: 13993
Posted by aweisberg <gi...@git.apache.org>.
Github user aweisberg commented on a diff in the pull request:
https://github.com/apache/cassandra/pull/191#discussion_r167016249
--- Diff: src/java/org/apache/cassandra/net/MessagingService.java ---
@@ -1664,4 +1676,113 @@ public static boolean isEncryptedConnection(InetAddressAndPort address)
}
return true;
}
+
+ public void blockForPeers()
+ {
+ // TODO make these yaml props?
+ int alivePercent = Integer.getInteger(Config.PROPERTY_PREFIX + "blockForPeers.percent", 70);
+ if (alivePercent < 0)
+ alivePercent = 0;
+ else if (alivePercent > 100)
+ alivePercent = 100;
+
+ int aliveTimeoutSecs = Integer.getInteger(Config.PROPERTY_PREFIX + "blockForPeers.timeout_in_secs", 10);
+ if (aliveTimeoutSecs < 0)
+ aliveTimeoutSecs = 1;
+ else if (aliveTimeoutSecs > 100)
+ aliveTimeoutSecs = 100;
+
+ if (alivePercent > 0)
+ blockForPeers(alivePercent, aliveTimeoutSecs);
+ }
+
+ private void blockForPeers(int targetAlivePercent, int aliveTimeoutSecs)
+ {
+ // grab a snapshot of the current cluster from Gossiper. this is completely prone to race conditions, but it's
+ // good enough for the purposes of blocking until some certain percentage of nodes are considered 'alive'/connected.
+ Set<Map.Entry<InetAddressAndPort, EndpointState>> peers = new HashSet<>(Gossiper.instance.getEndpointStates());
--- End diff --
When we reach this how do we know that Gossiper will be seeded with any endpoint states so we know to wait on a realistic portion of the cluster?
I assume it's implicit in the bootstrapping process before we get to this point?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra pull request #191: 13993
Posted by jasobrown <gi...@git.apache.org>.
Github user jasobrown commented on a diff in the pull request:
https://github.com/apache/cassandra/pull/191#discussion_r168185310
--- Diff: src/java/org/apache/cassandra/net/MessagingService.java ---
@@ -1664,4 +1676,113 @@ public static boolean isEncryptedConnection(InetAddressAndPort address)
}
return true;
}
+
+ public void blockForPeers()
+ {
+ // TODO make these yaml props?
+ int alivePercent = Integer.getInteger(Config.PROPERTY_PREFIX + "blockForPeers.percent", 70);
+ if (alivePercent < 0)
+ alivePercent = 0;
+ else if (alivePercent > 100)
+ alivePercent = 100;
+
+ int aliveTimeoutSecs = Integer.getInteger(Config.PROPERTY_PREFIX + "blockForPeers.timeout_in_secs", 10);
+ if (aliveTimeoutSecs < 0)
+ aliveTimeoutSecs = 1;
+ else if (aliveTimeoutSecs > 100)
+ aliveTimeoutSecs = 100;
+
+ if (alivePercent > 0)
+ blockForPeers(alivePercent, aliveTimeoutSecs);
+ }
+
+ private void blockForPeers(int targetAlivePercent, int aliveTimeoutSecs)
+ {
+ // grab a snapshot of the current cluster from Gossiper. this is completely prone to race conditions, but it's
+ // good enough for the purposes of blocking until some certain percentage of nodes are considered 'alive'/connected.
+ Set<Map.Entry<InetAddressAndPort, EndpointState>> peers = new HashSet<>(Gossiper.instance.getEndpointStates());
+
+ // remove current node from the set
+ peers = peers.stream()
+ .filter(entry -> !entry.getKey().equals(FBUtilities.getBroadcastAddressAndPort()))
+ .collect(Collectors.toSet());
+
+ final int totalSize = peers.size();
+
+ // don't block if there's no other nodes in the cluster (or we don't know about them)
+ if (totalSize <= 1)
+ return;
+
+ logger.info("choosing to block until {}% of peers are marked alive; max time to wait = {} seconds", targetAlivePercent, aliveTimeoutSecs);
+
+ // first, send out a ping message to open up the non-gossip connections
+ AtomicInteger connectedCount = sendPingMessages(peers);
--- End diff --
I thought about that, as well. I can force a message to go out on the large message connection, but the `REQUEST_RESPONSE` will come back on the small message connection. Unless, of course, I send some empty byte array that exceeds the `OutboundMessagingPool#LARGE_MESSAGE_THRESHOLD`, [which is currently 64k](https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/net/async/OutboundMessagingPool.java#L47). Admittedly, I'm reticent to do that. I could, however, create variant of the Ping/Pong messages (or modify those) to switch between either large or small message connection.
I guess the concern i had was that many apps might not need the large message connection, and thus it becomes unused, but consumed, resources. Every instance will need the gossip and small message connections, but not every use case calls for the large connections. wdyt?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra pull request #191: 13993
Posted by aweisberg <gi...@git.apache.org>.
Github user aweisberg commented on a diff in the pull request:
https://github.com/apache/cassandra/pull/191#discussion_r169697893
--- Diff: src/java/org/apache/cassandra/net/MessageOut.java ---
@@ -96,6 +97,11 @@
//the second object is the POJO to serialize
public final List<Object> parameters;
+ /**
+ * Allows sender to explicitly state which connection type the message should be sent on.
+ */
+ public final ConnectionType connectionType;
--- End diff --
A part of me wants to say don't make MessageOut bigger, but it's probably a drop in the bucket in terms of allocation rate for processing network messages.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra pull request #191: 13993
Posted by aweisberg <gi...@git.apache.org>.
Github user aweisberg commented on a diff in the pull request:
https://github.com/apache/cassandra/pull/191#discussion_r169707744
--- Diff: src/java/org/apache/cassandra/net/StartupClusterConnectivityChecker.java ---
@@ -0,0 +1,171 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.net;
+
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.function.Predicate;
+import java.util.stream.Collectors;
+
+import com.google.common.util.concurrent.Uninterruptibles;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.cassandra.locator.InetAddressAndPort;
+import org.apache.cassandra.net.async.OutboundConnectionIdentifier;
+import org.apache.cassandra.utils.FBUtilities;
+
+import static org.apache.cassandra.net.MessagingService.Verb.PING;
+
+public class StartupClusterConnectivityChecker
+{
+ private static final Logger logger = LoggerFactory.getLogger(StartupClusterConnectivityChecker.class);
+
+ enum State { CONTINUE, FINISH_SUCCESS, FINISH_TIMEOUT }
+
+ private final int targetPercent;
+ private final int timeoutSecs;
+ private final Predicate<InetAddressAndPort> gossipStatus;
+
+ public StartupClusterConnectivityChecker(int targetPercent, int timeoutSecs, Predicate<InetAddressAndPort> gossipStatus)
+ {
+ if (targetPercent < 0)
+ {
+ targetPercent = 0;
+ }
+ else if (targetPercent > 100)
+ {
+ targetPercent = 100;
+ }
+ this.targetPercent = targetPercent;
+
+ if (timeoutSecs < 0)
+ {
+ timeoutSecs = 1;
+ }
+ else if (timeoutSecs > 100)
+ {
+ logger.warn("setting the block-for-peers timeout (in seconds) to {} might be a bit excessive, but using it nonetheless", timeoutSecs);
+ }
+ this.timeoutSecs = timeoutSecs;
+
+ this.gossipStatus = gossipStatus;
+ }
+
+ public void execute(Set<InetAddressAndPort> peers)
+ {
+ if (peers == null || targetPercent == 0)
+ return;
+
+ // remove current node from the set
+ peers = peers.stream()
+ .filter(peer -> !peer.equals(FBUtilities.getBroadcastAddressAndPort()))
+ .collect(Collectors.toSet());
+
+ // don't block if there's no other nodes in the cluster (or we don't know about them)
+ if (peers.size() <= 0)
+ return;
+
+ logger.info("choosing to block until {}% of peers are marked alive and connections are established; max time to wait = {} seconds",
+ targetPercent, timeoutSecs);
+
+ // first, send out a ping message to open up the non-gossip connections
+ final AtomicInteger connectedCount = sendPingMessages(peers);
+
+ final long startNanos = System.nanoTime();
+ final long expirationNanos = startNanos + TimeUnit.SECONDS.toNanos(timeoutSecs);
+ int completedRounds = 0;
+ while (checkStatus(peers, connectedCount, startNanos, expirationNanos < System.nanoTime(), completedRounds) == State.CONTINUE)
+ {
+ completedRounds++;
+ Uninterruptibles.sleepUninterruptibly(1, TimeUnit.SECONDS);
--- End diff --
I think we want to check the condition pretty aggressively so that startup in test harnesses is as fast as possible since we do it a lot. Like check every millisecond.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra pull request #191: 13993
Posted by aweisberg <gi...@git.apache.org>.
Github user aweisberg commented on a diff in the pull request:
https://github.com/apache/cassandra/pull/191#discussion_r167016420
--- Diff: src/java/org/apache/cassandra/net/MessagingService.java ---
@@ -254,6 +258,8 @@ public long getTimeout()
return DatabaseDescriptor.getRangeRpcTimeout();
}
},
+ PING(),
--- End diff --
Should this go this early in the enum before USUED_* and company? Does it make sense to ever insert anything into the middle of the enum?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra pull request #191: 13993
Posted by josh-mckenzie <gi...@git.apache.org>.
Github user josh-mckenzie commented on a diff in the pull request:
https://github.com/apache/cassandra/pull/191#discussion_r167033937
--- Diff: src/java/org/apache/cassandra/net/MessagingService.java ---
@@ -254,6 +258,8 @@ public long getTimeout()
return DatabaseDescriptor.getRangeRpcTimeout();
}
},
+ PING(),
--- End diff --
Left a similar comment on ticket, but this should be added at the end:
***
// remember to add new verbs at the end, since we serialize by ordinal
UNUSED_1,
UNUSED_2,
UNUSED_3,
UNUSED_4,
UNUSED_5,
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra pull request #191: 13993
Posted by jasobrown <gi...@git.apache.org>.
Github user jasobrown commented on a diff in the pull request:
https://github.com/apache/cassandra/pull/191#discussion_r168186702
--- Diff: src/java/org/apache/cassandra/net/MessagingService.java ---
@@ -254,6 +258,8 @@ public long getTimeout()
return DatabaseDescriptor.getRangeRpcTimeout();
}
},
+ PING(),
--- End diff --
See discussion on ticket, but I'll clean up the enum and comments
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra pull request #191: 13993
Posted by aweisberg <gi...@git.apache.org>.
Github user aweisberg commented on a diff in the pull request:
https://github.com/apache/cassandra/pull/191#discussion_r168320433
--- Diff: src/java/org/apache/cassandra/net/MessagingService.java ---
@@ -1664,4 +1676,113 @@ public static boolean isEncryptedConnection(InetAddressAndPort address)
}
return true;
}
+
+ public void blockForPeers()
+ {
+ // TODO make these yaml props?
+ int alivePercent = Integer.getInteger(Config.PROPERTY_PREFIX + "blockForPeers.percent", 70);
+ if (alivePercent < 0)
--- End diff --
My issue isn't with having a bound it's with enforcing it silently. Some of the time you can pick a value for them, but not all of the time.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra pull request #191: 13993
Posted by jasobrown <gi...@git.apache.org>.
Github user jasobrown commented on a diff in the pull request:
https://github.com/apache/cassandra/pull/191#discussion_r168316916
--- Diff: src/java/org/apache/cassandra/net/MessagingService.java ---
@@ -1664,4 +1676,113 @@ public static boolean isEncryptedConnection(InetAddressAndPort address)
}
return true;
}
+
+ public void blockForPeers()
+ {
+ // TODO make these yaml props?
+ int alivePercent = Integer.getInteger(Config.PROPERTY_PREFIX + "blockForPeers.percent", 70);
+ if (alivePercent < 0)
+ alivePercent = 0;
+ else if (alivePercent > 100)
+ alivePercent = 100;
+
+ int aliveTimeoutSecs = Integer.getInteger(Config.PROPERTY_PREFIX + "blockForPeers.timeout_in_secs", 10);
+ if (aliveTimeoutSecs < 0)
+ aliveTimeoutSecs = 1;
+ else if (aliveTimeoutSecs > 100)
+ aliveTimeoutSecs = 100;
+
+ if (alivePercent > 0)
+ blockForPeers(alivePercent, aliveTimeoutSecs);
+ }
+
+ private void blockForPeers(int targetAlivePercent, int aliveTimeoutSecs)
+ {
+ // grab a snapshot of the current cluster from Gossiper. this is completely prone to race conditions, but it's
+ // good enough for the purposes of blocking until some certain percentage of nodes are considered 'alive'/connected.
+ Set<Map.Entry<InetAddressAndPort, EndpointState>> peers = new HashSet<>(Gossiper.instance.getEndpointStates());
+
+ // remove current node from the set
+ peers = peers.stream()
+ .filter(entry -> !entry.getKey().equals(FBUtilities.getBroadcastAddressAndPort()))
+ .collect(Collectors.toSet());
+
+ final int totalSize = peers.size();
+
+ // don't block if there's no other nodes in the cluster (or we don't know about them)
+ if (totalSize <= 1)
+ return;
+
+ logger.info("choosing to block until {}% of peers are marked alive; max time to wait = {} seconds", targetAlivePercent, aliveTimeoutSecs);
+
+ // first, send out a ping message to open up the non-gossip connections
+ AtomicInteger connectedCount = sendPingMessages(peers);
--- End diff --
>> Do the connections consume resources if there are no queued messages?
Reviewing `OutboundMessageConnection`, no, they do not consume much in terms of resources. I'm fine either way creating the large message connections eagerly or lazily, so I'll add that in.
>> do we even need to send a message or is just instructing the transport system to open them enough?
Because connections are unidirectional, and as we'll want each "pair" of outbound/inbound connections for each connection type (gossip, small message, large message) to be established, the only way to do that is to send a 'real' message that the peer can respond to. That being said, perhaps I can work in something that lets a Ping/Pong message "select" which type of transport it wants to be sent on.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra pull request #191: 13993
Posted by aweisberg <gi...@git.apache.org>.
Github user aweisberg commented on a diff in the pull request:
https://github.com/apache/cassandra/pull/191#discussion_r167017281
--- Diff: src/java/org/apache/cassandra/net/MessagingService.java ---
@@ -1664,4 +1676,113 @@ public static boolean isEncryptedConnection(InetAddressAndPort address)
}
return true;
}
+
+ public void blockForPeers()
+ {
+ // TODO make these yaml props?
--- End diff --
So, should these be yaml props?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra pull request #191: 13993
Posted by aweisberg <gi...@git.apache.org>.
Github user aweisberg commented on a diff in the pull request:
https://github.com/apache/cassandra/pull/191#discussion_r168305034
--- Diff: src/java/org/apache/cassandra/net/MessagingService.java ---
@@ -1664,4 +1676,113 @@ public static boolean isEncryptedConnection(InetAddressAndPort address)
}
return true;
}
+
+ public void blockForPeers()
+ {
+ // TODO make these yaml props?
--- End diff --
This is default on so no I don't think we need to add it to the YAML just clean up the TODO.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra pull request #191: 13993
Posted by jasobrown <gi...@git.apache.org>.
Github user jasobrown commented on a diff in the pull request:
https://github.com/apache/cassandra/pull/191#discussion_r168324237
--- Diff: src/java/org/apache/cassandra/net/MessagingService.java ---
@@ -1664,4 +1676,113 @@ public static boolean isEncryptedConnection(InetAddressAndPort address)
}
return true;
}
+
+ public void blockForPeers()
+ {
+ // TODO make these yaml props?
--- End diff --
ok, sgtm. I will add the params to `Config`, but not add to the yaml.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra pull request #191: 13993
Posted by aweisberg <gi...@git.apache.org>.
Github user aweisberg commented on a diff in the pull request:
https://github.com/apache/cassandra/pull/191#discussion_r167020713
--- Diff: src/java/org/apache/cassandra/net/MessagingService.java ---
@@ -1664,4 +1676,113 @@ public static boolean isEncryptedConnection(InetAddressAndPort address)
}
return true;
}
+
+ public void blockForPeers()
+ {
+ // TODO make these yaml props?
+ int alivePercent = Integer.getInteger(Config.PROPERTY_PREFIX + "blockForPeers.percent", 70);
+ if (alivePercent < 0)
+ alivePercent = 0;
+ else if (alivePercent > 100)
+ alivePercent = 100;
+
+ int aliveTimeoutSecs = Integer.getInteger(Config.PROPERTY_PREFIX + "blockForPeers.timeout_in_secs", 10);
+ if (aliveTimeoutSecs < 0)
+ aliveTimeoutSecs = 1;
+ else if (aliveTimeoutSecs > 100)
+ aliveTimeoutSecs = 100;
+
+ if (alivePercent > 0)
+ blockForPeers(alivePercent, aliveTimeoutSecs);
+ }
+
+ private void blockForPeers(int targetAlivePercent, int aliveTimeoutSecs)
+ {
+ // grab a snapshot of the current cluster from Gossiper. this is completely prone to race conditions, but it's
+ // good enough for the purposes of blocking until some certain percentage of nodes are considered 'alive'/connected.
+ Set<Map.Entry<InetAddressAndPort, EndpointState>> peers = new HashSet<>(Gossiper.instance.getEndpointStates());
+
+ // remove current node from the set
+ peers = peers.stream()
+ .filter(entry -> !entry.getKey().equals(FBUtilities.getBroadcastAddressAndPort()))
+ .collect(Collectors.toSet());
+
+ final int totalSize = peers.size();
+
+ // don't block if there's no other nodes in the cluster (or we don't know about them)
+ if (totalSize <= 1)
+ return;
+
+ logger.info("choosing to block until {}% of peers are marked alive; max time to wait = {} seconds", targetAlivePercent, aliveTimeoutSecs);
+
+ // first, send out a ping message to open up the non-gossip connections
+ AtomicInteger connectedCount = sendPingMessages(peers);
--- End diff --
Should we also set up the large message connections? We can set up connections in parallel now with Netty right (sweet!). I think we should up all of them. Gossip, large, small.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra pull request #191: 13993
Posted by aweisberg <gi...@git.apache.org>.
Github user aweisberg commented on a diff in the pull request:
https://github.com/apache/cassandra/pull/191#discussion_r168321619
--- Diff: src/java/org/apache/cassandra/net/MessagingService.java ---
@@ -1664,4 +1676,113 @@ public static boolean isEncryptedConnection(InetAddressAndPort address)
}
return true;
}
+
+ public void blockForPeers()
+ {
+ // TODO make these yaml props?
--- End diff --
If we are going that route it seems like well then it should just be in the YAML. I would say jvm.options should be for things we don't control because they aren't directly part of Cassandra.
Another option is as a YAML option but undocumented if you want to not clutter the YAML, but then operators get annoyed and confused and ask why it's not a yaml option and not documented.
I never really know how to balance the tension of not putting out thousands of config options into a file with not having undocumented but important knobs. I think this one doesn't need to be in the documented category yet because setting it is really as an escape hatch not something people should have to worry about. This is just some feature that should silently work and people don't have to think about. Then later if we find out we are wrong we can add the option and document. This stems the tide of config options people need to think about which has value.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra pull request #191: 13993
Posted by jasobrown <gi...@git.apache.org>.
Github user jasobrown commented on a diff in the pull request:
https://github.com/apache/cassandra/pull/191#discussion_r168186466
--- Diff: src/java/org/apache/cassandra/net/MessagingService.java ---
@@ -1664,4 +1676,113 @@ public static boolean isEncryptedConnection(InetAddressAndPort address)
}
return true;
}
+
+ public void blockForPeers()
+ {
+ // TODO make these yaml props?
--- End diff --
well, I wanted to get more input before shovelling more stuff into the yaml :) If we want users to actually use this in a 'standard' way, then they probably should be in the yaml
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra issue #191: 13993
Posted by jolynch <gi...@git.apache.org>.
Github user jolynch commented on the issue:
https://github.com/apache/cassandra/pull/191
@aweisberg since this has been merged should we close this one out?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra pull request #191: 13993
Posted by aweisberg <gi...@git.apache.org>.
Github user aweisberg closed the pull request at:
https://github.com/apache/cassandra/pull/191
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra pull request #191: 13993
Posted by jasobrown <gi...@git.apache.org>.
Github user jasobrown commented on a diff in the pull request:
https://github.com/apache/cassandra/pull/191#discussion_r168186993
--- Diff: src/java/org/apache/cassandra/net/MessagingService.java ---
@@ -1664,4 +1676,113 @@ public static boolean isEncryptedConnection(InetAddressAndPort address)
}
return true;
}
+
+ public void blockForPeers()
+ {
+ // TODO make these yaml props?
+ int alivePercent = Integer.getInteger(Config.PROPERTY_PREFIX + "blockForPeers.percent", 70);
+ if (alivePercent < 0)
+ alivePercent = 0;
+ else if (alivePercent > 100)
+ alivePercent = 100;
+
+ int aliveTimeoutSecs = Integer.getInteger(Config.PROPERTY_PREFIX + "blockForPeers.timeout_in_secs", 10);
+ if (aliveTimeoutSecs < 0)
+ aliveTimeoutSecs = 1;
+ else if (aliveTimeoutSecs > 100)
+ aliveTimeoutSecs = 100;
+
+ if (alivePercent > 0)
+ blockForPeers(alivePercent, aliveTimeoutSecs);
+ }
+
+ private void blockForPeers(int targetAlivePercent, int aliveTimeoutSecs)
+ {
+ // grab a snapshot of the current cluster from Gossiper. this is completely prone to race conditions, but it's
+ // good enough for the purposes of blocking until some certain percentage of nodes are considered 'alive'/connected.
+ Set<Map.Entry<InetAddressAndPort, EndpointState>> peers = new HashSet<>(Gossiper.instance.getEndpointStates());
--- End diff --
Yes, this is only called after we've joined the ring, done bootstrapping, started gossip, and so on.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra pull request #191: 13993
Posted by aweisberg <gi...@git.apache.org>.
Github user aweisberg commented on a diff in the pull request:
https://github.com/apache/cassandra/pull/191#discussion_r167017641
--- Diff: src/java/org/apache/cassandra/net/MessagingService.java ---
@@ -1664,4 +1676,113 @@ public static boolean isEncryptedConnection(InetAddressAndPort address)
}
return true;
}
+
+ public void blockForPeers()
+ {
+ // TODO make these yaml props?
+ int alivePercent = Integer.getInteger(Config.PROPERTY_PREFIX + "blockForPeers.percent", 70);
+ if (alivePercent < 0)
--- End diff --
As a matter of practice silently clamping values always seems wrong to me. They asked for something and you aren't going to give it to them therefore someone is in error and it shouldn't be silent so we can come back and supply the value we intended. I suppose it's how I interpret the principle of least surprise.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra pull request #191: 13993
Posted by aweisberg <gi...@git.apache.org>.
Github user aweisberg commented on a diff in the pull request:
https://github.com/apache/cassandra/pull/191#discussion_r168320211
--- Diff: src/java/org/apache/cassandra/net/MessagingService.java ---
@@ -1664,4 +1676,113 @@ public static boolean isEncryptedConnection(InetAddressAndPort address)
}
return true;
}
+
+ public void blockForPeers()
+ {
+ // TODO make these yaml props?
+ int alivePercent = Integer.getInteger(Config.PROPERTY_PREFIX + "blockForPeers.percent", 70);
+ if (alivePercent < 0)
--- End diff --
I don't like silently clamping any of them. If you feel strongly about upper bounding aliveTimeoutSecs it's the closest to being OK because the DB still comes up and is reliable at up to 100 seconds and most of the time it won't wait that long since it will get to 70%. If someone sets a value higher than that we "know" it's not useful so clamping it doesn't hurt them.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra pull request #191: 13993
Posted by aweisberg <gi...@git.apache.org>.
Github user aweisberg commented on a diff in the pull request:
https://github.com/apache/cassandra/pull/191#discussion_r168302781
--- Diff: src/java/org/apache/cassandra/net/MessagingService.java ---
@@ -1664,4 +1676,113 @@ public static boolean isEncryptedConnection(InetAddressAndPort address)
}
return true;
}
+
+ public void blockForPeers()
+ {
+ // TODO make these yaml props?
+ int alivePercent = Integer.getInteger(Config.PROPERTY_PREFIX + "blockForPeers.percent", 70);
+ if (alivePercent < 0)
--- End diff --
Well I think it really sucks when you think you are getting the benefit from some config option and your not and you don't know or don't know why. Out of scope arguments aren't really applicable because this is the ticket adding the feature driven by these config options.
I am not sure what you mean by order of magnitude. The zero clamp means someone tried to enable this feature, but didn't and so their stuff is silently unreliable.
The 100 clamp is they meant to set a value < 100 but had an extra digit and didn't and it's silently being slower to start. I doubt they are going to care as much about that in production use cases.
I don't think you'll find many operators that complain about being informed via a clear error message their config is not sane. This is also an option that people have to request explicitly in order to get an invalid value to clamp in the first place so I really do think they want to know.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra pull request #191: 13993
Posted by jasobrown <gi...@git.apache.org>.
Github user jasobrown commented on a diff in the pull request:
https://github.com/apache/cassandra/pull/191#discussion_r168319052
--- Diff: src/java/org/apache/cassandra/net/MessagingService.java ---
@@ -1664,4 +1676,113 @@ public static boolean isEncryptedConnection(InetAddressAndPort address)
}
return true;
}
+
+ public void blockForPeers()
+ {
+ // TODO make these yaml props?
--- End diff --
If we choose to not put these params (`alivePercent` and `aliveTimeoutSecs`) into the yaml, should they be documented and added to `conf/jvm.options`?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra pull request #191: 13993
Posted by aweisberg <gi...@git.apache.org>.
Github user aweisberg commented on a diff in the pull request:
https://github.com/apache/cassandra/pull/191#discussion_r168304560
--- Diff: src/java/org/apache/cassandra/net/MessagingService.java ---
@@ -1664,4 +1676,113 @@ public static boolean isEncryptedConnection(InetAddressAndPort address)
}
return true;
}
+
+ public void blockForPeers()
+ {
+ // TODO make these yaml props?
+ int alivePercent = Integer.getInteger(Config.PROPERTY_PREFIX + "blockForPeers.percent", 70);
+ if (alivePercent < 0)
+ alivePercent = 0;
+ else if (alivePercent > 100)
+ alivePercent = 100;
+
+ int aliveTimeoutSecs = Integer.getInteger(Config.PROPERTY_PREFIX + "blockForPeers.timeout_in_secs", 10);
+ if (aliveTimeoutSecs < 0)
+ aliveTimeoutSecs = 1;
+ else if (aliveTimeoutSecs > 100)
+ aliveTimeoutSecs = 100;
+
+ if (alivePercent > 0)
+ blockForPeers(alivePercent, aliveTimeoutSecs);
+ }
+
+ private void blockForPeers(int targetAlivePercent, int aliveTimeoutSecs)
+ {
+ // grab a snapshot of the current cluster from Gossiper. this is completely prone to race conditions, but it's
+ // good enough for the purposes of blocking until some certain percentage of nodes are considered 'alive'/connected.
+ Set<Map.Entry<InetAddressAndPort, EndpointState>> peers = new HashSet<>(Gossiper.instance.getEndpointStates());
+
+ // remove current node from the set
+ peers = peers.stream()
+ .filter(entry -> !entry.getKey().equals(FBUtilities.getBroadcastAddressAndPort()))
+ .collect(Collectors.toSet());
+
+ final int totalSize = peers.size();
+
+ // don't block if there's no other nodes in the cluster (or we don't know about them)
+ if (totalSize <= 1)
+ return;
+
+ logger.info("choosing to block until {}% of peers are marked alive; max time to wait = {} seconds", targetAlivePercent, aliveTimeoutSecs);
+
+ // first, send out a ping message to open up the non-gossip connections
+ AtomicInteger connectedCount = sendPingMessages(peers);
--- End diff --
Do the connections consume resources if there are no queued messages? Are there any buffers allocated? The resources consumed should be minimal and even then if we can theoretically provision these resources then we will get more predictable performance and behavior if we provision them all up front instead of lazily where we find out oh we didn't have enough at some arbitrary later time.
Maybe we are trying to hack this process too much by having the transport system unaware of what we are attempting and if we made it possible to say "hey connect on all things and send this small message on each" it would still be simple. In fact do we even need to send a message or is just instructing the transport system to open them enough?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra pull request #191: 13993
Posted by jasobrown <gi...@git.apache.org>.
Github user jasobrown commented on a diff in the pull request:
https://github.com/apache/cassandra/pull/191#discussion_r168318195
--- Diff: src/java/org/apache/cassandra/net/MessagingService.java ---
@@ -1664,4 +1676,113 @@ public static boolean isEncryptedConnection(InetAddressAndPort address)
}
return true;
}
+
+ public void blockForPeers()
+ {
+ // TODO make these yaml props?
+ int alivePercent = Integer.getInteger(Config.PROPERTY_PREFIX + "blockForPeers.percent", 70);
+ if (alivePercent < 0)
--- End diff --
Just to clarify: you do not like the bounding of `aliveTimeoutSecs`, correct? `alivePercent` is just bounding the percentage.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra pull request #191: 13993
Posted by jasobrown <gi...@git.apache.org>.
Github user jasobrown commented on a diff in the pull request:
https://github.com/apache/cassandra/pull/191#discussion_r168186136
--- Diff: src/java/org/apache/cassandra/net/MessagingService.java ---
@@ -1664,4 +1676,113 @@ public static boolean isEncryptedConnection(InetAddressAndPort address)
}
return true;
}
+
+ public void blockForPeers()
+ {
+ // TODO make these yaml props?
+ int alivePercent = Integer.getInteger(Config.PROPERTY_PREFIX + "blockForPeers.percent", 70);
+ if (alivePercent < 0)
--- End diff --
This is fair, but I felt that giving *some* upper bound didn't seem unreasonable. Should we really delay starting the process by an extra order of magnitude just because an operator added an extra zero to the configuration? I understand your point about least surprise, but that might be beyond the spirit of this ticket. wdyt?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] cassandra pull request #191: 13993
Posted by jasobrown <gi...@git.apache.org>.
Github user jasobrown commented on a diff in the pull request:
https://github.com/apache/cassandra/pull/191#discussion_r168326107
--- Diff: src/java/org/apache/cassandra/net/MessagingService.java ---
@@ -1664,4 +1676,113 @@ public static boolean isEncryptedConnection(InetAddressAndPort address)
}
return true;
}
+
+ public void blockForPeers()
+ {
+ // TODO make these yaml props?
+ int alivePercent = Integer.getInteger(Config.PROPERTY_PREFIX + "blockForPeers.percent", 70);
+ if (alivePercent < 0)
--- End diff --
hmm, starting thinking about this, and, yeah, I guess we can leave the upper bound unenforced and instead just log that the operator can choose to ignore
---
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org