Skip to content

Commit 36a22c6

Browse files
sanzmaurosanzmauro
committed
updated approach
1 parent 3f3c0fc commit 36a22c6

File tree

3 files changed

+80
-138
lines changed

3 files changed

+80
-138
lines changed

‎client/src/main/java/io/split/engine/common/SynchronizerImp.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -58,8 +58,8 @@ public void stopPeriodicFetching() {
5858
}
5959

6060
@Override
61-
public synchronized void refreshSplits(long targetChangeNumber) {
62-
while (targetChangeNumber > _splitFetcher.changeNumber()) {
61+
public void refreshSplits(long targetChangeNumber) {
62+
if (targetChangeNumber > _splitFetcher.changeNumber()) {
6363
_splitFetcher.forceRefresh();
6464
}
6565
}
@@ -73,8 +73,8 @@ public void localKillSplit(String splitName, String defaultTreatment, long newCh
7373
}
7474

7575
@Override
76-
public synchronized void refreshSegment(String segmentName, long changeNumber) {
77-
while (changeNumber > _segmentFetcher.getChangeNumber(segmentName)) {
76+
public void refreshSegment(String segmentName, long changeNumber) {
77+
if (changeNumber > _segmentFetcher.getChangeNumber(segmentName)) {
7878
_segmentFetcher.forceRefresh(segmentName);
7979
}
8080
}

‎client/src/main/java/io/split/engine/experiments/RefreshableSplitFetcher.java‎

Lines changed: 76 additions & 90 deletions
Original file line numberDiff line numberDiff line change
@@ -160,117 +160,103 @@ public void run() {
160160
}
161161

162162
public void runWithoutExceptionHandling() throws InterruptedException {
163-
SplitChange change = _splitChangeFetcher.fetch(_changeNumber.get());
163+
while (true) {
164+
SplitChange change = _splitChangeFetcher.fetch(_changeNumber.get());
164165

165-
if (change == null) {
166-
throw new IllegalStateException("SplitChange was null");
167-
}
168-
169-
if (change.till == _changeNumber.get()) {
170-
// no change.
171-
return;
172-
}
173-
174-
if (change.since != _changeNumber.get()
175-
|| change.till < _changeNumber.get()) {
176-
// some other thread may have updated the shared state. exit
177-
return;
178-
}
166+
if (change == null) {
167+
throw new IllegalStateException("SplitChange was null");
168+
}
179169

180-
if (change.splits.isEmpty()) {
181-
// there are no changes. weird!
182-
_changeNumber.set(change.till);
183-
return;
184-
}
170+
if (change.till == _changeNumber.get()) {
171+
// no change.
172+
break;
173+
}
185174

186-
synchronized (_lock) {
187-
// check state one more time.
188-
if (change.since != _changeNumber.get()
189-
|| change.till < _changeNumber.get()) {
175+
if (change.since != _changeNumber.get() || change.till < _changeNumber.get()) {
190176
// some other thread may have updated the shared state. exit
191-
return;
177+
break;
192178
}
193179

194-
Set<String> toRemove = Sets.newHashSet();
195-
Map<String, ParsedSplit> toAdd = Maps.newHashMap();
196-
List<String> trafficTypeNamesToRemove = Lists.newArrayList();
197-
List<String> trafficTypeNamesToAdd = Lists.newArrayList();
180+
if (change.splits.isEmpty()) {
181+
// there are no changes. weird!
182+
_changeNumber.set(change.till);
183+
break;
184+
}
198185

199-
for (Split split : change.splits) {
200-
if (Thread.currentThread().isInterrupted()) {
201-
throw new InterruptedException();
186+
synchronized (_lock) {
187+
// check state one more time.
188+
if (change.since != _changeNumber.get()
189+
|| change.till < _changeNumber.get()) {
190+
// some other thread may have updated the shared state. exit
191+
break;
202192
}
203193

204-
if (split.status != Status.ACTIVE) {
205-
// archive.
206-
toRemove.add(split.name);
207-
if (split.trafficTypeName != null) {
208-
trafficTypeNamesToRemove.add(split.trafficTypeName);
209-
}
210-
continue;
211-
}
194+
Set<String> toRemove = Sets.newHashSet();
195+
Map<String, ParsedSplit> toAdd = Maps.newHashMap();
196+
List<String> trafficTypeNamesToRemove = Lists.newArrayList();
197+
List<String> trafficTypeNamesToAdd = Lists.newArrayList();
212198

213-
ParsedSplit parsedSplit = _parser.parse(split);
214-
if (parsedSplit == null) {
215-
_log.info("We could not parse the experiment definition for: " + split.name + " so we are removing it completely to be careful");
216-
toRemove.add(split.name);
217-
if (split.trafficTypeName != null) {
218-
trafficTypeNamesToRemove.add(split.trafficTypeName);
199+
for (Split split : change.splits) {
200+
if (Thread.currentThread().isInterrupted()) {
201+
throw new InterruptedException();
219202
}
220-
continue;
221-
}
222-
223-
toAdd.put(split.name, parsedSplit);
224-
225-
// If the split already exists, this is either an update, or the split has been
226-
// deleted and recreated (possibly with a different traffic type).
227-
// If it's an update, the traffic type should NOT be increased.
228-
// If it's deleted & recreated, the old one should be decreased and the new one increased.
229-
// To handle both cases, we simply delete the old one if the split is present.
230-
// The new one is always increased.
231-
ParsedSplit current = _concurrentMap.get(split.name);
232-
if (current != null && current.trafficTypeName() != null) {
233-
trafficTypeNamesToRemove.add(current.trafficTypeName());
234-
}
235203

236-
if (split.trafficTypeName != null) {
237-
trafficTypeNamesToAdd.add(split.trafficTypeName);
238-
}
239-
}
204+
if (split.status != Status.ACTIVE) {
205+
// archive.
206+
toRemove.add(split.name);
207+
if (split.trafficTypeName != null) {
208+
trafficTypeNamesToRemove.add(split.trafficTypeName);
209+
}
210+
continue;
211+
}
240212

241-
_concurrentMap.putAll(toAdd);
242-
_concurrentTrafficTypeNameSet.addAll(trafficTypeNamesToAdd);
243-
//removeAll does not work here, since it wont remove all the occurrences, just one
244-
Multisets.removeOccurrences(_concurrentTrafficTypeNameSet, trafficTypeNamesToRemove);
213+
ParsedSplit parsedSplit = _parser.parse(split);
214+
if (parsedSplit == null) {
215+
_log.info("We could not parse the experiment definition for: " + split.name + " so we are removing it completely to be careful");
216+
toRemove.add(split.name);
217+
if (split.trafficTypeName != null) {
218+
trafficTypeNamesToRemove.add(split.trafficTypeName);
219+
}
220+
continue;
221+
}
245222

246-
for (String remove : toRemove) {
247-
_concurrentMap.remove(remove);
248-
}
223+
toAdd.put(split.name, parsedSplit);
224+
225+
// If the split already exists, this is either an update, or the split has been
226+
// deleted and recreated (possibly with a different traffic type).
227+
// If it's an update, the traffic type should NOT be increased.
228+
// If it's deleted & recreated, the old one should be decreased and the new one increased.
229+
// To handle both cases, we simply delete the old one if the split is present.
230+
// The new one is always increased.
231+
ParsedSplit current = _concurrentMap.get(split.name);
232+
if (current != null && current.trafficTypeName() != null) {
233+
trafficTypeNamesToRemove.add(current.trafficTypeName());
234+
}
249235

250-
if (!toAdd.isEmpty()) {
251-
_log.debug("Updated features: " + toAdd.keySet());
252-
}
236+
if (split.trafficTypeName != null) {
237+
trafficTypeNamesToAdd.add(split.trafficTypeName);
238+
}
239+
}
253240

254-
if (!toRemove.isEmpty()) {
255-
_log.debug("Deleted features: " + toRemove);
256-
}
241+
_concurrentMap.putAll(toAdd);
242+
_concurrentTrafficTypeNameSet.addAll(trafficTypeNamesToAdd);
243+
//removeAll does not work here, since it wont remove all the occurrences, just one
244+
Multisets.removeOccurrences(_concurrentTrafficTypeNameSet, trafficTypeNamesToRemove);
257245

258-
_changeNumber.set(change.till);
259-
}
246+
for (String remove : toRemove) {
247+
_concurrentMap.remove(remove);
248+
}
260249

261-
}
250+
if (!toAdd.isEmpty()) {
251+
_log.debug("Updated features: " + toAdd.keySet());
252+
}
262253

263-
private List<String> collectSegmentsInUse(Split split) {
264-
List<String> result = Lists.newArrayList();
265-
for (Condition condition : split.conditions) {
266-
for (Matcher matcher : condition.matcherGroup.matchers) {
267-
if (matcher.matcherType == MatcherType.IN_SEGMENT) {
268-
if (matcher.userDefinedSegmentMatcherData != null && matcher.userDefinedSegmentMatcherData.segmentName != null) {
269-
result.add(matcher.userDefinedSegmentMatcherData.segmentName);
270-
}
254+
if (!toRemove.isEmpty()) {
255+
_log.debug("Deleted features: " + toRemove);
271256
}
257+
258+
_changeNumber.set(change.till);
272259
}
273260
}
274-
return result;
275261
}
276262
}

‎client/src/test/java/io/split/engine/common/SynchronizerTest.java‎

Lines changed: 0 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -60,48 +60,4 @@ public void stopPeriodicFetching() {
6060
Mockito.verify(refreshableSplitFetcherProvider, Mockito.times(1)).stop();
6161
Mockito.verify(segmentFetcher, Mockito.times(1)).stop();
6262
}
63-
64-
@Test
65-
public void refreshSplits() {
66-
RefreshableSplitFetcherProvider refreshableSplitFetcherProvider = Mockito.mock(RefreshableSplitFetcherProvider.class);
67-
RefreshableSegmentFetcher segmentFetcher = Mockito.mock(RefreshableSegmentFetcher.class);
68-
RefreshableSplitFetcher splitFetcher = Mockito.mock(RefreshableSplitFetcher.class);
69-
70-
Mockito.when(refreshableSplitFetcherProvider.getFetcher())
71-
.thenReturn(splitFetcher);
72-
73-
Mockito.when(splitFetcher.changeNumber())
74-
.thenReturn(450L)
75-
.thenReturn(460L)
76-
.thenReturn(470L)
77-
.thenReturn(480L)
78-
.thenReturn(500L);
79-
80-
Synchronizer synchronizer = new SynchronizerImp(refreshableSplitFetcherProvider, segmentFetcher);
81-
synchronizer.refreshSplits(500);
82-
83-
Mockito.verify(splitFetcher, Mockito.times(4)).forceRefresh();
84-
}
85-
86-
@Test
87-
public void refreshSegment() {
88-
RefreshableSplitFetcherProvider refreshableSplitFetcherProvider = Mockito.mock(RefreshableSplitFetcherProvider.class);
89-
RefreshableSegmentFetcher segmentFetcher = Mockito.mock(RefreshableSegmentFetcher.class);
90-
RefreshableSplitFetcher splitFetcher = Mockito.mock(RefreshableSplitFetcher.class);
91-
92-
Mockito.when(refreshableSplitFetcherProvider.getFetcher())
93-
.thenReturn(splitFetcher);
94-
95-
Mockito.when(segmentFetcher.getChangeNumber("segment-name"))
96-
.thenReturn(450L)
97-
.thenReturn(460L)
98-
.thenReturn(470L)
99-
.thenReturn(480L)
100-
.thenReturn(500L);
101-
102-
Synchronizer synchronizer = new SynchronizerImp(refreshableSplitFetcherProvider, segmentFetcher);
103-
synchronizer.refreshSegment("segment-name", 500);
104-
105-
Mockito.verify(segmentFetcher, Mockito.times(4)).forceRefresh("segment-name");
106-
}
10763
}

0 commit comments

Comments
 (0)