patka / cassandra-migration

Schema migration library for Cassandra
MIT License
154 stars 47 forks source link

Cassandra RetryPolicy can't be used #48

Open osctro opened 4 years ago

osctro commented 4 years ago

It would be great if the migration script commands would be executed in a way that the Cassandra RetryPolicy or any other retry method could be used.

patka commented 4 years ago

I am not sure I get your request. About which driver version are we talking. As far as I understand it the RetryPolicy is set on the Cluster object (for version 3) and in a configuration file (for version 4). In both cases you are able to configure this. What else is needed? I never used this feature.

osctro commented 4 years ago

At least on version 3 the RetryPolicy is a kind of callback that can be registered at the Cluster level allowing you to retry failed commands. It receives and Exception and returns an value that let's the driver know what you want to do: cancel the operation and raise the exception or ignore the exception and retry the operation after a certain delay, number of attempts, etc. I use a cloud hosted Cassandra instance (PaaS) that has a limited operations rate. When the rate limit is reached I receive an OverloadedException from the provider that notifies my that I should try again some ms later. This works for the rest of the application using a RetryPolicy but it doesn't for migration part as I have no control on the way or rate of execution of the commands in the migration scripts.

osctro commented 4 years ago

I have solved it by introducing a delay after each migration. I have had to create my own MigrationTask passing it a subclass of Database that introduces a delay whenever the execute(migration) method is called. It would be very useful to have that functionality of waiting a configurable time after executing each statement or make "executeStatement" customizable via subclassing or callback.

patka commented 4 years ago

I already had a solution in mind to allow the execution of the statement to be somewhat more configurable. Seems the need for that is rising. I see how I can integrate this in the current version. At the moment my preferred way of doing this would be by using a Strategy that does the execution and that can be replaced with another one that does things a little different.

osctro commented 4 years ago

Yes, that's exactly what I need: a way to replace/customize the "executeStatement" method. An example could be polling the cluster metadata to check the schema agreement instead of trusting on executionInfo to check it, At least this callback/policy should receive the session/cluster and the CQL as arguments.

patka commented 4 years ago

I just created a branch called issue_48 that contains a first version with a strategy to execute statements. Could you have a look and see if that solves your issue? There might be some changes before it is finalized so please don't use it in production code already. This is just to be on the same page for the idea ;-)

osctro commented 4 years ago

It's ok. It gives us much more flexibility when executing the migration statements. It allows us to implement it with a simple lambda so I would probably annotate the strategy interface with FunctionalInterface. This is enough for me and probably for anyone using the plain library without spring boot support but when using from spring boot we need a way to set the strategy instance into the Database one and the only way I found to do it is by replacing the MigrationTask bean and then duplicating part of your code (not too bad, but ugly solution):

` @Bean(initMethod = "migrate") public MigrationTask migrationTask(Cluster cluster, CassandraMigrationConfigurationProperties migrationProperties, CassandraCustomProperties cassandraCustomProperties) {

// Duplicated code
if (!migrationProperties.hasKeyspaceName()) {
  throw new IllegalStateException("Please specify ['cassandra.migration.keyspace-name'] in order to migrate your database");
}

// Non duplicated code
Database database = ...
database.setExecutionStrategy(myStrategy);

// Duplicated code again
ScannerRegistry registry = new ScannerRegistry();
registry.register(ScannerRegistry.JAR_SCHEME, new SpringBootLocationScanner());
MigrationRepository migrationRepository = new MigrationRepository(migrationProperties.getScriptLocation(), new IgnoreDuplicatesCollector(), registry);

return new MigrationTask(database.setConsistencyLevel(migrationProperties.getConsistencyLevel()),
    migrationRepository,
    migrationProperties.isWithConsensus());

}`

patka commented 4 years ago

Of course the strategy will become an option in Spring Boot. This was just to see if that solution will help you. Thanks for the highlight of the FunctionalInterface. That is actually a very good point.