Skip to content

refactor(systemtest): Share KMS facades across RecordEncryptionST tests#3703

Merged
k-wall merged 2 commits intokroxylicious:mainfrom
k-wall:recenc-st-share-kms
Apr 14, 2026
Merged

refactor(systemtest): Share KMS facades across RecordEncryptionST tests#3703
k-wall merged 2 commits intokroxylicious:mainfrom
k-wall:recenc-st-share-kms

Conversation

@k-wall
Copy link
Copy Markdown
Member

@k-wall k-wall commented Apr 13, 2026

Refactor RecordEncryptionST to use shared KMS instances rather than creating a new KMS instance per test. This approach shows significant time savings by avoiding repeated KMS container startup/shutdown cycles.

This applies the same technique successfully used in #3681 for RecordEncryptionFilterIT, where ~600 seconds was reduced to ~180 seconds.

Note: There was previously an unintended change on this PR removing record encryption compression tests. This was never my intent. Driving error with my Claude co-pilot.

Performance Improvement Results

This PR achieves a 12.3% performance improvement in the RecordEncryptionST system test suite.

Detailed Results

This PR (Run 24392758048):

Baseline (5 recent successful runs without this PR):

Run ID Duration Status
24387795464 35.33 min (2,120 sec) ✅ success
24385692901 34.07 min (2,044 sec) ✅ success
24382931322 33.88 min (2,033 sec) ✅ success
24381384618 36.43 min (2,186 sec) ✅ success
24379168699 36.05 min (2,163 sec) ✅ success
Average 35.15 min (2,109 sec)

Improvement Summary

  • Time saved: 4.34 minutes (260 seconds per run)
  • Percentage improvement: 12.3%

@k-wall k-wall requested a review from a team as a code owner April 13, 2026 13:04
Copy link
Copy Markdown
Member

@franvila franvila left a comment

Choose a reason for hiding this comment

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

LGTM. One nit

Copy link
Copy Markdown
Member

@SamBarker SamBarker left a comment

Choose a reason for hiding this comment

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

I like the overall change but I'm -0 on cutting the compression algorithms to just GZIP.

To be clear that means I'm leaving it to your discretion @k-wall

@k-wall k-wall marked this pull request as draft April 13, 2026 14:42
@k-wall k-wall changed the title perf(systemtest): Share KMS facades across RecordEncryptionST tests refactor(systemtest): Share KMS facades across RecordEncryptionST tests Apr 13, 2026
@k-wall k-wall force-pushed the recenc-st-share-kms branch from bba95f3 to 6257cdc Compare April 13, 2026 15:58
@k-wall k-wall force-pushed the recenc-st-share-kms branch 3 times, most recently from 085eda1 to 9bb242c Compare April 13, 2026 21:52
@k-wall k-wall marked this pull request as ready for review April 13, 2026 21:52
@k-wall k-wall force-pushed the recenc-st-share-kms branch 4 times, most recently from d8203de to 3ca9bf9 Compare April 14, 2026 08:26
private void pingLocalStackEndpoint() {
var client = HttpClient.newHttpClient();
var client = HttpClient.newBuilder()
.connectTimeout(Duration.ofSeconds(5))
Copy link
Copy Markdown
Member Author

@k-wall k-wall Apr 14, 2026

Choose a reason for hiding this comment

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

(Actually relates to #3706 rather than this PR)

k-wall and others added 2 commits April 14, 2026 12:53
Apply the shared KMS facade pattern from PR kroxylicious#3681 and kroxylicious#3700 to
RecordEncryptionST. This starts each KMS facade once and shares it
across all tests in the class, rather than starting/stopping for
each test method.

Changes:
- Replace @testtemplate + TestKubeKmsFacadeInvocationContextProvider
  with @ParameterizedClass and @MethodSource
- Add facadesSource() method using lazy stream evaluation with .peek()
- Use autoCloseArguments = true for automatic facade cleanup
- Replace @testtemplate + CompressionTypeInvocationContextProvider
  with @ParameterizedTest + @EnumSource(CompressionType.class) for
  compression testing

This approach allows three different parameter injection mechanisms
to work together:
- testKmsFacade from @parameter field (via @ParameterizedClass)
- namespace from KroxyliciousExtension (via AbstractSystemTests)
- compressionType from @EnumSource (via @ParameterizedTest)

Expected performance improvement: ~60% faster overall execution based
on similar changes to KmsIT (kroxylicious#3700) which showed 60% overall improvement
and 96.4% faster test execution for container-based KMS providers.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Signed-off-by: Keith Wall <kwall@apache.org>
Signed-off-by: Keith Wall <kwall@apache.org>
@franvila franvila force-pushed the recenc-st-share-kms branch from eef1631 to 7c46051 Compare April 14, 2026 10:54
@k-wall k-wall enabled auto-merge (squash) April 14, 2026 10:56
@sonarqubecloud
Copy link
Copy Markdown

@sonarqubecloud
Copy link
Copy Markdown

@k-wall k-wall merged commit fda8483 into kroxylicious:main Apr 14, 2026
33 checks passed
dahyvuun pushed a commit to dahyvuun/kroxylicious that referenced this pull request Apr 28, 2026
…ts (kroxylicious#3703)

* perf(systemtest): Share KMS facades across RecordEncryptionST tests

Apply the shared KMS facade pattern from PR kroxylicious#3681 and kroxylicious#3700 to
RecordEncryptionST. This starts each KMS facade once and shares it
across all tests in the class, rather than starting/stopping for
each test method.

Changes:
- Replace @testtemplate + TestKubeKmsFacadeInvocationContextProvider
  with @ParameterizedClass and @MethodSource
- Add facadesSource() method using lazy stream evaluation with .peek()
- Use autoCloseArguments = true for automatic facade cleanup
- Replace @testtemplate + CompressionTypeInvocationContextProvider
  with @ParameterizedTest + @EnumSource(CompressionType.class) for
  compression testing

This approach allows three different parameter injection mechanisms
to work together:
- testKmsFacade from @parameter field (via @ParameterizedClass)
- namespace from KroxyliciousExtension (via AbstractSystemTests)
- compressionType from @EnumSource (via @ParameterizedTest)

Expected performance improvement: ~60% faster overall execution based
on similar changes to KmsIT (kroxylicious#3700) which showed 60% overall improvement
and 96.4% faster test execution for container-based KMS providers.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Signed-off-by: Keith Wall <kwall@apache.org>

* reorder to allow parameter resolution to work.

Signed-off-by: Keith Wall <kwall@apache.org>

---------

Signed-off-by: Keith Wall <kwall@apache.org>
Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
dahyvuun pushed a commit to dahyvuun/kroxylicious that referenced this pull request Apr 28, 2026
…ts (kroxylicious#3703)

* perf(systemtest): Share KMS facades across RecordEncryptionST tests

Apply the shared KMS facade pattern from PR kroxylicious#3681 and kroxylicious#3700 to
RecordEncryptionST. This starts each KMS facade once and shares it
across all tests in the class, rather than starting/stopping for
each test method.

Changes:
- Replace @testtemplate + TestKubeKmsFacadeInvocationContextProvider
  with @ParameterizedClass and @MethodSource
- Add facadesSource() method using lazy stream evaluation with .peek()
- Use autoCloseArguments = true for automatic facade cleanup
- Replace @testtemplate + CompressionTypeInvocationContextProvider
  with @ParameterizedTest + @EnumSource(CompressionType.class) for
  compression testing

This approach allows three different parameter injection mechanisms
to work together:
- testKmsFacade from @parameter field (via @ParameterizedClass)
- namespace from KroxyliciousExtension (via AbstractSystemTests)
- compressionType from @EnumSource (via @ParameterizedTest)

Expected performance improvement: ~60% faster overall execution based
on similar changes to KmsIT (kroxylicious#3700) which showed 60% overall improvement
and 96.4% faster test execution for container-based KMS providers.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Signed-off-by: Keith Wall <kwall@apache.org>

* reorder to allow parameter resolution to work.

Signed-off-by: Keith Wall <kwall@apache.org>

---------

Signed-off-by: Keith Wall <kwall@apache.org>
Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
Signed-off-by: dahyvuun <dahyvuun@gmail.com>
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.

3 participants