Skip to content

Commit

Permalink
remove some dual read related tests and deprecate dual read LB (#1014)
Browse files Browse the repository at this point in the history
  • Loading branch information
bohhyang authored Aug 9, 2024
1 parent cd385ff commit 3227d01
Show file tree
Hide file tree
Showing 4 changed files with 19 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
* In DUAL_READ mode, it reads from both the old and the new load balancer, but relies on the data from old
* load balancer only.
*/
@Deprecated
public class DualReadLoadBalancer implements LoadBalancerWithFacilities
{
private static final Logger LOG = LoggerFactory.getLogger(DualReadLoadBalancer.class);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -398,7 +398,7 @@ private static boolean isModeToWarmUp(DualReadModeProvider.DualReadMode mode, bo
public void shutdown(PropertyEventThread.PropertyEventShutdownCallback shutdown)
{
// avoid cleaning when you risk to have partial results since some of the services have not loaded yet
if (_outstandingRequests.size() == 0)
if (completedOutStandingRequests())
{
// cleanup from unused services
FileSystemDirectory fsDirectory = new FileSystemDirectory(_d2FsDirPath, _d2ServicePath);
Expand All @@ -412,6 +412,11 @@ public void shutdown(PropertyEventThread.PropertyEventShutdownCallback shutdown)
_loadBalancer.shutdown(shutdown);
}

boolean completedOutStandingRequests()
{
return _outstandingRequests.isEmpty();
}

private Set<String> getUsedClusters()
{
Set<String> usedClusters = new HashSet<>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
* discovery data sources: direct ZooKeeper data and xDS data. The {@link DualReadModeProvider} will
* determine dynamically at run-time which read mode to use.
*/
@Deprecated
public class DualReadZkAndXdsLoadBalancerFactory implements LoadBalancerWithFacilitiesFactory
{
private final LoadBalancerWithFacilitiesFactory _zkLbFactory;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import org.testng.annotations.AfterMethod;
import org.testng.annotations.BeforeMethod;
import org.testng.annotations.DataProvider;
import org.testng.annotations.Ignore;
import org.testng.annotations.Test;

import static com.linkedin.d2.balancer.dualread.DualReadModeProvider.DualReadMode.*;
Expand Down Expand Up @@ -127,7 +128,7 @@ public void testMakingWarmUpRequests() throws URISyntaxException, InterruptedExc
Assert.assertEquals(VALID_FILES.size(), requestCount.get());
}

@Test(timeOut = 10000, retryAnalyzer = ThreeRetries.class)
@Test(timeOut = 10_000)
public void testDeletingFilesAfterShutdown() throws InterruptedException, ExecutionException, TimeoutException
{
createDefaultServicesIniFiles();
Expand All @@ -138,7 +139,7 @@ public void testDeletingFilesAfterShutdown() throws InterruptedException, Execut

DownstreamServicesFetcher returnPartialDownstreams = callback -> callback.onSuccess(partialServices);

LoadBalancer warmUpLoadBalancer = new WarmUpLoadBalancer(balancer, balancer, Executors.newSingleThreadScheduledExecutor(),
WarmUpLoadBalancer warmUpLoadBalancer = new WarmUpLoadBalancer(balancer, balancer, Executors.newSingleThreadScheduledExecutor(),
_tmpdir.getAbsolutePath(), MY_SERVICES_FS, returnPartialDownstreams,
WarmUpLoadBalancer.DEFAULT_SEND_REQUESTS_TIMEOUT_SECONDS,
WarmUpLoadBalancer.DEFAULT_CONCURRENT_REQUESTS);
Expand All @@ -157,9 +158,11 @@ public void testDeletingFilesAfterShutdown() throws InterruptedException, Execut
"After shutdown the unused services should have been deleted. Expected lower number of:" + allServicesBeforeShutdown.size()
+ ", actual " + partialServices.size());

Assert.assertTrue(partialServices.containsAll(allServicesAfterShutdown)
&& allServicesAfterShutdown.containsAll(partialServices),
"There should be just the services that were passed by the partial fetcher");
if (warmUpLoadBalancer.completedOutStandingRequests()) {
Assert.assertTrue(partialServices.containsAll(allServicesAfterShutdown)
&& allServicesAfterShutdown.containsAll(partialServices),
"There should be just the services that were passed by the partial fetcher");
}
}

/**
Expand Down Expand Up @@ -373,7 +376,7 @@ public Object[][] modesToWarmUpDataProvider()
};
}

@Test(dataProvider = "modesToWarmUpDataProvider", retryAnalyzer = ThreeRetries.class)
@Ignore("dual read is only used in INDIS migration phase and should be deprecated")
public void testSuccessWithDualRead(DualReadModeProvider.DualReadMode mode, Boolean isIndis)
throws InterruptedException, ExecutionException, TimeoutException
{
Expand All @@ -399,7 +402,7 @@ public void testSuccessWithDualRead(DualReadModeProvider.DualReadMode mode, Bool
Assert.assertEquals(completedWarmUpCount.get(), VALID_FILES.size());
}

@Test(dataProvider = "modesToWarmUpDataProvider", retryAnalyzer = ThreeRetries.class)
@Ignore("dual read is only used in INDIS migration phase and should be deprecated")
public void testDualReadHitTimeout(DualReadModeProvider.DualReadMode mode, Boolean isIndis)
throws InterruptedException, ExecutionException, TimeoutException
{
Expand All @@ -425,7 +428,7 @@ public void testDualReadHitTimeout(DualReadModeProvider.DualReadMode mode, Boole
Assert.assertEquals(completedWarmUpCount.get(), 0);
}

@Test(dataProvider = "modesToWarmUpDataProvider", retryAnalyzer = ThreeRetries.class)
@Ignore("dual read is only used in INDIS migration phase and should be deprecated")
public void testDualReadCompleteWarmUpHitTimeout(DualReadModeProvider.DualReadMode mode, Boolean isIndis)
throws InterruptedException, ExecutionException, TimeoutException
{
Expand Down

0 comments on commit 3227d01

Please sign in to comment.