Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
5 changes: 5 additions & 0 deletions dbbackuprestore/dbbackuprestore-aws/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,11 @@
<type>test-jar</type>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.openjdk.jmh</groupId>
<artifactId>jmh-generator-annprocess</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>com.salesforce.multicloudj</groupId>
<artifactId>multicloudj-common</artifactId>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
package com.salesforce.multicloudj.dbbackuprestore.aws;

import com.salesforce.multicloudj.common.util.UUID;
import com.salesforce.multicloudj.dbbackuprestore.client.AbstractDBBackupRestoreBenchmarkTest;
import com.salesforce.multicloudj.dbbackuprestore.client.DBBackupRestoreClient;
import org.junit.jupiter.api.TestInstance;
import software.amazon.awssdk.regions.Region;
import software.amazon.awssdk.services.backup.BackupClient;

/**
* Hits the real AWS Backup endpoint directly, so it requires live AWS credentials via the default
* provider chain (OS env / profile) and network access.
*/
@TestInstance(TestInstance.Lifecycle.PER_CLASS)
public class AwsDBBackupRestoreBenchmarkTest extends AbstractDBBackupRestoreBenchmarkTest {

@Override
protected Harness createHarness() {
return new HarnessImpl();
}

@Override
protected String getProviderId() {
return "aws";
}

public static class HarnessImpl implements Harness {

private final String region = requireEnv("DBBACKUPRESTORE_BENCHMARK_AWS_REGION");
private final String tableArn = requireEnv("DBBACKUPRESTORE_BENCHMARK_AWS_TABLE_ARN");
private final String roleId = requireEnv("DBBACKUPRESTORE_BENCHMARK_AWS_ROLE_ID");
private BackupClient backupClient;

@Override
public DBBackupRestoreClient createClient() {
// Credentials flow via the AWS default provider chain (OS env / profile), never -D.
backupClient = BackupClient.builder().region(Region.of(region)).build();

AwsDBBackupRestore dbBackupRestore =
new AwsDBBackupRestore.Builder()
.withBackupClient(backupClient)
.withRegion(region)
.withResourceName(tableArn)
.build();

return new DBBackupRestoreClient(dbBackupRestore);
}

@Override
public String getTargetResource() {
return "restored-table-benchmark-" + UUID.uniqueString();
}

@Override
public String getRoleId() {
return roleId;
}

@Override
public void close() {
if (backupClient != null) {
backupClient.close();
}
}
}
}
12 changes: 12 additions & 0 deletions dbbackuprestore/dbbackuprestore-client/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,18 @@
<type>test-jar</type>
<scope>test</scope>
</dependency>

<!-- JMH Dependencies for Benchmarking -->
<dependency>
<groupId>org.openjdk.jmh</groupId>
<artifactId>jmh-core</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.openjdk.jmh</groupId>
<artifactId>jmh-generator-annprocess</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.projectlombok</groupId>
<artifactId>lombok</artifactId>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,263 @@
package com.salesforce.multicloudj.dbbackuprestore.client;

import com.salesforce.multicloudj.dbbackuprestore.driver.Backup;
import com.salesforce.multicloudj.dbbackuprestore.driver.Restore;
import com.salesforce.multicloudj.dbbackuprestore.driver.RestoreRequest;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.TimeUnit;
import org.apache.commons.lang3.StringUtils;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.TestInstance;
import org.junit.jupiter.api.condition.EnabledIfSystemProperty;
import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.BenchmarkMode;
import org.openjdk.jmh.annotations.Fork;
import org.openjdk.jmh.annotations.Level;
import org.openjdk.jmh.annotations.Measurement;
import org.openjdk.jmh.annotations.Mode;
import org.openjdk.jmh.annotations.OutputTimeUnit;
import org.openjdk.jmh.annotations.Scope;
import org.openjdk.jmh.annotations.Setup;
import org.openjdk.jmh.annotations.State;
import org.openjdk.jmh.annotations.TearDown;
import org.openjdk.jmh.annotations.Warmup;
import org.openjdk.jmh.infra.Blackhole;
import org.openjdk.jmh.results.format.ResultFormatType;
import org.openjdk.jmh.runner.Runner;
import org.openjdk.jmh.runner.RunnerException;
import org.openjdk.jmh.runner.options.Options;
import org.openjdk.jmh.runner.options.OptionsBuilder;

/**
* JMH benchmarks for DB backup/restore control-plane operations via {@link DBBackupRestoreClient}.
*
* <p>Backup/restore admin APIs are latency-primary and low-QPS: throughput mostly reflects the
* provider's rate limiter, so {@code SampleTime} percentiles are the signal and {@code Throughput}
* is trend-only.
*
* <p>Restore is kickoff-only: {@code restoreBackup} initiates an async job and there is no delete
* API, so a restored table/database is left behind permanently. {@code benchmarkRestoreKickoff}
* must never run in a warmup/measurement loop; only the read-only methods are safe to sweep.
*/
@BenchmarkMode({Mode.Throughput, Mode.SampleTime})
@OutputTimeUnit(TimeUnit.SECONDS)
@State(Scope.Benchmark)
@Warmup(iterations = 3, time = 2, timeUnit = TimeUnit.SECONDS)
@Measurement(iterations = 5, time = 3, timeUnit = TimeUnit.SECONDS)
@Fork(1)
@TestInstance(TestInstance.Lifecycle.PER_CLASS)
public abstract class AbstractDBBackupRestoreBenchmarkTest {

protected DBBackupRestoreClient client;
protected String backupId;
protected String restoreId;

// Harness interface
public interface Harness extends AutoCloseable {
DBBackupRestoreClient createClient();

String getTargetResource();

default String getKnownBackupId() {
return null;
}

/**
* A statically-known restore ID to exercise {@link #benchmarkGetRestoreJob(Blackhole)}. When
* unset, setup does NOT create one (that would spawn an uncleaned-up restore job on every
* trial, since {@code @Setup(Level.Trial)} runs once per fork per benchmark method and there is
* no delete API); the get-restore-job benchmark simply skips itself.
*/
default String getKnownRestoreId() {
return null;
}

default String getRoleId() {
return null;
}

default String getVaultId() {
return null;
}

default String getKmsEncryptionKeyId() {
return null;
}
}

protected Harness harness;

protected abstract Harness createHarness();

protected abstract String getProviderId();

/**
* Reads a required config value from OS environment first, then from -D system properties.
* Fails fast with a clear error if neither is set.
*/
protected static String requireEnv(String name) {
String value = System.getenv(name);
if (StringUtils.isBlank(value)) {
value = System.getProperty(name);
}
if (StringUtils.isBlank(value)) {
throw new IllegalStateException("Required environment variable not set: " + name);
}
return value;
}

/**
* Reads an optional config value from OS environment first, then from -D system properties.
* Returns null when unset (used for the optional role/vault/KMS/restore-id inputs).
*/
protected static String optionalEnv(String name) {
String value = System.getenv(name);
if (StringUtils.isBlank(value)) {
value = System.getProperty(name);
}
return StringUtils.isBlank(value) ? null : value;
}

@Setup(Level.Trial)
public void setupBenchmark() throws Exception {
try {
harness = createHarness();
client = harness.createClient();

backupId = harness.getKnownBackupId();
if (backupId == null || backupId.isEmpty()) {
List<Backup> backups = client.listBackups();
if (backups.isEmpty()) {
throw new IllegalStateException(
"No backups available for resource; cannot set up DBBackupRestore benchmark");
}
backupId = backups.get(0).getBackupId();
}

// Intentionally do NOT kick off a real restore here. setup runs once per fork per benchmark
// method, so a fallback restore would spawn an uncleaned-up restore job on every trial (there
// is no delete API). benchmarkGetRestoreJob null-guards itself when no known restore ID is
// provided; supply one via Harness.getKnownRestoreId() to exercise that path.
restoreId = harness.getKnownRestoreId();
} catch (Exception e) {
throw new RuntimeException("Failed to setup benchmark", e);
}
}

// DBBackupRestoreClient exposes no delete API, so any restored table/database created by
// benchmarkRestoreKickoff or setupBenchmark's fallback restore is left behind permanently.
@TearDown(Level.Trial)
public void teardownBenchmark() throws Exception {
try {
if (client != null) {
client.close();
}

if (harness != null) {
harness.close();
}
} catch (Exception e) {
throw new RuntimeException("Error closing harness", e);
}
}

private RestoreRequest buildRestoreRequest() {
RestoreRequest.RestoreRequestBuilder requestBuilder =
RestoreRequest.builder().backupId(backupId).targetResource(harness.getTargetResource());

if (harness.getRoleId() != null) {
requestBuilder.roleId(harness.getRoleId());
}
if (harness.getVaultId() != null) {
requestBuilder.vaultId(harness.getVaultId());
}
if (harness.getKmsEncryptionKeyId() != null) {
requestBuilder.kmsEncryptionKeyId(harness.getKmsEncryptionKeyId());
}

return requestBuilder.build();
}

/** List Backups - list + pagination, the most common admin read. */
@Benchmark
public void benchmarkListBackups(Blackhole bh) {
try {
List<Backup> backups = client.listBackups();
bh.consume(backups);
} catch (Exception e) {
throw new RuntimeException("Benchmark list backups failed", e);
}
}

/** Get Backup - single metadata fetch. */
@Benchmark
public void benchmarkGetBackup(Blackhole bh) {
try {
Backup backup = client.getBackup(backupId);
bh.consume(backup);
} catch (Exception e) {
throw new RuntimeException("Benchmark get backup failed", e);
}
}

/**
* Restore Kickoff. Measures the latency of the API call that INITIATES a restore, not the time
* the restore itself takes (restores run async on the provider side and can take minutes to
* hours). Excluded from the default sweep in {@link #runBenchmarks()} since each invocation
* leaves an uncleaned-up restore; kept as a {@code @Benchmark} so an operator with a disposable
* target can run it explicitly.
*/
@Benchmark
public void benchmarkRestoreKickoff(Blackhole bh) {
try {
String newRestoreId = client.restoreBackup(buildRestoreRequest());
bh.consume(newRestoreId);
} catch (Exception e) {
throw new RuntimeException("Benchmark restore kickoff failed", e);
}
}

/** Get Restore Job - the status-poll read that runs repeatedly during a restore. */
@Benchmark
public void benchmarkGetRestoreJob(Blackhole bh) {
// No restore ID is available unless the harness supplies one via getKnownRestoreId(); setup
// deliberately does not create one. Skip cheaply rather than NPE so the default read-only
// trial stays green.
if (restoreId == null || restoreId.isEmpty()) {
bh.consume(restoreId);
return;
}
try {
Restore restore = client.getRestoreJob(restoreId);
bh.consume(restore);
} catch (Exception e) {
throw new RuntimeException("Benchmark get restore job failed", e);
}
}

@Test
@EnabledIfSystemProperty(named = "runBenchmarks", matches = "true")
public void runBenchmarks() throws RunnerException {
List<String> forwardedArgs = new ArrayList<>();
for (String key : System.getProperties().stringPropertyNames()) {
if (key.startsWith("DBBACKUPRESTORE_BENCHMARK_")) {
forwardedArgs.add("-D" + key + "=" + System.getProperty(key));
}
}

// Exclude benchmarkRestoreKickoff from the swept set: each invocation kicks off a real,
// uncleaned-up restore, so it must never run in a warmup/measurement loop.
Options opt =
new OptionsBuilder()
.include(".*" + this.getClass().getName() + ".*")
.exclude(".*benchmarkRestoreKickoff.*")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

⚠️ A launcher-side regex is the only thing standing between a normal run and a storm of undeletable restores

benchmarkRestoreKickoff creates a real table or database on every invocation with a unique name (AwsDBBackupRestoreBenchmarkTest:51, GcpFirestoreDBBackupRestoreBenchmarkTest:54), and the driver contract exposes no delete — the comment at :148-149 concedes the resources are "left behind permanently". Its only guard is this .exclude(). But the PR body says the chameleon pipeline "overrides via its own BenchmarkRunner", and if that runner builds its own Options the exclusion doesn't exist there; the generated META-INF/BenchmarkList entry is unconditional. Over 21 s × 2 modes at control-plane latencies that's on the order of 100+ restore jobs, against a Firestore default of 100 databases per project. An in-method guard that fails unless something like DBBACKUPRESTORE_BENCHMARK_ALLOW_DESTRUCTIVE is set would be a sturdier fence than a regex the caller can replace.

The sibling benchmark at :227 has the inverse problem. benchmarkGetRestoreJob early-returns whenever restoreId is null, getKnownRestoreId() defaults to null (:72-74), and neither harness overrides it — and optionalEnv (:114), whose javadoc names "the optional role/vault/KMS/restore-id inputs", has zero call sites anywhere in the repo. So there is no configuration path to make it non-empty short of editing source, and it always times an empty method. Measured with JMH 1.37 under this suite's own class annotations, that publishes ~1.5 × 10⁹ ops/s (≈0.6 ns/op) in Throughput against 17.4 ops/s for a 50 ms control. Between this and the excluded kickoff, two of the four benchmarks in the suite measure nothing.

Both are the same underlying gap: capability should be a harness predicate that decides what goes into .exclude(), rather than a hardcoded regex on one side and a silent no-op on the other. Same ask as on #588.

Two smaller things while you're in here, which compound:

  • DBBACKUPRESTORE_BENCHMARK_GCP_LOCATION is handed straight to listBackups(parent) (FSDBBackupRestore.java:89), which needs the full projects/{p}/locations/{loc} path — FSDBBackupRestoreIT.java:52 shows the shape — but the run contract documents it as <location>. Following the documented contract literally gives INVALID_ARGUMENT inside @Setup.
  • The launcher never sets .failOnError(true), and JMH defaults it to false. So the IllegalStateException("No backups available…") at :132-134, or a setup failure from the item above, or every invocation throttling, all still exit 0 with a near-empty results file — I confirmed the behaviour by pointing an existing suite at a nonexistent bucket: 768 failed invocations, BUILD SUCCESS, and a 6-byte [] result. None of the eight suites in the repo sets it, so this one is worth fixing once across the template rather than here alone.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks — sharp catch on all counts. Addressed in the latest push:

Destructive kickoff: Added an in-method guard. benchmarkRestoreKickoff now hard-fails with IllegalStateException unless DBBACKUPRESTORE_BENCHMARK_ALLOW_DESTRUCTIVE=true, so it's fenced regardless of who builds the Options — the launcher .exclude() is now just the convenience layer, not the only guard.

getRestoreJob measuring nothing: getKnownRestoreId() now reads DBBACKUPRESTORE_BENCHMARK_RESTORE_ID via optionalEnv (giving optionalEnv its first real call site), so there's finally a config path. When no restore id is set, the launcher drops the benchmark from the sweep entirely rather than timing the empty-method no-op — so the default sweep is now exactly the two working read-only benchmarks.

Capability-driven exclusion: Both of the above are now decided by harness predicates (isDestructiveEnabled(), getKnownRestoreId()) at launch time rather than a fixed regex, which is the underlying refactor you asked for.

GCP LOCATION: Documented in the harness that the value is the full projects/{p}/locations/{loc} parent path, since it's passed straight to listBackups(parent). Fixed the run-contract wording in the PR body too.

failOnError: Deliberately holding off here. shouldFailOnError(true) aborts the whole run on the first error, so a single transient throttle would kill every remaining benchmark on a live-cloud suite — a bad trade for this workload. Agree it's worth solving centrally; since you noted it (and the capability-predicate pattern) spans all the suites and #588, I'd rather fix the empty-results-look-like-success problem once across the template than one-off it here. Happy to open a follow-up for that.

.forks(1)
.resultFormat(ResultFormatType.JSON)
.result("target/jmh-dbbackuprestore-results-" + getProviderId() + ".json")
.jvmArgsAppend(forwardedArgs.toArray(new String[0]))
.build();

new Runner(opt).run();
}
}
5 changes: 5 additions & 0 deletions dbbackuprestore/dbbackuprestore-gcp-firestore/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,11 @@
<type>test-jar</type>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.openjdk.jmh</groupId>
<artifactId>jmh-generator-annprocess</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>com.salesforce.multicloudj</groupId>
<artifactId>multicloudj-common</artifactId>
Expand Down
Loading
Loading