ekaterinadimitrova2 commented on code in PR #2972:
URL: https://github.com/apache/cassandra/pull/2972#discussion_r1420744994
##########
src/java/org/apache/cassandra/schema/MigrationCoordinator.java:
##########
@@ -548,8 +551,16 @@ private Future<Void> scheduleSchemaPull(InetAddressAndPort
endpoint, VersionInfo
if (shouldPullImmediately(endpoint, info.version))
{
- logger.debug("Pulling {} immediately from {}", info, endpoint);
- submitToMigrationIfNotShutdown(task);
+ if (lastPullFailures.contains(endpoint))
+ {
+ logger.debug("Pulling {} immediately from {} with backoff
interval = {}", info, endpoint, PULL_BACKOFF_INTERVAL_MS);
+ ScheduledExecutors.nonPeriodicTasks.schedule(() ->
submitToMigrationIfNotShutdown(task), PULL_BACKOFF_INTERVAL_MS,
TimeUnit.MILLISECONDS);
Review Comment:
And if we happen to run into constant failures for specific endpoint, we
will just run into the same issue as we do today, right? I just hope people do
not abuse extending this timeout too much
##########
test/distributed/org/apache/cassandra/distributed/test/MigrationCoordinatorTest.java:
##########
@@ -116,7 +119,6 @@ public void explicitVersionIgnore() throws Throwable
IInstanceConfig config = cluster.newInstanceConfig();
config.set("auto_bootstrap", true);
IGNORED_SCHEMA_CHECK_VERSIONS.setString(initialVersion.toString()
+ ',' + oldVersion);
- System.setProperty("cassandra.consistent.rangemovement", "false");
Review Comment:
Why was this one removed?
##########
src/java/org/apache/cassandra/config/CassandraRelevantProperties.java:
##########
@@ -344,6 +346,11 @@ public enum CassandraRelevantProperties
*/
GOSSIP_SETTLE_POLL_SUCCESSES_REQUIRED("cassandra.gossip_settle_poll_success_required",
"3"),
+ CONSISTENT_RANGE_MOVEMENT("cassandra.consistent.rangemovement", "true"),
+
CONSISTENT_SIMULTANEOUS_MOVES_ALLOW("cassandra.consistent.simultaneousmoves.allow"),
+ REPLACE_ADDRESS("cassandra.replace_address"),
+ BROADCAST_INTERVAL_MS("cassandra.broadcast_interval_ms", "60000"),
Review Comment:
I believe all those are moved to this class already in 5.0+ and they will
appear in the VTs. I think we should not move them in older branches now. Also,
you would have had to change how they are set/get in more places. Result of a
quick grep:
```
grep -r "cassandra.consistent.rangemovement" *
NEWS.txt: you can set the following property
(-Dcassandra.consistent.rangemovement=false)
Binary file
build/test/classes/org/apache/cassandra/distributed/test/PartitionDenylistTest.class
matches
Binary file
build/classes/main/org/apache/cassandra/config/CassandraRelevantProperties.class
matches
Binary file
build/classes/main/org/apache/cassandra/service/StorageService.class matches
doc/modules/cassandra/pages/operating/topo_changes.adoc:`-Dcassandra.consistent.rangemovement=false`.
src/java/org/apache/cassandra/config/CassandraRelevantProperties.java:
CONSISTENT_RANGE_MOVEMENT("cassandra.consistent.rangemovement", "true"),
src/java/org/apache/cassandra/service/StorageService.java: public static
final boolean useStrictConsistency =
Boolean.parseBoolean(System.getProperty("cassandra.consistent.rangemovement",
"true"));
src/java/org/apache/cassandra/service/StorageService.java:
throw new UnsupportedOperationException("Other bootstrapping/leaving/moving
nodes detected, cannot bootstrap while cassandra.consistent.rangemovement is
true");
src/java/org/apache/cassandra/service/StorageService.java: throw
new UnsupportedOperationException(String.format("Other
bootstrapping/leaving/moving nodes detected, cannot bootstrap while
cassandra.consistent.rangemovement is true. Nodes detected, bootstrapping: %s;
leaving: %s; moving: %s;", bootstrapTokens, leavingTokens, movingTokens));
test/distributed/org/apache/cassandra/distributed/test/PartitionDenylistTest.java:
System.setProperty("cassandra.consistent.rangemovement", "false");
```
##########
test/unit/org/apache/cassandra/schema/MigrationCoordinatorTest.java:
##########
@@ -96,6 +100,8 @@ public class MigrationCoordinatorTest
throw new AssertionError(e);
}
+ CassandraRelevantProperties.SCHEMA_PULL_BACKOFF_INTERVAL_MS.setLong(0);
Review Comment:
So this is optimization not to use the default value. as it does not make
sense in these tests?
##########
test/distributed/org/apache/cassandra/distributed/impl/Instance.java:
##########
@@ -589,12 +592,20 @@ public void startup(ICluster cluster)
//
org.apache.cassandra.distributed.impl.AbstractCluster.startup sets the
exception handler for the thread
// so extract it to populate ExecutorFactory.Global
ExecutorFactory.Global.tryUnsafeSet(new
ExecutorFactory.Default(Thread.currentThread().getContextClassLoader(), null,
Thread.getDefaultUncaughtExceptionHandler()));
+
+ assert
!FBUtilities.getReleaseVersionString().equals(FBUtilities.UNKNOWN_RELEASE_VERSION)
: "Unknown version";
+ assert !FBUtilities.getReleaseVersionString().isEmpty() :
"Empty version";
+ assert FBUtilities.getReleaseVersionString().contains(".") :
"Invalid version: " + FBUtilities.getReleaseVersionString();
Review Comment:
Isn't this the case when it is `UNKNOWN_RELEASE_VERSION`?
##########
test/distributed/org/apache/cassandra/distributed/test/MigrationCoordinatorTest.java:
##########
@@ -30,22 +30,26 @@
import org.apache.cassandra.distributed.shared.NetworkTopology;
import org.apache.cassandra.schema.Schema;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.BROADCAST_INTERVAL_MS;
import static
org.apache.cassandra.config.CassandraRelevantProperties.IGNORED_SCHEMA_CHECK_ENDPOINTS;
import static
org.apache.cassandra.config.CassandraRelevantProperties.IGNORED_SCHEMA_CHECK_VERSIONS;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.REPLACE_ADDRESS;
+import static
org.apache.cassandra.config.CassandraRelevantProperties.RING_DELAY;
import static org.apache.cassandra.distributed.api.Feature.GOSSIP;
import static org.apache.cassandra.distributed.api.Feature.NETWORK;
public class MigrationCoordinatorTest extends TestBaseImpl
{
-
@Before
public void setUp()
{
- System.clearProperty("cassandra.replace_address");
- System.clearProperty("cassandra.consistent.rangemovement");
+ REPLACE_ADDRESS.clearValue();
Review Comment:
Isn't it clearer if we add `@After` for the cleaning?
##########
test/distributed/org/apache/cassandra/distributed/impl/Instance.java:
##########
@@ -589,12 +592,20 @@ public void startup(ICluster cluster)
//
org.apache.cassandra.distributed.impl.AbstractCluster.startup sets the
exception handler for the thread
// so extract it to populate ExecutorFactory.Global
ExecutorFactory.Global.tryUnsafeSet(new
ExecutorFactory.Default(Thread.currentThread().getContextClassLoader(), null,
Thread.getDefaultUncaughtExceptionHandler()));
+
+ assert
!FBUtilities.getReleaseVersionString().equals(FBUtilities.UNKNOWN_RELEASE_VERSION)
: "Unknown version";
+ assert !FBUtilities.getReleaseVersionString().isEmpty() :
"Empty version";
+ assert FBUtilities.getReleaseVersionString().contains(".") :
"Invalid version: " + FBUtilities.getReleaseVersionString();
+
if (config.has(GOSSIP))
{
// TODO: hacky
- System.setProperty("cassandra.ring_delay_ms", "15000");
- System.setProperty("cassandra.consistent.rangemovement",
"false");
-
System.setProperty("cassandra.consistent.simultaneousmoves.allow", "true");
+ if (!RING_DELAY.isPresent())
+ RING_DELAY.setLong(15000);
+ if (!CONSISTENT_RANGE_MOVEMENT.isPresent())
+ CONSISTENT_RANGE_MOVEMENT.setBoolean(false);
+ if (!CONSISTENT_SIMULTANEOUS_MOVES_ALLOW.isPresent())
+ CONSISTENT_SIMULTANEOUS_MOVES_ALLOW.setBoolean(true);
Review Comment:
This is applied after any test configuration is loaded (setUp). Isn't this a
bug we need to fix in previous branches? Or at least 4.0, which will still be
supported for some time? - I responded to my question - no, those properties
are not presented there. I also do not see other options being set on startup,
at least in 4.1 that can crash if any test `setUp` method is setting. Please
check that nothing has changed in the other newer branches when preparing them.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
