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 <[email protected]>
  • Loading branch information
@reta
reta committed Nov 16, 2023
1 parent 37ec313 commit 07623b3
Show file tree
Hide file tree
Showing 10 changed files with 57 additions and 58 deletions.
6 changes: 3 additions & 3 deletions server/src/main/java/org/opensearch/action/search/SearchPhaseContext.java
View file
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
8 changes: 5 additions & 3 deletions server/src/main/java/org/opensearch/action/search/SearchRequestContext.java
View file
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
26 changes: 13 additions & 13 deletions server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListener.java
View file
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
6 changes: 3 additions & 3 deletions server/src/main/java/org/opensearch/action/search/SearchRequestSlowLog.java
View file
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
14 changes: 7 additions & 7 deletions server/src/main/java/org/opensearch/action/search/SearchRequestStats.java
View file
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
11 changes: 4 additions & 7 deletions server/src/main/java/org/opensearch/action/search/TransportSearchAction.java
View file
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
16 changes: 8 additions & 8 deletions server/src/test/java/org/opensearch/action/search/SearchRequestOperationsListenerTests.java
View file
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
16 changes: 8 additions & 8 deletions server/src/test/java/org/opensearch/action/search/SearchRequestStatsTests.java
View file
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
8 changes: 4 additions & 4 deletions server/src/test/java/org/opensearch/action/search/SearchTimeProviderTests.java
View file
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
4 changes: 2 additions & 2 deletions server/src/test/java/org/opensearch/index/search/stats/SearchStatsTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,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(), new SearchRequestContext());
}
}
searchStats1.setSearchRequestStats(testRequestStats);
Expand Down

0 comments on commit 07623b3

Please sign in to comment.