Skip to content

docs: fix comment-rot, README Quick-Start scope, and benchmark inaccuracies#56

Merged
endrju19 merged 2 commits into
mainfrom
docs-comment-cleanup
Jun 5, 2026
Merged

docs: fix comment-rot, README Quick-Start scope, and benchmark inaccuracies#56
endrju19 merged 2 commits into
mainfrom
docs-comment-cleanup

Conversation

@endrju19
Copy link
Copy Markdown
Collaborator

@endrju19 endrju19 commented Jun 3, 2026

Summary

Outcome of a repo-wide documentation audit (cross-checking every comment/KDoc against the code it describes). All changes are comment/KDoc, test-display-name, or markdown only — zero runtime behaviour change. Verified: ./gradlew ktlintCheck green, :okapi-core:test + :okapi-micrometer:test pass, all edited test sources compile.

Each fix was independently confirmed by an adversarial verifier that decided which side was correct — in every case here the code was right and the documentation was wrong (one separate genuine code bug found by the same audit is tracked separately, not in this PR).

Comment-rot / stale-reference / misleading

File Was Now
DeliveryInfo.kt claimed CompositeMessageDeliverer routes by deserializing metadata routes via OutboxEntry.deliveryType; serialize() is for the concrete deliverer's own deserialize
TransactionRunner.kt non-existent modules okapi-spring / okapi-ktor okapi-spring-boot / okapi-exposed
TransactionContextValidator.kt "Standalone: no-op (always returns true)" — no such impl ships "Standalone: user-provided implementation"
ExposedConnectionProvider.kt TransactionManager.current() currentOrNull() (matches code)
ConnectionLeakProofTest.kt false history: "old getConnection(): Connection contract" that never existed describes the design pitfall the test guards against
MultiDataSourceTransactionTest.kt "PROPAGATION_REQUIRED nests via savepoints" (wrong) REQUIRED joins the existing tx; savepoints are PROPAGATION_NESTED
MicrometerOutboxMetricsTest.kt name/KDoc claimed "eager NaN before first refresh", contradicting its own inline note retitled to match what it asserts (lazy refresh, no store query on construction)
OkapiLiquibaseAutoConfiguration.kt @AutoConfigureAfter (×2) @AutoConfiguration(after = ...)
OkapiMetricsProperties.kt "Each refresh runs one transaction" (unconditional) wrapped in a transaction only when a tx manager is available
OutboxAutoConfiguration.kt "Requires a TransactionRunner bean" (unconditional) required only when a scheduler is enabled (publish-only needs none)
LiquibaseAutoConfigurationTest.kt brittle build.gradle.kts:35-36 ref (pointed at unrelated block) dropped the line numbers
OutboxAutoConfigurationTransactionRunnerTest.kt test name "...but currently is silent" (stale; assertion requires the WARN) describes the asserted WARN-branch behaviour
ObservabilityEndToEndTest.kt comment that just narrated the next two lines removed

README — Quick Start scope

The Quick Start had drifted into reference-manual depth. Trimmed the FOR UPDATE SKIP LOCKED mechanism explanation to a one-liner + pointer, and moved (verbatim, nothing deleted) the mechanism paragraph, the multi-DataSource paragraph, the TransactionTemplate-precedence paragraph, and the direct-scheduler-construction block into a new Advanced: transactions & multi-DataSource section placed right before How It Works.

benchmarks/README

  • Heap -Xms2g -Xmx2g-Xms8g -Xmx8g (matches build.gradle.kts; 2g OOMs in throughput mode).
  • Replaced the wrong "2 transports × 3 batchSize × 8 iterations" wall-time formula (HTTP actually runs 9 param combos via httpLatencyMs; warmup/measurement/fork were conflated) with an accurate matrix description.
  • Fixed an untranslated word.

Test plan

  • ./gradlew ktlintCheck — green
  • ./gradlew :okapi-core:test :okapi-micrometer:test — pass
  • :okapi-spring-boot:compileTestKotlin + :okapi-integration-tests:compileTestKotlin — compile clean (container E2E tests need Docker, unaffected — comment-only edits)
  • Confirmed via diff: no runtime code changed (only comments, KDoc, two test display names, markdown)

endrju19 added 2 commits June 3, 2026 09:47
…racies

Outcome of a repo-wide documentation audit. All changes are comment/KDoc,
test-display-name, or markdown only — zero runtime behaviour change.

Comment-rot / stale-reference / misleading (code was correct, comments fixed):
- DeliveryInfo: serialize() rationale claimed CompositeMessageDeliverer routes
  by deserializing metadata; it routes via OutboxEntry.deliveryType.
- TransactionRunner: KDoc named non-existent modules okapi-spring / okapi-ktor
  -> okapi-spring-boot / okapi-exposed.
- TransactionContextValidator: dropped false "Standalone: no-op (always true)"
  (no such impl ships) -> "user-provided implementation".
- ExposedConnectionProvider: current() -> currentOrNull() to match the code.
- ConnectionLeakProofTest: removed false history of an "old getConnection()
  contract" that never existed; describes the design pitfall instead.
- MultiDataSourceTransactionTest: corrected Spring semantics — REQUIRED joins
  the existing tx (no savepoint); savepoints are PROPAGATION_NESTED.
- MicrometerOutboxMetricsTest: renamed/retitled to match what it asserts
  (lazy refresh, no store query on construction), not "eager NaN" framing.
- OkapiLiquibaseAutoConfiguration: @AutoConfigureAfter -> @autoConfiguration(after=).
- OkapiMetricsProperties: refresh transaction is conditional on a tx manager.
- OutboxAutoConfiguration: TransactionRunner requirement is conditional on a
  scheduler being enabled (publish-only mode needs none).
- LiquibaseAutoConfigurationTest: dropped brittle build.gradle.kts:35-36 line ref.
- OutboxAutoConfigurationTransactionRunnerTest: dropped stale "currently is
  silent" from a test name (the assertion requires the WARN to be emitted).
- ObservabilityEndToEndTest: removed a comment that just narrated the code.

README: trimmed the FOR UPDATE SKIP LOCKED internals out of Quick Start to a
one-liner + pointer; moved the mechanism, multi-DataSource, TransactionTemplate
precedence, and direct-scheduler-construction paragraphs into a new
"Advanced: transactions & multi-DataSource" section (content preserved verbatim).

benchmarks/README: heap 2g -> 8g (matches build.gradle.kts; 2g OOMs in
throughput mode); replaced the wrong "2 transports x 3 x 8" wall-time formula
with an accurate matrix description; fixed an untranslated word.
The inline comment was fixed to explain PROPAGATION_REQUIRED joins the existing
transaction (no savepoint), but the test display name still read '(savepoint)',
contradicting the comment and perpetuating the misconception the audit removed.
(Code review follow-up.)
@endrju19 endrju19 merged commit 203a751 into main Jun 5, 2026
8 checks passed
@endrju19 endrju19 deleted the docs-comment-cleanup branch June 5, 2026 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant