Fix rescheduling on stopped tasks after migrating back to origin

[tkountis]

Starting a task on a single node, and then adding one more member, causes some of the tasks to migrate to partitions owned by the latter member.
During this process, the container stops currently running task, leaving it in a cancelled state. If this state, doesn't get disposed (eg. cluster never gets bigger), when the latter member goes down, the tasks will migrate back to the first one. However, since their state there is marked as cancelled, they never get re-scheduled.

The fix, introduces a stop method, which still cancels the task, but doesn't interact with the state.
Fix #10603

Also, minor cleanup and namings.

[Donnerbart]

ScheduledTaskStatisticsImpl{runs= without a space matches the majority of our toString() methods.

[Donnerbart]

How can you be sure the task was migrated to the second member in between? assertTrueEventually(new AllTasksRunning(scheduler)); should also pass if there was no migration at all, shouldn't it?

[tkountis]

Well, I am not sure, I can add an extra check to see if there are tasks in the second member, specifically, but I tried to keep the taskCount rather high so that at least some will migrate. But yes, its not deterministic. I will modify to make it so.

[Donnerbart]

I thought there would be more logic inside the getScheduledExecutor() method, but since it isn't I would just use first.getScheduledExecutorService("scheduler");

[tkountis]

its used in the Client tests and its slightly different there. eg. ClientScheduledExecutorServiceBasicTest

[Donnerbart]

I don't want to remove the whole method. But this test looks like it doesn't need it, since it requires to construct an array (which looks more complex than just retrieving the scheduler).

[devOpsHazelcast]

Test PASSed.

[devOpsHazelcast]

Test PASSed.

[ruslan-belinskyy]

Guys,
i'm currently getting:

java.lang.NullPointerException
	at com.hazelcast.scheduledexecutor.impl.ScheduledTaskDescriptor.stopForMigration(ScheduledTaskDescriptor.java:146) ~[hazelcast-all-3.8.3.jar:3.8.3]

When trying to schedule the Task:

IScheduledExecutorService service = hazelcastInstance.getScheduledExecutorService(executorName);
service.scheduleAtFixedRate(new HzcastTimerExchangeSender(this.getEndpoint().getEndpointUri(), executorName), endpoint.getDelay(), endpoint.getPeriod(), TimeUnit.MILLISECONDS);

I have looked to the fix you have here and it doesn't look safe and could cause issue i have.

You had:

try {
   descriptor.cancel(true);
   descriptor.setScheduledFuture(null);
   descriptor.setTaskOwner(false);
} catch (Exception ex) {
   throw rethrow(ex);
}

And now:

try {
    descriptor.stopForMigration();
} catch (Exception ex) {
   throw rethrow(ex);
}

Where descriptor.stopForMigration() has:

void stopForMigration() {
 +        // Result is not set, allowing task to get re-scheduled, if/when needed.
 +        this.isTaskOwner = false;
 +        this.future.cancel(true); //Nullpointer here
 +        this.future = null;
 +    }

And in old code :

descriptor.cancel(true);

Which has:

boolean cancel(boolean mayInterrupt)
            throws ExecutionException, InterruptedException {
        if (!resultRef.compareAndSet(null, new ScheduledTaskResult(true)) || future == null) {
            return false;
        }

        return future.cancel(mayInterrupt);
    }

This look way safer.

Nullpointer i have on 2 machines which have code you can see above.

[mmedenjak]

@Batter2014 can you please submit a new issue for this?

[ruslan-belinskyy]

Yep, just did that: #11047