Minor refactoring of slot migration source code #1373
Merged
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
tisonkun
reviewed
Apr 6, 2023
Comment on lines
+109
to
+110
| Status FinishSuccessfulMigration(); | ||
| Status FinishFailedMigration(); |
There was a problem hiding this comment.
These two method name changes seem wordy.
There was a problem hiding this comment.
I thought that new names clearly describe what they do.
tisonkun
reviewed
Apr 6, 2023
| @@ -111,7 +111,7 @@ class ServerLogData { | |||
| }; | |||
|
|
|||
| class SlotImport; | |||
| class SlotMigrate; | |||
| class SlotMigrator; | |||
There was a problem hiding this comment.
If we just name SlotImport and SlotMigrate, let's keep it as is.
There was a problem hiding this comment.
I got your point. The reason is that I didn't touch slot_import.h/.cc in this PR.
tisonkun
reviewed
Apr 6, 2023
|
@tisonkun You are absolutely right: it's hard to review because of so many changes. |
git-hulk
reviewed
Apr 7, 2023
git-hulk
approved these changes
Apr 7, 2023
|
@PragmaTwice Could you please have a look at my PR? As @tisonkun advised, I can split it into several distinct PRs to make the review more convenient. What do you think? |
PragmaTwice
approved these changes
Apr 7, 2023
|
@PragmaTwice I'll handle |
|
Thanks all, merging... |
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.
Unused variables were removed.
Some variables and methods were renamed.
Destination socket descriptor wrapped by
UniqueFDand moved outsideSlotMigrationJob(now it contains only migration parameters ).Enum classes were used instead of plain enums.
Move the logic of setting the forbidden slot into
SetForbiddenSlotand call it explicitly between two calls of syncing from WAL.