-
Notifications
You must be signed in to change notification settings - Fork 883
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
chore: clean up save #1709
chore: clean up save #1709
Conversation
5393469
to
1664d6a
Compare
Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>
1664d6a
to
75651dd
Compare
@kostasrim Please take a brief look, I might proceed with some more changes. Some clarity comments might be missing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably the next step would be to move all the snapshot code out of server_family.cc
You mean in this PR? |
src/server/server_family.cc
Outdated
if (!is_cloud_) | ||
full_path_ += ".tmp"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to add the .tmp suffix only if its not saved on cloud. Previously it was added in any case, yet snapshots are not renamed for the cloud env. I assume this was a bug
@@ -522,6 +516,247 @@ bool IsReplicatingNoOne(string_view host, string_view port) { | |||
return absl::EqualsIgnoreCase(host, "no") && absl::EqualsIgnoreCase(port, "one"); | |||
} | |||
|
|||
struct SaveStagesInputs { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
according to the stylieguide, consider make it a class and declare variables as protected (they have underscores).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved this out to a separate struct only to use aggregate initialization, its not possible with private/protected members. Alternatively I can just move the fields back and make a separate gigantic contructor that accepts them all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, how about the option of making the fields public (without underscores) and make this struct a data member instead of an inheritance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will open a separate fix on this 😬
no :) |
lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice, much much simpler
Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>
d8de3ba
to
f771b35
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The current
DoSave()
became a 200+ loc monster that is not only difficult to extend but also to read. I noticed this when I wanted to extend it for search serialization.Should not include any functional changes