Skip to content

refactor(config): replace the hand-rolled AuthConfig secret lock with lazy val - #7040

Merged
aglinxinyuan merged 2 commits into
apache:mainfrom
aglinxinyuan:replace-authconfig-dcl-with-lazy-val
Jul 30, 2026
Merged

refactor(config): replace the hand-rolled AuthConfig secret lock with lazy val#7040
aglinxinyuan merged 2 commits into
apache:mainfrom
aglinxinyuan:replace-authconfig-dcl-with-lazy-val

Conversation

@aglinxinyuan

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

AuthConfig.jwtSecretKey hand-rolls a double-checked-locking lazy initializer — a @volatile var, a null sentinel and a synchronized block — to compute one value once. Scala's lazy val is that exact pattern, generated by the compiler.

// before
@volatile private var secretKey: String = _

def jwtSecretKey: String = {
  synchronized {
    if (secretKey == null) {
      secretKey = conf.getString("auth.jwt.256-bit-secret").toLowerCase() match {
        case "random" => getRandomHexString
        case key      => key
      }
    }
  }
  secretKey
}

// after
lazy val jwtSecretKey: String = conf.getString("auth.jwt.256-bit-secret").toLowerCase() match {
  case "random" => getRandomHexString
  case key      => key
}

Behaviour is preserved on every axis I checked:

hand-rolled lazy val
initialized at most once yes yes
safe under concurrent first access yes (synchronized) yes (compiler-generated DCL)
safe publication of the result yes (@volatile) yes
behaviour if the initializer throws leaves the field null, retries next call initialized bit not set, retries next call
call-site shape AuthConfig.jwtSecretKey unchanged

The single caller, JwtAuth.scala:35 (final val TOKEN_SECRET = AuthConfig.jwtSecretKey), needs no change — def and lazy val are indistinguishable at the call site. getRandomHexString stays a private def, so AuthConfigSpec's reflective lookup of it still resolves.

−14 lines, +6.

Any related issues, documentation, discussions?

Closes #7039

How was this PR tested?

Existing tests only — this PR adds none, since it is a behaviour-preserving rewrite already covered by AuthConfigSpec (which pins both the configured-secret path and that the value is memoized across calls).

Locally, from the repo root with Java 17:

  • sbt "scalafixAll --check" — clean.
  • sbt scalafmtCheckAll — clean.
  • sbt "Config/testOnly *AuthConfigSpec" — 3 tests, all pass, unmodified.

The whole-repo scalafix/scalafmt run also compiles common/auth, so the JwtAuth call site is covered.

Was this PR authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Claude Opus 5)

@github-actions

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

  • Contributors with relevant context: @Ma77Ball
    You can notify them by mentioning @Ma77Ball in a comment.

@codecov-commenter

codecov-commenter commented Jul 29, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.09%. Comparing base (02ae12b) to head (8a81451).

Files with missing lines Patch % Lines
...a/org/apache/texera/common/config/AuthConfig.scala 33.33% 1 Missing and 1 partial ⚠️

❌ Your patch status has failed because the patch coverage (33.33%) is below the target coverage (60.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7040      +/-   ##
============================================
- Coverage     79.27%   79.09%   -0.18%     
+ Complexity     3787     3786       -1     
============================================
  Files          1160     1160              
  Lines         46124    46053      -71     
  Branches       5114     5108       -6     
============================================
- Hits          36563    36424     -139     
- Misses         7942     8007      +65     
- Partials       1619     1622       +3     
Flag Coverage Δ *Carryforward flag
access-control-service 70.00% <ø> (ø)
agent-service 76.76% <ø> (ø) Carriedforward from 9de1386
amber 72.60% <33.33%> (-0.01%) ⬇️
computing-unit-managing-service 20.49% <ø> (ø)
config-service 66.66% <ø> (ø)
file-service 67.21% <ø> (ø)
frontend 82.91% <ø> (-0.18%) ⬇️ Carriedforward from 9de1386
notebook-migration-service 78.94% <ø> (ø)
pyamber 95.38% <ø> (-0.97%) ⬇️ Carriedforward from 9de1386
workflow-compiling-service 26.31% <ø> (ø)

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

🟢 6 better · 🔴 3 worse · ⚪ 6 noise (<±5%) · 0 without baseline

Compared against main 02ae12b benchmarked on this same runner, so the delta is largely free of cross-runner hardware noise. The "7d avg" column still reflects the gh-pages dashboard. Treat <±5% as noise unless repeated.

Dashboard · Run

config throughput MB/s latency max Δ latest / 7d
🔴 bs=10 sw=10 sl=64 577 0.352 17,089/23,879/23,879 us 🔴 +20.8% / 🔴 +51.4%
🟢 bs=100 sw=10 sl=64 1,444 0.881 68,098/94,967/94,967 us 🟢 -11.2% / 🟢 +44.4%
🟢 bs=1000 sw=10 sl=64 1,677 1.024 598,922/611,477/611,477 us 🟢 -15.5% / 🟢 +62.7%
Baseline details

Latest main 02ae12b from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 577 tuples/sec 666 tuples/sec 786.12 tuples/sec -13.4% -26.6%
bs=10 sw=10 sl=64 MB/s 0.352 MB/s 0.407 MB/s 0.48 MB/s -13.5% -26.6%
bs=10 sw=10 sl=64 p50 17,089 us 14,146 us 12,305 us +20.8% +38.9%
bs=10 sw=10 sl=64 p95 23,879 us 26,267 us 15,774 us -9.1% +51.4%
bs=10 sw=10 sl=64 p99 23,879 us 26,267 us 18,978 us -9.1% +25.8%
bs=100 sw=10 sl=64 throughput 1,444 tuples/sec 1,416 tuples/sec 999.71 tuples/sec +2.0% +44.4%
bs=100 sw=10 sl=64 MB/s 0.881 MB/s 0.864 MB/s 0.61 MB/s +2.0% +44.4%
bs=100 sw=10 sl=64 p50 68,098 us 65,957 us 100,616 us +3.2% -32.3%
bs=100 sw=10 sl=64 p95 94,967 us 106,969 us 107,356 us -11.2% -11.5%
bs=100 sw=10 sl=64 p99 94,967 us 106,969 us 113,255 us -11.2% -16.1%
bs=1000 sw=10 sl=64 throughput 1,677 tuples/sec 1,609 tuples/sec 1,031 tuples/sec +4.2% +62.6%
bs=1000 sw=10 sl=64 MB/s 1.024 MB/s 0.982 MB/s 0.63 MB/s +4.3% +62.7%
bs=1000 sw=10 sl=64 p50 598,922 us 600,838 us 980,328 us -0.3% -38.9%
bs=1000 sw=10 sl=64 p95 611,477 us 723,719 us 1,027,528 us -15.5% -40.5%
bs=1000 sw=10 sl=64 p99 611,477 us 723,719 us 1,054,298 us -15.5% -42.0%
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,346.89,200,128000,577,0.352,17088.98,23878.89,23878.89
1,100,10,64,20,1384.93,2000,1280000,1444,0.881,68097.73,94967.34,94967.34
2,1000,10,64,20,11925.29,20000,12800000,1677,1.024,598921.70,611477.25,611477.25

@aglinxinyuan
aglinxinyuan added this pull request to the merge queue Jul 30, 2026
Merged via the queue into apache:main with commit 0834f25 Jul 30, 2026
31 checks passed
@aglinxinyuan
aglinxinyuan deleted the replace-authconfig-dcl-with-lazy-val branch July 30, 2026 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace the hand-rolled AuthConfig secret lock with lazy val

3 participants