Skip to content

Commit f21eb24

Browse files
feeblefakiekomamitsubrfrn169
authored
Backport to branch(3) : Relax visibility of Coordinator's methods for use by other ScalarDB components (#2638)
Co-authored-by: Mitsunori Komatsu <komamitsu@gmail.com> Co-authored-by: Toshihiro Suzuki <brfrn169@gmail.com>
1 parent 1a56b71 commit f21eb24

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
@@ -79,24 +79,40 @@ public Coordinator(DistributedStorage storage, ConsensusCommitConfig config) {
7979
keyManipulator = new CoordinatorGroupCommitKeyManipulator();
8080
}
8181

82+
/**
83+
* Gets the coordinator state by ID. If the ID is a full ID for the coordinator group commit, it
84+
* will look up the state using the parent ID and the child ID. Otherwise, it will look up the
85+
* state only by ID.
86+
*
87+
* @param id the ID of the coordinator state
88+
* @return the coordinator state
89+
* @throws CoordinatorException if the coordinator state cannot be retrieved
90+
*/
8291
public Optional<Coordinator.State> getState(String id) throws CoordinatorException {
8392
if (keyManipulator.isFullKey(id)) {
8493
return getStateForGroupCommit(id);
8594
}
8695

87-
Get get = createGetWith(id);
88-
return get(get);
96+
return getStateInternal(id);
8997
}
9098

99+
/**
100+
* Gets the coordinator state for a group commit by ID. It first looks up the state using the
101+
* parent ID and then checks if the child ID is contained in the state. If the child ID is not
102+
* found, it will look up the state using the full ID.
103+
*
104+
* @param fullId the full ID for the coordinator group commit
105+
* @return the coordinator state
106+
* @throws CoordinatorException if the coordinator state cannot be retrieved
107+
*/
91108
@VisibleForTesting
92109
Optional<Coordinator.State> getStateForGroupCommit(String fullId) throws CoordinatorException {
93110
// Scan with the parent ID for a normal group that contains multiple transactions.
94111
Keys<String, String, String> idForGroupCommit = keyManipulator.keysFromFullKey(fullId);
95112

96113
String parentId = idForGroupCommit.parentKey;
97114
String childId = idForGroupCommit.childKey;
98-
Get get = createGetWith(parentId);
99-
Optional<State> state = get(get);
115+
Optional<State> state = getStateByParentId(parentId);
100116
// The current implementation is optimized for cases where most transactions are
101117
// group-committed. It first looks up a transaction state using the parent ID with a single read
102118
// operation. If no matching transaction state is found (i.e., the transaction was delayed and
@@ -113,7 +129,41 @@ Optional<Coordinator.State> getStateForGroupCommit(String fullId) throws Coordin
113129
return stateContainingTargetTxId;
114130
}
115131

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

119169
public void putState(Coordinator.State state) throws CoordinatorException {
@@ -339,6 +389,10 @@ public State(String id, TransactionState state) {
339389
this(id, state, System.currentTimeMillis());
340390
}
341391

392+
public State(String id, List<String> childIds, TransactionState state) {
393+
this(id, childIds, state, System.currentTimeMillis());
394+
}
395+
342396
@VisibleForTesting
343397
State(String id, List<String> childIds, TransactionState state, long createdAt) {
344398
this.id = checkNotNull(id);
@@ -374,8 +428,9 @@ public long getCreatedAt() {
374428
return createdAt;
375429
}
376430

377-
@VisibleForTesting
378-
List<String> getChildIds() {
431+
@SuppressFBWarnings("EI_EXPOSE_REP")
432+
@Nonnull
433+
public List<String> getChildIds() {
379434
return childIds;
380435
}
381436

‎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)