dbbackuprestore: add JMH benchmark suite - #591
Conversation
Adds an abstract AbstractDBBackupRestoreBenchmarkTest in dbbackuprestore-client plus thin AWS/GCP-Firestore concretes, gated by @EnabledIfSystemProperty(runBenchmarks=true). Swept: listBackups, getBackup, getRestoreJob (self-guards to a no-op unless a known restore id is supplied). restoreKickoff is kept but excluded from the sweep since each invocation kicks off a real restore with no delete API to clean it up. Story: https://gus.lightning.force.com/lightning/r/ADM_Work__c/a07EE00002hP986YAC/view (W-23830175)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #591 +/- ##
=========================================
Coverage 83.61% 83.61%
Complexity 674 674
=========================================
Files 215 215
Lines 15010 15010
Branches 2076 2076
=========================================
Hits 12550 12550
Misses 1636 1636
Partials 824 824
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| Options opt = | ||
| new OptionsBuilder() | ||
| .include(".*" + this.getClass().getName() + ".*") | ||
| .exclude(".*benchmarkRestoreKickoff.*") |
There was a problem hiding this comment.
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_LOCATIONis handed straight tolistBackups(parent)(FSDBBackupRestore.java:89), which needs the fullprojects/{p}/locations/{loc}path —FSDBBackupRestoreIT.java:52shows the shape — but the run contract documents it as<location>. Following the documented contract literally givesINVALID_ARGUMENTinside@Setup.- The launcher never sets
.failOnError(true), and JMH defaults it tofalse. So theIllegalStateException("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.
There was a problem hiding this comment.
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.
- Guard destructive benchmarkRestoreKickoff in-method via isDestructiveEnabled()
(DBBACKUPRESTORE_BENCHMARK_ALLOW_DESTRUCTIVE), not just a launcher .exclude() that
a runner building its own Options can bypass.
- Wire getKnownRestoreId() to DBBACKUPRESTORE_BENCHMARK_RESTORE_ID so
benchmarkGetRestoreJob has a real config path; drop it from the sweep when unset
instead of timing an empty method.
- Make launcher exclusions capability-driven rather than a fixed regex.
- Document that the GCP LOCATION env is the full projects/{p}/locations/{loc} parent
path (a bare location is rejected with INVALID_ARGUMENT).
|
|
||
| // benchmarkRestoreKickoff kicks off a real, uncleaned-up restore on every invocation, so it | ||
| // must never run in a warmup/measurement loop unless the operator explicitly opts in. | ||
| if (!capabilities.isDestructiveEnabled()) { |
There was a problem hiding this comment.
🚨 The opt-in flag both admits benchmarkRestoreKickoff to the sweep and is its only bound, so enabling it does exactly what this class's javadoc says must never happen
The in-method guard at :228 is right, and it is genuinely fail-closed: Boolean.parseBoolean(null) is false, and it sits inside the @Benchmark, so a caller that builds its own Options or drives the generated BenchmarkList directly cannot bypass it.
But the same flag that arms the guard also skips this .exclude(), and benchmarkRestoreKickoff has no per-method @Warmup/@Measurement/@BenchmarkMode override. So with DBBACKUPRESTORE_BENCHMARK_ALLOW_DESTRUCTIVE=true it inherits the class-level 3×2 s warmup + 5×3 s measurement (:46-47) across both Throughput and SampleTime — roughly 42 s of back-to-back restoreBackup calls, each one a distinct target because getTargetResource() mints a fresh UUID name per call (AwsDBBackupRestoreBenchmarkTest:51, GcpFirestoreDBBackupRestoreBenchmarkTest:57), and none of them deletable. That is the 100+ undeletable restores that :40-41 says must never run in a warmup/measurement loop — now reachable by setting one env var on what an operator believes is a single opt-in.
Suggested fix: make the destructive path a single shot rather than a sweep, e.g. annotate the method @BenchmarkMode(Mode.SingleShotTime) @Warmup(iterations = 0) @Measurement(iterations = 1) (method-level annotations override the class), or count invocations in the method and refuse past a small cap.
Summary
Adds a JMH benchmark suite for DB backup/restore:
AbstractDBBackupRestoreBenchmarkTest(dbbackuprestore-client) plus thin AWS + GCP-Firestore concretes. Gated by@EnabledIfSystemProperty(named="runBenchmarks", matches="true")— inert in CI.Swept by default (2 read-only):
listBackups,getBackup.getRestoreJobis swept only when a restore id is supplied viaDBBACKUPRESTORE_BENCHMARK_RESTORE_ID; otherwise it is dropped from the sweep (it would otherwise time an empty method and publish a meaningless throughput number).restoreKickoffis destructive — each invocation initiates a real restore and the client exposes no delete API, so a restored table/database is left behind permanently. It is fenced two ways: excluded from the default sweep, and it hard-fails in-method unlessDBBACKUPRESTORE_BENCHMARK_ALLOW_DESTRUCTIVE=trueis set. Kept as a@Benchmarkso an operator with a disposable target can run it explicitly.Run contract
DBBACKUPRESTORE_BENCHMARK_AWS_{REGION,TABLE_ARN,ROLE_ID}DBBACKUPRESTORE_BENCHMARK_GCP_LOCATION— full parent pathprojects/{project}/locations/{location}(a bare location is rejected withINVALID_ARGUMENTinside@Setup)DBBACKUPRESTORE_BENCHMARK_GCP_DATABASE_NAMEDBBACKUPRESTORE_BENCHMARK_RESTORE_ID,DBBACKUPRESTORE_BENCHMARK_ALLOW_DESTRUCTIVERequires ≥1 existing backup on the target resource. Credentials flow via each cloud's default provider chain (OS env / profile / ADC), never
-D.Testing
Run locally against live AWS DynamoDB backups + GCP Firestore, both JMH modes. The 2 read-only methods produced populated results on both clouds.
getRestoreJobis dropped from the sweep when no restore id is supplied, andrestoreKickoffstays fenced unless the destructive opt-in is set (by design).Invocation (creds via OS env only):
JMH config note
Class-level annotations are the local-run baseline; a downstream pipeline may override them via its own runner.
Merge order
Independent. Recommend the root-pom JMH fix merges first.