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

improve rename logic to use 2 phase commit idea #600

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

stevie9868
Copy link
Contributor

No description provided.

@stevie9868 stevie9868 force-pushed the yingjianw/ParentChildRename branch 2 times, most recently from 697e605 to b2275f5 Compare July 4, 2024 15:34
@stevie9868 stevie9868 force-pushed the yingjianw/ParentChildRename branch 3 times, most recently from b6d9a54 to d087a04 Compare July 10, 2024 14:33
Copy link
Contributor

@amboz amboz left a comment

Choose a reason for hiding this comment

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

awesome amount of test cases/coverage

*
* @param oldName the current name to be renamed
* @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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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))
Copy link
Contributor

Choose a reason for hiding this comment

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

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

*
* @param oldName the current name to be renamed
* @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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 oldName the current name to be renamed
* @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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Otherwise the wording is such that it looks like the whole method is always a noop

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants