Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fix] Make UserAgent's otherInfo thread-safe #357

Merged
merged 6 commits into from
Oct 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,9 @@ public static void withPartner(String partner) {
public static void withOtherInfo(String key, String value) {
matchAlphanum(key);
matchAlphanumOrSemVer(value);
otherInfo.add(new Info(key, value));
synchronized (otherInfo) {
otherInfo.add(new Info(key, value));
}
}

private static String osName() {
Expand All @@ -119,10 +121,13 @@ public static String asString() {
segments.add(String.format("databricks-sdk-java/%s", version));
segments.add(String.format("jvm/%s", jvmVersion()));
segments.add(String.format("os/%s", osName()));
segments.addAll(
otherInfo.stream()
.map(e -> String.format("%s/%s", e.getKey(), e.getValue()))
.collect(Collectors.toSet()));
// Concurrent iteration over ArrayList must be guarded with synchronized.
synchronized (otherInfo) {
segments.addAll(
otherInfo.stream()
.map(e -> String.format("%s/%s", e.getKey(), e.getValue()))
.collect(Collectors.toSet()));
}
return segments.stream().collect(Collectors.joining(" "));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,35 @@

import static org.junit.jupiter.api.Assertions.*;

import com.databricks.sdk.core.DatabricksConfig;
import com.databricks.sdk.core.commons.CommonsHttpClient;
import com.databricks.sdk.integration.framework.EnvContext;
import com.databricks.sdk.integration.framework.EnvOrSkip;
import com.databricks.sdk.integration.framework.EnvTest;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.*;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;

/*
This test executes the authentication workflow 200 times concurrently and verifies that all 200 runs complete successfully.
It is designed to address a previously observed issue where multiple SDK operations needed to authenticate, causing the OIDC endpoints to rate-limit the requests.
Now that these endpoints are configured to retry upon receiving a 429 error, the test runs successfully.
However, since this test generates a large number of requests, it should be run manually rather than being included in CI processes.
*/
/*
public class DatabricksAuthLoadTest implements GitHubUtils, ConfigResolving {
/**
* This test executes the authentication workflow 200 times concurrently and verifies that all 200
* runs complete successfully. It is designed to address a previously observed issue where multiple
* SDK operations needed to authenticate, causing the OIDC endpoints to rate-limit the requests. Now
* that these endpoints are configured to retry upon receiving a 429 error, the test runs
* successfully. However, since this test generates a large number of requests, it should be run
* manually rather than being included in CI processes.
*/
@EnvContext("workspace")
@ExtendWith(EnvTest.class)
public class DatabricksAuthLoadIT {

@Test
@Disabled
public void testConcurrentConfigBasicAuthAttrs() throws Exception {
public void testConcurrentConfigBasicAuthAttrs(
@EnvOrSkip("DATABRICKS_HOST") String host,
@EnvOrSkip("DATABRICKS_CLIENT_ID") String clientId,
@EnvOrSkip("DATABRICKS_CLIENT_SECRET") String clientSecret)
throws Exception {
Comment on lines +29 to +33
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better! :)

int numThreads = 200;
ExecutorService executorService = Executors.newFixedThreadPool(numThreads);
List<Future<Boolean>> futures = new ArrayList<>();
Expand All @@ -27,9 +42,9 @@ public void testConcurrentConfigBasicAuthAttrs() throws Exception {
try {
DatabricksConfig config =
new DatabricksConfig()
.setHost("https://dbc-bb03964f-3f59.cloud.databricks.com")
.setClientId("<<REDACTED>>")
.setClientSecret("<<REDACTED>>");
.setHost(host)
.setClientId(clientId)
.setClientSecret(clientSecret);

config.setHttpClient(new CommonsHttpClient.Builder().withTimeoutSeconds(30).build());
config.authenticate();
Expand Down Expand Up @@ -70,4 +85,3 @@ public void testConcurrentConfigBasicAuthAttrs() throws Exception {
assertEquals(0, failureCount);
}
}
*/
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# Benchmark Tests

The tests defined in this module are benchmarks of the Databricks SDK and its internal components. These tests ensure that the SDK works correctly and with reasonable performance under high load and concurrency. These tests are not run in normal CI builds to prevent regularly exhausting the REST API rate limit.

Load tests of components that don't make any network requests (e.g. `UserAgentLoadTest`) should be colocated with the unit tests for that component.

## Adding a Benchmark Test

All test files in this module should be named `*LoadIT.java`. They should be written as integration tests, with the `@EnvContext()` and `@ExtendWith(EnvTest.class)` annotations.

## Running the Benchmarks

Use [IntelliJ's built-in test runner](https://www.jetbrains.com/help/idea/performing-tests.html) to run benchmark tests.
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
package com.databricks.sdk.benchmark;

import static org.junit.jupiter.api.Assertions.assertEquals;

import com.databricks.sdk.WorkspaceClient;
import com.databricks.sdk.core.DatabricksConfig;
import com.databricks.sdk.integration.framework.EnvContext;
import com.databricks.sdk.integration.framework.EnvOrSkip;
import com.databricks.sdk.integration.framework.EnvTest;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.*;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;

/**
* This test executes a simple operation 150 times concurrently to verify that the end-to-end
* behavior of the SDK is correct under concurrent load. This test is designed to be run manually
* rather than being included in CI processes, as it generates a large number of requests.
*/
@EnvContext("workspace")
@ExtendWith(EnvTest.class)
public class WorkspaceClientLoadIT {

@Test
public void testConcurrentCurrentUserMe(
@EnvOrSkip("DATABRICKS_HOST") String host,
@EnvOrSkip("DATABRICKS_CLIENT_ID") String clientId,
@EnvOrSkip("DATABRICKS_CLIENT_SECRET") String clientSecret)
throws Exception {
int numThreads = 150;
ExecutorService executorService = Executors.newFixedThreadPool(numThreads);
List<Future<Boolean>> futures = new ArrayList<>();
int successCount = 0;
int failureCount = 0;

Callable<Boolean> task =
() -> {
try {
DatabricksConfig config =
new DatabricksConfig()
.setHost(host)
.setClientId(clientId)
.setClientSecret(clientSecret);

WorkspaceClient w = new WorkspaceClient(config);

// This should not throw an exception.
w.currentUser().me();

return true;
} catch (Exception e) {
System.err.println(
"DatabricksException occurred in thread " + Thread.currentThread().getName());
e.printStackTrace();
return false;
}
};

for (int i = 0; i < numThreads; i++) {
futures.add(executorService.submit(task));
}

executorService.shutdown();
if (!executorService.awaitTermination(60, TimeUnit.SECONDS)) {
executorService.shutdownNow();
}

for (Future<Boolean> future : futures) {
if (future.get()) {
successCount++;
} else {
failureCount++;
}
}

// Log the results
System.out.println("Number of successful threads: " + successCount);
System.out.println("Number of failed threads: " + failureCount);

// Optionally, you can assert that there were no failures
assertEquals(0, failureCount);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
package com.databricks.sdk.core;

import static org.junit.jupiter.api.Assertions.assertEquals;

import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.*;
import org.junit.jupiter.api.Test;

/**
* Load tests for the UserAgent class.
*
* <p>This is not part of the benchmark module because it doesn't make any network requests.
*/
public class UserAgentLoadTest {

@Test
public void testAsStringConcurrent() throws InterruptedException, ExecutionException {
int numThreads = 200;
ExecutorService executorService = Executors.newFixedThreadPool(numThreads);
List<Future<Boolean>> futures = new ArrayList<>();
int successCount = 0;
int failureCount = 0;

// Add some user agent info
Callable<Boolean> task =
() -> {
try {
UserAgent.withOtherInfo("key1", "value1");
UserAgent.asString();
return true;
} catch (Exception e) {
System.err.println(
"DatabricksException occurred in thread " + Thread.currentThread().getName());
e.printStackTrace();
return false;
}
};

for (int i = 0; i < numThreads; i++) {
futures.add(executorService.submit(task));
}

executorService.shutdown();
if (!executorService.awaitTermination(60, TimeUnit.SECONDS)) {
executorService.shutdownNow();
}

for (Future<Boolean> future : futures) {
if (future.get()) {
successCount++;
} else {
failureCount++;
}
}

// Log the results
System.out.println("Number of successful threads: " + successCount);
System.out.println("Number of failed threads: " + failureCount);

// Optionally, you can assert that there were no failures
assertEquals(0, failureCount);
}
}
21 changes: 21 additions & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -103,10 +103,31 @@
</pom>
</configuration>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-surefire-plugin</artifactId>
<version>3.1.2</version>
<configuration>
<!--
Benchmarks are disabled in normal tests. See
databricks-sdk-java/src/test/java/com/databricks/sdk/benchmark/README.md for more information.
-->
<excludes>
<exclude>**/benchmark/**</exclude>
</excludes>
</configuration>
</plugin>
<plugin>
<artifactId>maven-failsafe-plugin</artifactId>
<version>3.2.5</version>
<configuration>
<!--
Benchmarks are disabled in normal tests. See
databricks-sdk-java/src/test/java/com/databricks/sdk/benchmark/README.md for more information.
-->
<excludes>
<exclude>**/benchmark/**</exclude>
</excludes>
<rerunFailingTestsCount>2</rerunFailingTestsCount>
</configuration>
<executions>
Expand Down
Loading