Skip to content

Commit 1b441e0

Browse files
komamitsufeeblefakiebrfrn169
authored
Relax visibility of Coordinator's methods for use by other ScalarDB components (#2612)
Co-authored-by: Hiroyuki Yamada <mogwaing@gmail.com> Co-authored-by: Toshihiro Suzuki <brfrn169@gmail.com>
1 parent f14554c commit 1b441e0

File tree

2 files changed

+129
-7
lines changed

2 files changed

+129
-7
lines changed

‎core/src/main/java/com/scalar/db/transaction/consensuscommit/Coordinator.java‎

Lines changed: 62 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -72,24 +72,40 @@ public Coordinator(DistributedStorage storage, ConsensusCommitConfig config) {
7272
keyManipulator = new CoordinatorGroupCommitKeyManipulator();
7373
}
7474

75+
/**
76+
* Gets the coordinator state by ID. If the ID is a full ID for the coordinator group commit, it
77+
* will look up the state using the parent ID and the child ID. Otherwise, it will look up the
78+
* state only by ID.
79+
*
80+
* @param id the ID of the coordinator state
81+
* @return the coordinator state
82+
* @throws CoordinatorException if the coordinator state cannot be retrieved
83+
*/
7584
public Optional<Coordinator.State> getState(String id) throws CoordinatorException {
7685
if (keyManipulator.isFullKey(id)) {
7786
return getStateForGroupCommit(id);
7887
}
7988

80-
Get get = createGetWith(id);
81-
return get(get);
89+
return getStateInternal(id);
8290
}
8391

92+
/**
93+
* Gets the coordinator state for a group commit by ID. It first looks up the state using the
94+
* parent ID and then checks if the child ID is contained in the state. If the child ID is not
95+
* found, it will look up the state using the full ID.
96+
*
97+
* @param fullId the full ID for the coordinator group commit
98+
* @return the coordinator state
99+
* @throws CoordinatorException if the coordinator state cannot be retrieved
100+
*/
84101
@VisibleForTesting
85102
Optional<Coordinator.State> getStateForGroupCommit(String fullId) throws CoordinatorException {
86103
// Scan with the parent ID for a normal group that contains multiple transactions.
87104
Keys<String, String, String> idForGroupCommit = keyManipulator.keysFromFullKey(fullId);
88105

89106
String parentId = idForGroupCommit.parentKey;
90107
String childId = idForGroupCommit.childKey;
91-
Get get = createGetWith(parentId);
92-
Optional<State> state = get(get);
108+
Optional<State> state = getStateByParentId(parentId);
93109
// The current implementation is optimized for cases where most transactions are
94110
// group-committed. It first looks up a transaction state using the parent ID with a single read
95111
// operation. If no matching transaction state is found (i.e., the transaction was delayed and
@@ -106,7 +122,41 @@ Optional<Coordinator.State> getStateForGroupCommit(String fullId) throws Coordin
106122
return stateContainingTargetTxId;
107123
}
108124

109-
return get(createGetWith(fullId));
125+
return getStateByFullId(fullId);
126+
}
127+
128+
private Optional<Coordinator.State> getStateInternal(String id) throws CoordinatorException {
129+
Get get = createGetWith(id);
130+
return get(get);
131+
}
132+
133+
/**
134+
* Gets the coordinator state by the parent ID for the coordinator group commit. Note: The scope
135+
* of this method has public visibility, but is intended for internal use. Also, the method only
136+
* calls {@link #getStateInternal(String)} with the parent ID, but it exists as a separate method
137+
* for clarifying this specific use case.
138+
*
139+
* @param parentId the parent ID of the coordinator state for the coordinator group commit
140+
* @return the coordinator state
141+
* @throws CoordinatorException if the coordinator state cannot be retrieved
142+
*/
143+
public Optional<Coordinator.State> getStateByParentId(String parentId)
144+
throws CoordinatorException {
145+
return getStateInternal(parentId);
146+
}
147+
148+
/**
149+
* Gets the coordinator state by the full ID for the coordinator group commit. Note: The scope of
150+
* this method has public visibility, but is intended for internal use. Also, the method only
151+
* calls {@link #getStateInternal(String)} with the parent ID, but it exists as a separate method
152+
* for clarifying this specific use case.
153+
*
154+
* @param fullId the parent ID of the coordinator state for the coordinator group commit
155+
* @return the coordinator state
156+
* @throws CoordinatorException if the coordinator state cannot be retrieved
157+
*/
158+
public Optional<Coordinator.State> getStateByFullId(String fullId) throws CoordinatorException {
159+
return getStateInternal(fullId);
110160
}
111161

112162
public void putState(Coordinator.State state) throws CoordinatorException {
@@ -332,6 +382,10 @@ public State(String id, TransactionState state) {
332382
this(id, state, System.currentTimeMillis());
333383
}
334384

385+
public State(String id, List<String> childIds, TransactionState state) {
386+
this(id, childIds, state, System.currentTimeMillis());
387+
}
388+
335389
@VisibleForTesting
336390
State(String id, List<String> childIds, TransactionState state, long createdAt) {
337391
this.id = checkNotNull(id);
@@ -367,8 +421,9 @@ public long getCreatedAt() {
367421
return createdAt;
368422
}
369423

370-
@VisibleForTesting
371-
List<String> getChildIds() {
424+
@SuppressFBWarnings("EI_EXPOSE_REP")
425+
@Nonnull
426+
public List<String> getChildIds() {
372427
return childIds;
373428
}
374429

‎core/src/test/java/com/scalar/db/transaction/consensuscommit/CoordinatorTest.java‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,73 @@ public void getState_TransactionIdGivenAndExceptionThrownInGet_ShouldThrowCoordi
102102
assertThatThrownBy(() -> coordinator.getState(id)).isInstanceOf(CoordinatorException.class);
103103
}
104104

105+
@Test
106+
public void getStateByParentId_GroupCommitParentIdGiven_ShouldReturnStateUsingItParent()
107+
throws ExecutionException, CoordinatorException {
108+
// Arrange
109+
CoordinatorGroupCommitKeyManipulator keyManipulator =
110+
new CoordinatorGroupCommitKeyManipulator();
111+
String parentId = keyManipulator.generateParentKey();
112+
String childIdsStr =
113+
String.join(
114+
",",
115+
UUID.randomUUID().toString(),
116+
UUID.randomUUID().toString(),
117+
UUID.randomUUID().toString());
118+
119+
Result result = mock(Result.class);
120+
when(result.getValue(Attribute.ID))
121+
.thenReturn(Optional.of(new TextValue(Attribute.ID, parentId)));
122+
when(result.getValue(Attribute.CHILD_IDS))
123+
.thenReturn(Optional.of(new TextValue(Attribute.CHILD_IDS, childIdsStr)));
124+
when(result.getValue(Attribute.STATE))
125+
.thenReturn(Optional.of(new IntValue(Attribute.STATE, TransactionState.ABORTED.get())));
126+
when(result.getValue(Attribute.CREATED_AT))
127+
.thenReturn(Optional.of(new BigIntValue(Attribute.CREATED_AT, ANY_TIME_1)));
128+
when(storage.get(any(Get.class))).thenReturn(Optional.of(result));
129+
130+
// Act
131+
Optional<Coordinator.State> state = coordinator.getStateByParentId(parentId);
132+
133+
// Assert
134+
assertThat(state.get().getId()).isEqualTo(parentId);
135+
assertThat(state.get().getChildIds()).isEqualTo(Arrays.asList(childIdsStr.split(",")));
136+
assertThat(state.get().getChildIdsAsString()).isEqualTo(childIdsStr);
137+
Assertions.assertThat(state.get().getState()).isEqualTo(TransactionState.ABORTED);
138+
assertThat(state.get().getCreatedAt()).isEqualTo(ANY_TIME_1);
139+
}
140+
141+
@Test
142+
public void getStateByFullId_GroupCommitFullIdGiven_ShouldReturnStateUsingItParent()
143+
throws ExecutionException, CoordinatorException {
144+
// Arrange
145+
CoordinatorGroupCommitKeyManipulator keyManipulator =
146+
new CoordinatorGroupCommitKeyManipulator();
147+
String fullId =
148+
keyManipulator.fullKey(keyManipulator.generateParentKey(), UUID.randomUUID().toString());
149+
150+
Result result = mock(Result.class);
151+
when(result.getValue(Attribute.ID))
152+
.thenReturn(Optional.of(new TextValue(Attribute.ID, fullId)));
153+
when(result.getValue(Attribute.CHILD_IDS))
154+
.thenReturn(Optional.of(new TextValue(Attribute.CHILD_IDS, EMPTY_CHILD_IDS)));
155+
when(result.getValue(Attribute.STATE))
156+
.thenReturn(Optional.of(new IntValue(Attribute.STATE, TransactionState.ABORTED.get())));
157+
when(result.getValue(Attribute.CREATED_AT))
158+
.thenReturn(Optional.of(new BigIntValue(Attribute.CREATED_AT, ANY_TIME_1)));
159+
when(storage.get(any(Get.class))).thenReturn(Optional.of(result));
160+
161+
// Act
162+
Optional<Coordinator.State> state = coordinator.getStateByFullId(fullId);
163+
164+
// Assert
165+
assertThat(state.get().getId()).isEqualTo(fullId);
166+
assertThat(state.get().getChildIds()).isEmpty();
167+
assertThat(state.get().getChildIdsAsString()).isEmpty();
168+
Assertions.assertThat(state.get().getState()).isEqualTo(TransactionState.ABORTED);
169+
assertThat(state.get().getCreatedAt()).isEqualTo(ANY_TIME_1);
170+
}
171+
105172
@Test
106173
public void putState_StateGiven_ShouldPutWithCorrectValues()
107174
throws ExecutionException, CoordinatorException {

0 commit comments

Comments
 (0)