Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add timestamp annotations in AOF #9326

Merged
merged 7 commits into from
Oct 25, 2021
Merged

Add timestamp annotations in AOF #9326

merged 7 commits into from
Oct 25, 2021

Conversation

ShooterIT
Copy link
Collaborator

@ShooterIT ShooterIT commented Aug 6, 2021

Add timestamp annotation in AOF, one part of #9325.

Enabled with the new aof-timestamp-enabled config option.

Timestamp annotation format is "#TS:${timestamp}\r\n"."
TS" is short of timestamp and this method could save extra bytes in AOF.

We can use timestamp annotation for some special functions.

  • know the executing time of commands
  • restore data to a specific point-in-time (by using redis-check-aof to truncate the file)

@ShooterIT ShooterIT marked this pull request as ready for review August 6, 2021 11:14
Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems a bit odd that we'll merge this when we don't yet do anything with it.
maybe we should improve aof-check with some feature to truncate the AOF after a certain time?
or add a feature for redis in which it stops the loading when reaching a certain time? (so you don't need to process a huge aof file twice)

src/aof.c Outdated Show resolved Hide resolved
@ShooterIT
Copy link
Collaborator Author

Yes, we should provide tool or support to restore data to a specific time.
Currently, i enhance aof-check to support truncate AOF to specific timestamp, users could decide to load or discard by the result of truncating AOF, users may not want to load AOF if can't find specific timestamp from AOF, maybe AOF is too new or AOF doesn't have timestamp annotation.

add a feature for redis in which it stops the loading when reaching a certain time

Initially, i also want to do this, but there are some worries. First, The config of stop time is dangerous, redis may only load old data if we keep this config in config file. And users usually download backup AOF and reload to specific time for special purposes , and we also face there are some scenarios, no timestamps or not finding specific timestamp in AOF, it is not appropriate to directly do something without asking opinions of users or corresponding configs.

src/redis-check-aof.c Show resolved Hide resolved
src/redis-check-aof.c Show resolved Hide resolved
@oranagra
Copy link
Member

@redis/core-team please approve the concept, new config option, and new redis-check-aof feature.

@oranagra oranagra added approval-needed Waiting for core team approval to be merged release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus labels Aug 29, 2021
Copy link
Member

@itamarhaber itamarhaber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

redis.conf Outdated Show resolved Hide resolved
*
* Timestamp annotation format is "#TS:${timestamp}\r\n". "TS" is short of
* timestamp and this method could save extra bytes in AOF. */
sds genAofTimestampAnnotationIfNeeded(int force) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will the annotations generally be of the format: #{name}:{value}\r\n ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also forgive my ignorance about AOF, but I thought it generally followed the RESP protocol, so instead should we use the attribute type notation from RESP3 which describes a similar syntax.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @madolson the format: #{name}:{value}\r\n is only one of annotations, the format is loose #9325.
Initially, i want to use a special command to identify annotations, but AOF is the set of write commands, so it will be odd, maybe we even add one more command for annotations, so i abandon.
The attribute type of RESP3 can represent notation, but i think it is for reply, i am not sure.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was reading this comment:

AOF (Append Only File): The AOF persistence logs every write operation received by the server, that will be played again at server startup, reconstructing the original dataset. Commands are logged using the same format as the Redis protocol itself, in an append-only fashion. Redis is able to rewrite the log in the background when it gets too big.

So if someone was tailing it, they could use a RESP parser to understand what was going on. Maybe that is a dumb comment and someone that runs a lot of AOF systems can tell me otherwise, I just wanted to mention it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think we must look too much into that comment.
i think it is ok for the annotations to be AOF "comments" and not use RESP3 attributes.
First because AOF is indeed requests (which aren't actually using RESP3, just multi-bulks), and secondly, even if requests in the future will be using attributes, maybe we don't want to collide with them.
i.e. the annotations are (for now) generic comments about the AOF, and not explicitly referring to the immediate request that follows (next command).

so this actually raises another problem. what if AOF file will in the future wanna use the RESP3 booleans? (which use #)
maybe, just to be on the safe side, we should chose another character? or use two ## instead of one?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think - should be safe, even in a future where we accept RESP3 formatted commands it's not likely that clients will be sending errors to servers.

Copy link
Collaborator Author

@ShooterIT ShooterIT Sep 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use RESP3 in AOF, maybe timestamp looks like that. But it brings much complexity for processMultibulkBuffer. i am not sure we will support in the future.

*3
|1
+ts
:1632711499
$3
set
$1
a
$1
b

The reason why I use # is that # is some languages' annotation. Currently, RESP3 already used many normal characters. i have no idea which character to use.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use / instead? Kind of like a comment?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/ is a comment? you're referring to //?
I feel this discussion is a dead end.. let's stick to #

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @oranagra for your merging unstable branch
even we accept RESP3 formatted commands, we just support some RESP3 fields in multi-bulks, right? So i think annotation prefix(#, one character just is different from *) doesn't do harm since we do parse based on some context.
We need to push it forward, if we don't strongly oppose #, i think we can adopt it.

For AOF annotations, maybe we could support some descriptions that are similar with RDB AUX fields if needed, such as version(when we add new data type, change it), module info, memory usage...

@oranagra oranagra mentioned this pull request Oct 14, 2021
@oranagra oranagra merged commit 9ec3294 into redis:unstable Oct 25, 2021
@ShooterIT ShooterIT deleted the aof-ts branch October 25, 2021 10:24
oranagra pushed a commit that referenced this pull request Oct 26, 2021
Fix timing issue of a new test introduced in #9326
@oranagra oranagra mentioned this pull request Nov 11, 2021
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval-needed Waiting for core team approval to be merged release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants