2.8 #1
Open
2.8 #1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a general fix (check that dirty delta is positive) but actually should have as the only effect fixing the SAVE propagation to AOF and slaves.
Recently we introduced the ability to load truncated AOFs, but unfortuantely the support was broken since the server, after loading the truncated AOF, continues appending to the file that is corrupted at the end. The problem is fixed only in the next AOF rewrite. This commit fixes the issue by truncating the AOF to the last valid opcode, and aborting if it is not possible to truncate the file correctly.
It is not clear if files open in append only mode will automatically fix their offset after a truncate(2) operation. This commit makes sure that we reposition the AOF file descriptor offset at the end of the file after a truncated AOF is loaded and trimmed to the last valid command.
Now there are tests to write more data after loading a truncated AOF, testing that the loaded data is correct, appending more, and testing again.
Closes #1537
Closes #1713
error != success; and 0 != number of bytes written Closes #1806
Closes #1871
Closes #1373
Closes #1386
Closes #1739
Some language in the comment was difficult to understand, so this commit: clarifies wording, removes unnecessary words, and relocates some dependent clauses closer to what they actually describe. I also tried to break up longer chains of thought (if X, then Y, and Q, and also F, so obviously M) into more manageable chunks for ease of understanding.
We have a check to rewrite the config properly when a failover is in progress, in order to add the current (already failed over) master as slave, and don't include in the slave list the promoted slave itself. However there was an issue, the variable with the right address was computed but never used when the code was modified, and no tests are available for this feature for two reasons: 1. The Sentinel unit test currently does not test Sentinel ability to persist its state at all. 2. It is a very hard to trigger state since it lasts for little time in the context of the testing framework. However this feature should be covered in the test in some way. The bug was found by @badboy using the clang static analyzer. Effects of the bug on safety of Sentinel === This bug results in severe issues in the following case: 1. A Sentinel is elected leader. 2. During the failover, it persists a wrong config with a known-slave entry listing the master address. 3. The Sentinel crashes and restarts, reading invalid configuration from disk. 4. It sees that the slave now does not obey the logical configuration (should replicate from the current master), so it sends a SLAVEOF command to the master (since the slave master is the same) creating a replication loop (attempt to replicate from itself) which Redis is currently unable to detect. 5. This means that the master is no longer available because of the bug. However the lack of availability should be only transient (at least in my tests, but other states could be possible where the problem is not recovered automatically) because: 6. Sentinels treat masters reporting to be slaves as failing. 7. A new failover is triggered, and a slave is promoted to master. Bug lifetime === The bug is there forever. Commit 16237d7 actually tried to fix the bug but in the wrong way (the computed variable was never used! My fault). So this bug is there basically since the start of Sentinel. Since the bug is hard to trigger, I remember little reports matching this condition, but I remember at least a few. Also in automated tests where instances were stopped and restarted multiple times automatically I remember hitting this issue, however I was not able to reproduce nor to determine with the information I had at the time what was causing the issue.
As Oran Agra suggested, in startBgsaveForReplication() when the BGSAVE attempt returns an error, we scan the list of slaves in order to remove them since there is no way to serve them currently. However we check for the replication state BGSAVE_START, which was modified by rdbSaveToSlaveSockets() before forking(). So when fork fails the state of slaves remain BGSAVE_END and no cleanup is performed. This commit fixes the problem by making rdbSaveToSlavesSockets() able to undo the state change on fork failure.
MOVE was not able to move the TTL: when a key was moved into a different database number, it became persistent like if PERSIST was used. In some incredible way (I guess almost nobody uses Redis MOVE) this bug remained unnoticed inside Redis internals for many years. Finally Andy Grunwald discovered it and opened an issue. This commit fixes the bug and adds a regression test. Close #2765.
getExpire() returns -1 when no expire exists. Related to #2765.
Related to #2765.
HINCRBY* tests later used the value "tmp" that was sometimes generated by the random key generation function. The result was ovewriting what Tcl expected to be inside Redis with another value, causing the next HSTRLEN test to fail.
The code was broken and resulted in redis-cli --pipe to, most of the times, writing everything received in the standard input to the Redis connection socket without ever reading back the replies, until all the content to write was written. This means that Redis had to accumulate all the output in the output buffers of the client, consuming a lot of memory. Fixed thanks to the original report of anomalies in the behavior provided by Twitter user @fsaintjacques.
This change allows a slave to properly time out a dead master during the extended asynchronous synchronization state machine. Now, slaves will record their last interaction with the master and apply the replication timeout before a response to the PSYNC request is received.
Fix master timeout during handshake
* Function to test for slave handshake renamed slaveIsInHandshakeState. * Function no longer accepts arguments since it always tests the same global state. * Test for state translated to a range test since defines are guaranteed to stay in order in the future. * Use the new function in the ROLE command implementation as well.
Make sure that people from the future will not break this rule. Related to issue #2813.
For an error I missed the last handshake state. Related to issue #2813.
In issue #2948 a crash was reported in processCommand(). Later Oran Agra (@oranagra) traced the bug (in private chat) in the following sequence of events: 1. Some maxmemory is set. 2. The slave is the currently active client and is executing PING or REPLCONF or whatever a slave can send to its master. 3. freeMemoryIfNeeded() is called since maxmemory is set. 4. flushSlavesOutputBuffers() is called by freeMemoryIfNeeded(). 5. During slaves buffers flush, a write error could be encoutered in writeToClient() or sendReplyToClient() depending on the version of Redis. This will trigger freeClient() against the currently active client, so a segmentation fault will likely happen in processCommand() immediately after the call to freeMemoryIfNeeded(). There are different possible fixes: 1. Add flags to writeToClient() (recent versions code base) so that we can ignore the write errors, and use this flag in flushSlavesOutputBuffers(). However this is not simple to do in older versions of Redis. 2. Use freeClientAsync() during write errors. This works but changes the current behavior of releasing clients ASAP when possible. Normally we write to clients during the normal event loop processing, in the writable client, where there is no active client, so no care must be taken. 3. The fix of this commit: to detect that the current client is no longer valid. This fix is a bit "ad-hoc", but works across all the versions and has the advantage of not changing the remaining behavior. Only alters what happens during this race condition, hopefully.
…buf free space in cmsgpack
fix to #4493 at 2.8 version
Fix to dict int problem at 2.8 version
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.