Skip to content

OSIS-156: log empty bucket list at DEBUG, classify on AmazonS3Exception 404#167

Merged
anurag4DSB merged 2 commits into
mainfrom
improvement/OSIS-156-bucket-list-empty-log-noise-mainport
Jun 29, 2026
Merged

OSIS-156: log empty bucket list at DEBUG, classify on AmazonS3Exception 404#167
anurag4DSB merged 2 commits into
mainfrom
improvement/OSIS-156-bucket-list-empty-log-noise-mainport

Conversation

@anurag4DSB

Copy link
Copy Markdown
Contributor

Intent: why does this change exist?

getBucketList returns an empty page when a tenant has no buckets, but the catch logged every case at ERROR with a stack trace, so the normal "no buckets yet" case looked like a failure and muddied diagnosis.

System impact: what's affected, including downstream?

Logging classification inside getBucketList only. The empty PageOfOsisBucketMeta return contract is unchanged.

Preserved behavior: what explicitly stays the same?

The empty-list contract is unchanged, and genuine faults (Vault, S3, network, auth) still log at ERROR with their stack trace exactly as before. Only the expected empty case is reclassified.

Intended change: what's different after this PR?

An expected empty list (an AmazonS3Exception with a 404 status, the type AmazonS3.listBuckets actually throws) is logged at DEBUG with a concise message and no stack trace. Tests pin DEBUG and no-trace for the empty case, and ERROR with a trace for a genuine fault.

Verification: how do we know this worked, or how would we know if it didn't?

Ran the full osis-core test module including the JaCoCo gate: green. The tests fail if the empty case logs at ERROR or WARN or carries a trace, or if a genuine fault stops logging at ERROR.

@claude

claude Bot commented Jun 26, 2026

Copy link
Copy Markdown

LGTM — the 404 classification is correct: the only AmazonS3Exception source in getBucketList is listBuckets(), so the instanceof+statusCode check accurately identifies the "no buckets" case. The isAdminPolicyError (403) path doesn't interfere. Tests cover both the expected-empty (404 → DEBUG, no trace) and genuine-fault (500 → ERROR with trace) paths with proper logger cleanup. The S3ServiceException → AmazonS3Exception change in the existing test aligns it with what the S3 client actually throws.

Review by Claude Code

@anurag4DSB anurag4DSB force-pushed the improvement/OSIS-156-bucket-list-empty-log-noise-mainport branch 2 times, most recently from 75dbb37 to 63751d1 Compare June 26, 2026 09:04
@claude

claude Bot commented Jun 26, 2026

Copy link
Copy Markdown

dump.rdb — Accidental Redis database dump committed to the repo. This file must be removed before merge. It may contain encrypted S3 credentials from the osis:s3credentials hash. Add *.rdb to .gitignore to prevent recurrence.


The core OSIS change (log-level reclassification in getBucketList) is clean:

  • The 404 → DEBUG classification is correct and well-placed — it runs after isAdminPolicyError (which only matches 403), so there's no interaction.
  • Non-404 faults still surface at ERROR with their stack trace, preserving the existing contract.
  • The two new tests pin both the expected-empty (DEBUG, no trace) and genuine-fault (ERROR, with trace) cases, and the existing testGetBucketListErr was correctly updated from S3ServiceException to AmazonS3Exception to match what the AWS SDK actually throws.
  • The dev/vcd-ose-lab/ changes (public-IP support, API probe scripts) are unrelated lab tooling and look fine.

No other issues found.

Review by Claude Code

@claude

claude Bot commented Jun 26, 2026

Copy link
Copy Markdown

LGTM — the change correctly classifies an expected 404 AmazonS3Exception (tenant with no buckets) to DEBUG while preserving ERROR+stack-trace for genuine faults. The empty-page return contract is unchanged. The existing testGetBucketListErr is updated to throw AmazonS3Exception (more realistic for an AWS SDK listBuckets call) instead of the Spring S3ServiceException, and the two new tests pin the log-level classification in both directions. No issues found.

Review by Claude Code

@anurag4DSB anurag4DSB marked this pull request as ready for review June 26, 2026 10:27
@anurag4DSB anurag4DSB requested a review from a team as a code owner June 26, 2026 10:27
@anurag4DSB anurag4DSB force-pushed the improvement/OSIS-156-bucket-list-empty-log-noise-mainport branch from 63751d1 to 30a8a56 Compare June 29, 2026 10:11
@anurag4DSB

Copy link
Copy Markdown
Contributor Author

no code change pushed, just rebased to main and force pushed.

…D after rebase

A rebase onto main re-expanded the wildcard imports (29 -> 34, over PMD's
ExcessiveImports limit of 30) and renamed s3Exception back to ex (ShortVariable),
silently reverting the PMD fix that had already passed CI. Restore both.
@anurag4DSB

anurag4DSB commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

reverted to wild card imports, no such change in the code behaviour.

@anurag4DSB anurag4DSB merged commit 564088c into main Jun 29, 2026
7 checks passed
@anurag4DSB anurag4DSB deleted the improvement/OSIS-156-bucket-list-empty-log-noise-mainport branch June 29, 2026 10:47
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