Does anyone know why LOADING was ever considered a reason to prevent failover? As far as I can tell, PONG, LOADING and MASTERDOWN were all determined to mean the instance is available in the same commit and I cannot see any obvious reason for including LOADING in the list.

If LOADING means "not available", and you shut down the master and all of it's replicas gracefully at a time when you have a significant amount of data, then start everything very quickly, you would see failovers during the restart. Could that have been the reason for adding this line? https://github.com/redis/redis/blob/e96bb30ebf532496e7f8b5fca208b6e86ab3ae4f/src/sentinel.c#L2818

@daniel-house If you restart both masters and replicas, you'd probably get -LOADING from the master and -MASTERDOWN from the replica. I assume the rationale behind avoiding a failover is that in this case you may end up switching back and forth, whereas simply waiting should (eventually) yield a reasonable result.

On the other hand, if the replica is up and running and it's only the master that's -LOADING - failover does make sense.

@daniel-house If you restart both masters and replicas, you'd probably get -LOADING from the master and -MASTERDOWN from the replica. I assume the rationale behind avoiding a failover is that in this case you may end up switching back and forth, whereas simply waiting should (eventually) yield a reasonable result.

On the other hand, if the replica is up and running and it's only the master that's -LOADING - failover does make sense.

I think for this case, if you restart both masters and replicas in very short time, we could assume it takes only less than 0.1 or 0.5 seconds, then you could get -LOADING from the master and replica, because replica need to first load its own RDB file or AOF file and then try to connect to master. If at that time master is still unavailable, replica will declare "MASTERDOWN"

@hwware Code looks good, a few nitpicks.

Basically we identify a rebooting master as a new distinct kind of failure mode, for which we apply reboot-down-after-milliseconds configuration. I have a couple of conceptual questions here:

• Is it really necessary to do that, vs. just treating -LOADING as any other error and failing over?
• If we do, won't it make sense to do this configurable rather than hard coded?

I will be happy to also get the perspective of recent reporters of this issue, @daniel-house and others.

@yossigo For this issue, i find it almost exists in all previous versions, if do you think merge these changed codes to those versions, thanks.

@yossigo Let me describe the whole process for this solution. Thanks.
The issue happens when master node suddenly reboot in a very shot time. At this time, Because the reboot time is too short, Sentinel can not execute failover. The master node is loading data from RDB, thus client can not read and write data from this master node.
My solution is: because Sentinel could receive a "reboot" signal, then I add a new signal variable "SRI_MASTER_REBOOT" to indicate master node are rebooting and sentinel begins timing. Once master can not finish the reboot process in 10 seconds (I set this value), then Sentinel will failover process
one concern for this solution is: this solution is how to proceed if master and some replica reboot together? I think my solution works as well, Because if more than half replicas still works, failover could happens. And if most replicas and master do not work, thus failover will not happens. The Sentinel will period send ping and info to master and replica, If master finishes the loading process, then Sentinel will remove the "SRI_MASTER_REBOOT" signal. If master is still loading the data, and most replicas works, then Sentinel could be ready to another failover process.
Above is all summary for the Sentinel issue, please take sometime to review, welcome to any discussion, thanks

@hwware I had another discussion about this with @oranagra.

One way to look at this fix is that it improves availability over durability of data because, at least in some scenarios, the master will have the most up to date dataset and we just need to wait for it to load.

The concern is that different deployments may have conflicting needs: some may want to push availability further and not even wait 10*ping time before failing over a master that's loading, and others may want to wait longer (or forever).

We could achieve that if master_reboot_down_after_period was configurable and support a value of zero to maintain the existing behavior. WDUT?

@hwware I had another discussion about this with @oranagra.

One way to look at this fix is that it improves availability over durability of data because, at least in some scenarios, the master will have the most up to date dataset and we just need to wait for it to load.

The concern is that different deployments may have conflicting needs: some may want to push availability further and not even wait 10*ping time before failing over a master that's loading, and others may want to wait longer (or forever).

We could achieve that if master_reboot_down_after_period was configurable and support a value of zero to maintain the existing behavior. WDUT?

I totally agree what you suggest, making master_reboot_down_after_period configurable gives user more flexible choices. I could update codes in this PR, Thanks

@hwware I had another discussion about this with @oranagra.

One way to look at this fix is that it improves availability over durability of data because, at least in some scenarios, the master will have the most up to date dataset and we just need to wait for it to load.

The concern is that different deployments may have conflicting needs: some may want to push availability further and not even wait 10*ping time before failing over a master that's loading, and others may want to wait longer (or forever).

We could achieve that if master_reboot_down_after_period was configurable and support a value of zero to maintain the existing behavior. WDUT?

@hwware Hi Yossi, follow your suggestion, I change "`master_reboot_down_after_period" as configurable, and client could
set it by command: sentinel set, please take a look the updated codes, Thanks a lot.

@hwware I've pushed a commit with minor cleanups, I think the only thing missing to merge this is a test.

@yossigo Hi Yossi, I just add the test for this PR, please take a look, thanks