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

Refactoring CoordinatorGroupCommitKeyManipulator by moving the implementation to DefaultGroupCommitKeyManipulator to improve the reusability #2389

Merged
merged 2 commits into from
Dec 16, 2024

Conversation

komamitsu
Copy link
Contributor

@komamitsu komamitsu commented Dec 5, 2024

Description

In the current implementation, CoordinatorGroupCommitKeyManipulator designed to manipulate GroupCommit keys that are all String. But, the functionality should be reusable in other use cases, not only for the Coordinator Group Commit feature. This PR moves the implementation of CoordinatorGroupCommitKeyManipulator to DefaultGroupCommitKeyManipulator to make the functionality easier to reuse.

Related issues and/or PRs

Changes made

  • Move the implementation of CoordinatorGroupCommitKeyManipulator to DefaultGroupCommitKeyManipulator. CoordinatorGroupCommitKeyManipulator inherits DefaultGroupCommitKeyManipulator now.
  • Rename interface KeyManipulator to GroupCommitKeyManipulator to clarify what kind of keys the interface manipulate.

Checklist

The following is a best-effort checklist. If any items in this checklist are not applicable to this PR or are dependent on other, unmerged PRs, please still mark the checkboxes after you have read and understood each item.

  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation to reflect the changes.
  • Any remaining open issues linked to this PR are documented and up-to-date (Jira, GitHub, etc.).
  • Tests (unit, integration, etc.) have been added for the changes.
  • My changes generate no new warnings.
  • Any dependent changes in other PRs have been merged and published.

Additional notes (optional)

This changes addresses this issue.

Release notes

N/A

to DefaultGroupCommitKeyManipulator to re-use it
@komamitsu komamitsu self-assigned this Dec 5, 2024
@komamitsu komamitsu changed the title Move the implementation of CoordinatorGroupCommitKeyManipulator to DefaultGroupCommitKeyManipulator to easily re-use it [Refactoring] Move the implementation of CoordinatorGroupCommitKeyManipulator to DefaultGroupCommitKeyManipulator to easily re-use it Dec 5, 2024
@komamitsu komamitsu changed the title [Refactoring] Move the implementation of CoordinatorGroupCommitKeyManipulator to DefaultGroupCommitKeyManipulator to easily re-use it Refactoring CoordinatorGroupCommitKeyManipulator by moving the implementation to DefaultGroupCommitKeyManipulator to improve the reusability Dec 5, 2024
Copy link
Contributor

@Torch3333 Torch3333 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

Copy link
Collaborator

@brfrn169 brfrn169 left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

Copy link
Contributor

@feeblefakie feeblefakie left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

@komamitsu komamitsu merged commit 00949f0 into master Dec 16, 2024
48 checks passed
@komamitsu komamitsu deleted the refactor-group-commit-key-manipulator branch December 16, 2024 02:03
feeblefakie pushed a commit that referenced this pull request Dec 16, 2024
…ementation to `DefaultGroupCommitKeyManipulator` to improve the reusability (#2389)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants