Relax visibility of Coordinator's methods for use by other ScalarDB components #2612
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR relaxes the visibility of certain Coordinator methods to allow reusability by other ScalarDB components. Key changes include:
- Extracting and making public the getStateByIdOnly method for state retrieval.
- Updating Coordinator and its tests to support both normal transaction IDs and group commit IDs.
- Adjusting the visibility of State#getChildIds and adding an overloaded State constructor.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| core/src/test/java/com/scalar/db/transaction/consensuscommit/CoordinatorTest.java | Added tests for the new getStateByIdOnly method with coverage for normal and group commit scenarios. |
| core/src/main/java/com/scalar/db/transaction/consensuscommit/Coordinator.java | Refactored state retrieval to use getStateByIdOnly and updated State’s API by making getChildIds public and adding a new constructor. |
| public Optional<Coordinator.State> getStateByIdOnly(String id) throws CoordinatorException { | ||
| Get get = createGetWith(id); | ||
| return get(get); | ||
| } |
There was a problem hiding this comment.
I feel that the already exposed getState() method behaves the same way when the ID is not a full ID for the coordinator group commit.
So, we need this method purely for performance reasons, without checking the ID format. Is that correct?
There was a problem hiding this comment.
@brfrn169 Sorry for lack of explanation. The current implementation doesn't allow external components to directly look up a state by the parent ID for the coordinator group commit. It's sometimes useful for optimization (e.g., for caching coordinator state records using parent IDs as keys) in external components.
I realized the method names introduced in this PR were a bit unclear from the use case perspective. I updated them in 496c8f8. I hope it clarifies the code.
I also updated the description of the PR.
to clarify the use cases
…omponents (#2612) Co-authored-by: Hiroyuki Yamada <mogwaing@gmail.com> Co-authored-by: Toshihiro Suzuki <brfrn169@gmail.com>
Description
com.scalar.db.transaction.consensuscommit.Coordinatorhas some implementation that are not public or reusable. For example, the current implementation doesn't allow external ScalarDB components to directly look up a state by the parent ID for the coordinator group commit. It's sometimes useful for optimization (e.g., for caching coordinator state records using parent IDs as keys) in external components.It's possible to inherit
Coordinatorclass in those external components and implements the same logic by themselves, but it would result in duplicated logics and implementations over multiple components.This PR makes those logics and implementations accessible in
Coordinatorclass so that they can be reused when necessary.Related issues and/or PRs
None
Changes made
public Optional<Coordinator.State> getStateByIdOnly(String id)to make it reusableCoordinator.State#getChildIds()publicChecklist
Additional notes (optional)
None
Release notes
N/A