Skip to content

Commit

Permalink
remove getLoadStatsStore method from LocalityStore
Browse files Browse the repository at this point in the history
Currently `LoadStatsStore` is create by `LocalityStore` and `LoadReportClient` retrieves `LoadStatsStore` from `LocalityStore.getLoadStatsStore()`.

But `LocalityStore` is create by EDS policy, whereas `LoadReportClient` and `LoadStatsStore` should be created by CDS (if not EDS-only), before `LocalityStore` is created. If `LoadReportClient` is embedded in `XdsClientImpl`, it need a `LoadStatsStore` which shouldn't be created by `LocalityStore`.

Instead, `LoadStatsStore` should be create before `LocalityStore` is created, and be passed to `LocalityStore`'s constructor. A getter is not needed.
  • Loading branch information
dapengzhang0 authored and ericgribkoff committed Dec 6, 2019
1 parent bf37767 commit 24e731a
Show file tree
Hide file tree
Showing 3 changed files with 14 additions and 17 deletions.
20 changes: 8 additions & 12 deletions xds/src/main/java/io/grpc/xds/LocalityStore.java
Expand Up @@ -76,23 +76,23 @@ interface LocalityStore {

void updateOobMetricsReportInterval(long reportIntervalNano);

LoadStatsStore getLoadStatsStore();

@VisibleForTesting
abstract class LocalityStoreFactory {
private static final LocalityStoreFactory DEFAULT_INSTANCE =
new LocalityStoreFactory() {
@Override
LocalityStore newLocalityStore(Helper helper, LoadBalancerRegistry lbRegistry) {
return new LocalityStoreImpl(helper, lbRegistry);
LocalityStore newLocalityStore(
Helper helper, LoadBalancerRegistry lbRegistry, LoadStatsStore loadStatsStore) {
return new LocalityStoreImpl(helper, lbRegistry, loadStatsStore);
}
};

static LocalityStoreFactory getInstance() {
return DEFAULT_INSTANCE;
}

abstract LocalityStore newLocalityStore(Helper helper, LoadBalancerRegistry lbRegistry);
abstract LocalityStore newLocalityStore(
Helper helper, LoadBalancerRegistry lbRegistry, LoadStatsStore loadStatsStore);
}

final class LocalityStoreImpl implements LocalityStore {
Expand All @@ -113,9 +113,10 @@ final class LocalityStoreImpl implements LocalityStore {
private List<DropOverload> dropOverloads = ImmutableList.of();
private long metricsReportIntervalNano = -1;

LocalityStoreImpl(Helper helper, LoadBalancerRegistry lbRegistry) {
LocalityStoreImpl(
Helper helper, LoadBalancerRegistry lbRegistry, LoadStatsStore loadStatsStore) {
this(helper, pickerFactoryImpl, lbRegistry, ThreadSafeRandom.ThreadSafeRandomImpl.instance,
new LoadStatsStoreImpl(), OrcaPerRequestUtil.getInstance(), OrcaOobUtil.getInstance());
loadStatsStore, OrcaPerRequestUtil.getInstance(), OrcaOobUtil.getInstance());
}

@VisibleForTesting
Expand Down Expand Up @@ -284,11 +285,6 @@ public String toString() {
TimeUnit.MINUTES, helper.getScheduledExecutorService());
}

@Override
public LoadStatsStore getLoadStatsStore() {
return loadStatsStore;
}

@Override
public void updateOobMetricsReportInterval(long reportIntervalNano) {
metricsReportIntervalNano = reportIntervalNano;
Expand Down
6 changes: 4 additions & 2 deletions xds/src/main/java/io/grpc/xds/LookasideLb.java
Expand Up @@ -175,11 +175,13 @@ public void handleResolvedAddresses(ResolvedAddresses resolvedAddresses) {
xdsClient = new XdsComms2(
channel, helper, new ExponentialBackoffPolicy.Provider(),
GrpcUtil.STOPWATCH_SUPPLIER, node);
localityStore = localityStoreFactory.newLocalityStore(helper, lbRegistry);
LoadStatsStore loadStatsStore = new LoadStatsStoreImpl();
localityStore = localityStoreFactory.newLocalityStore(
helper, lbRegistry, loadStatsStore);
// TODO(zdapeng): Use XdsClient to do Lrs directly.
lrsClient = loadReportClientFactory.createLoadReportClient(
channel, helper, new ExponentialBackoffPolicy.Provider(),
localityStore.getLoadStatsStore());
loadStatsStore);
final LoadReportCallback lrsCallback =
new LoadReportCallback() {
@Override
Expand Down
5 changes: 2 additions & 3 deletions xds/src/test/java/io/grpc/xds/LookasideLbTest.java
Expand Up @@ -194,11 +194,10 @@ public void onCompleted() {

localityStoreFactory = new LocalityStoreFactory() {
@Override
public LocalityStore newLocalityStore(Helper helper, LoadBalancerRegistry lbRegistry) {
public LocalityStore newLocalityStore(
Helper helper, LoadBalancerRegistry lbRegistry, LoadStatsStore loadStatsStore) {
helpers.add(helper);
LocalityStore localityStore = mock(LocalityStore.class);
LoadStatsStore loadStatsStore = mock(LoadStatsStore.class);
doReturn(loadStatsStore).when(localityStore).getLoadStatsStore();
localityStores.add(localityStore);
return localityStore;
}
Expand Down

0 comments on commit 24e731a

Please sign in to comment.