Skip to content

Commit

Permalink
[BWC and API enforcement] Reduce the visibility of some existing APIs
Browse files Browse the repository at this point in the history
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
  • Loading branch information
reta committed Nov 16, 2023
1 parent 37ec313 commit f5ce904
Show file tree
Hide file tree
Showing 10 changed files with 57 additions and 59 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
import org.apache.logging.log4j.Logger;
import org.opensearch.action.OriginalIndices;
import org.opensearch.common.Nullable;
import org.opensearch.common.annotation.PublicApi;
import org.opensearch.common.annotation.InternalApi;
import org.opensearch.common.lease.Releasable;
import org.opensearch.common.util.concurrent.AtomicArray;
import org.opensearch.search.SearchPhaseResult;
Expand All @@ -49,9 +49,9 @@
/**
* This class provide contextual state and access to resources across multiple search phases.
*
* @opensearch.api
* @opensearch.internal
*/
@PublicApi(since = "1.0.0")
@InternalApi
public interface SearchPhaseContext extends Executor {
// TODO maybe we can make this concrete later - for now we just implement this in the base class for all initial phases

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

import org.apache.logging.log4j.LogManager;
import org.apache.lucene.search.TotalHits;
import org.opensearch.common.annotation.InternalApi;

import java.util.EnumMap;
import java.util.HashMap;
Expand All @@ -22,7 +23,8 @@
*
* @opensearch.internal
*/
public class SearchRequestContext {
@InternalApi
class SearchRequestContext {
private final SearchRequestOperationsListener searchRequestOperationsListener;
private long absoluteStartNanos;
private final Map<String, Long> phaseTookMap;
Expand All @@ -32,11 +34,11 @@ public class SearchRequestContext {
/**
* This constructor is for testing only
*/
public SearchRequestContext() {
SearchRequestContext() {
this(new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()));
}

public SearchRequestContext(SearchRequestOperationsListener searchRequestOperationsListener) {
SearchRequestContext(SearchRequestOperationsListener searchRequestOperationsListener) {
this.searchRequestOperationsListener = searchRequestOperationsListener;
this.absoluteStartNanos = System.nanoTime();
this.phaseTookMap = new HashMap<>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,23 +10,23 @@

import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.message.ParameterizedMessage;
import org.opensearch.common.annotation.PublicApi;
import org.opensearch.common.annotation.InternalApi;

import java.util.List;

/**
* A listener for search, fetch and context events at the coordinator node level
*
* @opensearch.api
* @opensearch.internal
*/
@PublicApi(since = "1.0.0")
public interface SearchRequestOperationsListener {
@InternalApi
interface SearchRequestOperationsListener {

void onPhaseStart(SearchPhaseContext context);
void onPhaseStart(SearchPhase phase);

void onPhaseEnd(SearchPhaseContext context, SearchRequestContext searchRequestContext);
void onPhaseEnd(SearchPhase context, SearchRequestContext searchRequestContext);

void onPhaseFailure(SearchPhaseContext context);
void onPhaseFailure(SearchPhase phase);

default void onRequestStart(SearchRequestContext searchRequestContext) {}

Expand All @@ -48,32 +48,32 @@ public CompositeListener(List<SearchRequestOperationsListener> listeners, Logger
}

@Override
public void onPhaseStart(SearchPhaseContext context) {
public void onPhaseStart(SearchPhase phase) {
for (SearchRequestOperationsListener listener : listeners) {
try {
listener.onPhaseStart(context);
listener.onPhaseStart(phase);
} catch (Exception e) {
logger.warn(() -> new ParameterizedMessage("onPhaseStart listener [{}] failed", listener), e);
}
}
}

@Override
public void onPhaseEnd(SearchPhaseContext context, SearchRequestContext searchRequestContext) {
public void onPhaseEnd(SearchPhase phase, SearchRequestContext searchRequestContext) {
for (SearchRequestOperationsListener listener : listeners) {
try {
listener.onPhaseEnd(context, searchRequestContext);
listener.onPhaseEnd(phase, searchRequestContext);
} catch (Exception e) {
logger.warn(() -> new ParameterizedMessage("onPhaseEnd listener [{}] failed", listener), e);
}
}
}

@Override
public void onPhaseFailure(SearchPhaseContext context) {
public void onPhaseFailure(SearchPhase phase) {
for (SearchRequestOperationsListener listener : listeners) {
try {
listener.onPhaseFailure(context);
listener.onPhaseFailure(phase);
} catch (Exception e) {
logger.warn(() -> new ParameterizedMessage("onPhaseFailure listener [{}] failed", listener), e);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,13 +134,13 @@ public SearchRequestSlowLog(ClusterService clusterService) {
}

@Override
public void onPhaseStart(SearchPhaseContext context) {}
public void onPhaseStart(SearchPhase phase) {}

@Override
public void onPhaseEnd(SearchPhaseContext context, SearchRequestContext searchRequestContext) {}
public void onPhaseEnd(SearchPhase phase, SearchRequestContext searchRequestContext) {}

@Override
public void onPhaseFailure(SearchPhaseContext context) {}
public void onPhaseFailure(SearchPhase phase) {}

@Override
public void onRequestStart(SearchRequestContext searchRequestContext) {}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,21 +46,21 @@ public long getPhaseMetric(SearchPhaseName searchPhaseName) {
}

@Override
public void onPhaseStart(SearchPhaseContext context) {
phaseStatsMap.get(context.getCurrentPhase().getSearchPhaseName()).current.inc();
public void onPhaseStart(SearchPhase phase) {
phaseStatsMap.get(phase.getSearchPhaseName()).current.inc();
}

@Override
public void onPhaseEnd(SearchPhaseContext context, SearchRequestContext searchRequestContext) {
StatsHolder phaseStats = phaseStatsMap.get(context.getCurrentPhase().getSearchPhaseName());
public void onPhaseEnd(SearchPhase phase, SearchRequestContext searchRequestContext) {
StatsHolder phaseStats = phaseStatsMap.get(phase.getSearchPhaseName());
phaseStats.current.dec();
phaseStats.total.inc();
phaseStats.timing.inc(TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - context.getCurrentPhase().getStartTimeInNanos()));
phaseStats.timing.inc(TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - phase.getStartTimeInNanos()));
}

@Override
public void onPhaseFailure(SearchPhaseContext context) {
phaseStatsMap.get(context.getCurrentPhase().getSearchPhaseName()).current.dec();
public void onPhaseFailure(SearchPhase phase) {
phaseStatsMap.get(phase.getSearchPhaseName()).current.dec();
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -352,18 +352,15 @@ SearchResponse.PhaseTook getPhaseTook() {
Map<SearchPhaseName, Long> phaseStatsMap = new EnumMap<>(SearchPhaseName.class);

@Override
public void onPhaseStart(SearchPhaseContext context) {}
public void onPhaseStart(SearchPhase phase) {}

@Override
public void onPhaseEnd(SearchPhaseContext context, SearchRequestContext searchRequestContext) {
phaseStatsMap.put(
context.getCurrentPhase().getSearchPhaseName(),
TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - context.getCurrentPhase().getStartTimeInNanos())
);
public void onPhaseEnd(SearchPhase phase, SearchRequestContext searchRequestContext) {
phaseStatsMap.put(phase.getSearchPhaseName(), TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - phase.getStartTimeInNanos()));
}

@Override
public void onPhaseFailure(SearchPhaseContext context) {}
public void onPhaseFailure(SearchPhase phase) {}

public Long getPhaseTookTime(SearchPhaseName searchPhaseName) {
return phaseStatsMap.get(searchPhaseName);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,19 +29,19 @@ public void testListenersAreExecuted() {
SearchRequestOperationsListener testListener = new SearchRequestOperationsListener() {

@Override
public void onPhaseStart(SearchPhaseContext context) {
searchPhaseMap.get(context.getCurrentPhase().getSearchPhaseName()).current.inc();
public void onPhaseStart(SearchPhase phase) {
searchPhaseMap.get(phase.getSearchPhaseName()).current.inc();
}

@Override
public void onPhaseEnd(SearchPhaseContext context, SearchRequestContext searchRequestContext) {
searchPhaseMap.get(context.getCurrentPhase().getSearchPhaseName()).current.dec();
searchPhaseMap.get(context.getCurrentPhase().getSearchPhaseName()).total.inc();
public void onPhaseEnd(SearchPhase phase, SearchRequestContext searchRequestContext) {
searchPhaseMap.get(phase.getSearchPhaseName()).current.dec();
searchPhaseMap.get(phase.getSearchPhaseName()).total.inc();
}

@Override
public void onPhaseFailure(SearchPhaseContext context) {
searchPhaseMap.get(context.getCurrentPhase().getSearchPhaseName()).current.dec();
public void onPhaseFailure(SearchPhase phase) {
searchPhaseMap.get(phase.getSearchPhaseName()).current.dec();
}
};

Expand All @@ -62,7 +62,7 @@ public void onPhaseFailure(SearchPhaseContext context) {
for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) {
when(ctx.getCurrentPhase()).thenReturn(searchPhase);
when(searchPhase.getSearchPhaseName()).thenReturn(searchPhaseName);
compositeListener.onPhaseStart(ctx);
compositeListener.onPhaseStart(ctx.getCurrentPhase());
assertEquals(totalListeners, searchPhaseMap.get(searchPhaseName).current.count());
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@ public void testSearchRequestPhaseFailure() {

for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) {
when(mockSearchPhase.getSearchPhaseName()).thenReturn(searchPhaseName);
testRequestStats.onPhaseStart(ctx);
testRequestStats.onPhaseStart(ctx.getCurrentPhase());
assertEquals(1, testRequestStats.getPhaseCurrent(searchPhaseName));
testRequestStats.onPhaseFailure(ctx);
testRequestStats.onPhaseFailure(ctx.getCurrentPhase());
assertEquals(0, testRequestStats.getPhaseCurrent(searchPhaseName));
}
}
Expand All @@ -46,11 +46,11 @@ public void testSearchRequestStats() {
for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) {
when(mockSearchPhase.getSearchPhaseName()).thenReturn(searchPhaseName);
long tookTimeInMillis = randomIntBetween(1, 10);
testRequestStats.onPhaseStart(ctx);
testRequestStats.onPhaseStart(ctx.getCurrentPhase());
long startTime = System.nanoTime() - TimeUnit.MILLISECONDS.toNanos(tookTimeInMillis);
when(mockSearchPhase.getStartTimeInNanos()).thenReturn(startTime);
assertEquals(1, testRequestStats.getPhaseCurrent(searchPhaseName));
testRequestStats.onPhaseEnd(ctx, new SearchRequestContext());
testRequestStats.onPhaseEnd(ctx.getCurrentPhase(), new SearchRequestContext());
assertEquals(0, testRequestStats.getPhaseCurrent(searchPhaseName));
assertEquals(1, testRequestStats.getPhaseTotal(searchPhaseName));
assertThat(testRequestStats.getPhaseMetric(searchPhaseName), greaterThanOrEqualTo(tookTimeInMillis));
Expand All @@ -71,7 +71,7 @@ public void testSearchRequestStatsOnPhaseStartConcurrently() throws InterruptedE
for (int i = 0; i < numTasks; i++) {
threads[i] = new Thread(() -> {
phaser.arriveAndAwaitAdvance();
testRequestStats.onPhaseStart(ctx);
testRequestStats.onPhaseStart(ctx.getCurrentPhase());
countDownLatch.countDown();
});
threads[i].start();
Expand Down Expand Up @@ -102,7 +102,7 @@ public void testSearchRequestStatsOnPhaseEndConcurrently() throws InterruptedExc
for (int i = 0; i < numTasks; i++) {
threads[i] = new Thread(() -> {
phaser.arriveAndAwaitAdvance();
testRequestStats.onPhaseEnd(ctx, new SearchRequestContext());
testRequestStats.onPhaseEnd(ctx.getCurrentPhase(), new SearchRequestContext());
countDownLatch.countDown();
});
threads[i].start();
Expand Down Expand Up @@ -134,8 +134,8 @@ public void testSearchRequestStatsOnPhaseFailureConcurrently() throws Interrupte
for (int i = 0; i < numTasks; i++) {
threads[i] = new Thread(() -> {
phaser.arriveAndAwaitAdvance();
testRequestStats.onPhaseStart(ctx);
testRequestStats.onPhaseFailure(ctx);
testRequestStats.onPhaseStart(ctx.getCurrentPhase());
testRequestStats.onPhaseFailure(ctx.getCurrentPhase());
countDownLatch.countDown();
});
threads[i].start();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@ public void testSearchTimeProviderPhaseFailure() {

for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) {
when(mockSearchPhase.getSearchPhaseName()).thenReturn(searchPhaseName);
testTimeProvider.onPhaseStart(ctx);
testTimeProvider.onPhaseStart(ctx.getCurrentPhase());
assertNull(testTimeProvider.getPhaseTookTime(searchPhaseName));
testTimeProvider.onPhaseFailure(ctx);
testTimeProvider.onPhaseFailure(ctx.getCurrentPhase());
assertNull(testTimeProvider.getPhaseTookTime(searchPhaseName));
}
}
Expand All @@ -43,11 +43,11 @@ public void testSearchTimeProviderPhaseEnd() {
for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) {
when(mockSearchPhase.getSearchPhaseName()).thenReturn(searchPhaseName);
long tookTimeInMillis = randomIntBetween(1, 100);
testTimeProvider.onPhaseStart(ctx);
testTimeProvider.onPhaseStart(ctx.getCurrentPhase());
long startTime = System.nanoTime() - TimeUnit.MILLISECONDS.toNanos(tookTimeInMillis);
when(mockSearchPhase.getStartTimeInNanos()).thenReturn(startTime);
assertNull(testTimeProvider.getPhaseTookTime(searchPhaseName));
testTimeProvider.onPhaseEnd(ctx, new SearchRequestContext());
testTimeProvider.onPhaseEnd(ctx.getCurrentPhase(), new SearchRequestContext());
assertThat(testTimeProvider.getPhaseTookTime(searchPhaseName), greaterThanOrEqualTo(tookTimeInMillis));
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@
import org.opensearch.action.search.SearchPhase;
import org.opensearch.action.search.SearchPhaseContext;
import org.opensearch.action.search.SearchPhaseName;
import org.opensearch.action.search.SearchRequestContext;
import org.opensearch.action.search.SearchRequestStats;
import org.opensearch.index.search.stats.SearchStats.Stats;
import org.opensearch.test.OpenSearchTestCase;
Expand Down Expand Up @@ -85,8 +84,8 @@ public void testShardLevelSearchGroupStats() throws Exception {
when(mockSearchPhase.getStartTimeInNanos()).thenReturn(System.nanoTime() - TimeUnit.SECONDS.toNanos(paramValue));
when(mockSearchPhase.getSearchPhaseName()).thenReturn(searchPhaseName);
for (int iterator = 0; iterator < paramValue; iterator++) {
testRequestStats.onPhaseStart(ctx);
testRequestStats.onPhaseEnd(ctx, new SearchRequestContext());
testRequestStats.onPhaseStart(ctx.getCurrentPhase());
testRequestStats.onPhaseEnd(ctx.getCurrentPhase(), null);
}
}
searchStats1.setSearchRequestStats(testRequestStats);
Expand Down

0 comments on commit f5ce904

Please sign in to comment.