improve rename logic to use 2 phase commit idea - #600
Open
yingjianwu98 wants to merge 1 commit into
Open
Conversation
yingjianwu98
force-pushed
the
yingjianw/ParentChildRename
branch
2 times, most recently
from
July 4, 2024 15:34
697e605 to
b2275f5
Compare
yingjianwu98
force-pushed
the
yingjianw/ParentChildRename
branch
3 times, most recently
from
July 10, 2024 14:33
b6d9a54 to
d087a04
Compare
yingjianwu98
force-pushed
the
yingjianw/ParentChildRename
branch
from
July 10, 2024 19:51
d087a04 to
b538e55
Compare
amboz
approved these changes
Jul 23, 2024
amboz
left a comment
amboz
left a comment
Collaborator
There was a problem hiding this comment.
awesome amount of test cases/coverage
| * @param newName the new name to rename to | ||
| * @return return a pair of set, | ||
| * where the first set represents the affected parent_uuid with parent = oldName | ||
| * and the second set represents the affected child_uuid with child = oldName |
Collaborator
There was a problem hiding this comment.
Suggested change
| * and the second set represents the affected child_uuid with child = oldName | |
| * and the second set represents the affected child_uuid with child = oldName | |
| * This is a noop and will return a pair of empty sets in the event there are no parent or child records for oldName |
Contributor
There was a problem hiding this comment.
Suggested change
| * and the second set represents the affected child_uuid with child = oldName | |
| * In the event there are no parent or child records for oldName this is a noop and will return a pair of empty sets |
Contributor
There was a problem hiding this comment.
Otherwise the wording is such that it looks like the whole method is always a noop
| // that are impacted so later during | ||
| // either dropping the oldName after successful connectorTableServiceProxy.rename | ||
| // or dropping the newName after failed connectorTableServiceProxy.rename | ||
| // we only touch records with those specific uuids |
Collaborator
There was a problem hiding this comment.
Suggested change
| // we only touch records with those specific uuids | |
| // we only touch records with those specific uuids, if any |
| then: | ||
| 1 * config.getNoTableRenameOnTags() >> [] | ||
| 1 * config.isParentChildRenameEnabled() >> true | ||
| 0 * parentChildRelSvc.isChildTable(oldName) |
Collaborator
There was a problem hiding this comment.
Suggested change
| 0 * parentChildRelSvc.isChildTable(oldName) | |
| 0 * parentChildRelSvc.isChildTable(oldName) | |
| 1 * parentChildRelSvc.isChildTable(newName) |
| 1 * config.getNoTableRenameOnTags() >> [] | ||
| 1 * config.isParentChildRenameEnabled() >> true | ||
| 0 * parentChildRelSvc.isChildTable(oldName) | ||
| 0 * parentChildRelSvc.isParentTable(oldName) |
Collaborator
There was a problem hiding this comment.
Suggested change
| 0 * parentChildRelSvc.isParentTable(oldName) | |
| 0 * parentChildRelSvc.isParentTable(oldName) | |
| 1 * parentChildRelSvc.isParentTable(newName) |
| 0 * parentChildRelSvc.isParentTable(oldName) | ||
| 1 * parentChildRelSvc.renameSoftInsert(oldName, newName) >> emptyPair | ||
| 1 * connectorTableServiceProxy.rename(oldName, newName, _) | ||
| 0 * parentChildRelSvc.drop(oldName, Optional.of(emptyPair)) |
Collaborator
There was a problem hiding this comment.
Suggested change
| 0 * parentChildRelSvc.drop(oldName, Optional.of(emptyPair)) | |
| 0 * parentChildRelSvc.drop(_, _) |
nit: just more comprehensive sicne we dont expect any dropping when the pair was just empty sets
insyncoss
approved these changes
Jul 23, 2024
| * @param newName the new name to rename to | ||
| * @return return a pair of set, | ||
| * where the first set represents the affected parent_uuid with parent = oldName | ||
| * and the second set represents the affected child_uuid with child = oldName |
Contributor
There was a problem hiding this comment.
Suggested change
| * and the second set represents the affected child_uuid with child = oldName | |
| * In the event there are no parent or child records for oldName this is a noop and will return a pair of empty sets |
| * @param newName the new name to rename to | ||
| * @return return a pair of set, | ||
| * where the first set represents the affected parent_uuid with parent = oldName | ||
| * and the second set represents the affected child_uuid with child = oldName |
Contributor
There was a problem hiding this comment.
Otherwise the wording is such that it looks like the whole method is always a noop
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.