Skip to content

fix(storage/chunk/client/aws): have GetObject check for canceled context - #14420

Merged
rfratto merged 1 commit into
grafana:mainfrom
rfratto:fix-aws-client
Oct 8, 2024
Merged

fix(storage/chunk/client/aws): have GetObject check for canceled context#14420
rfratto merged 1 commit into
grafana:mainfrom
rfratto:fix-aws-client

Conversation

@rfratto

@rfratto rfratto commented Oct 8, 2024

Copy link
Copy Markdown
Member

What this PR does / why we need it:

S3ObjectClient.GetObject incorrectly returned nil, 0, nil when the provided context is already canceled.

Which issue(s) this PR fixes:
N/A

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • Title matches the required conventional commits format, see here
    • Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR
S3ObjectClient.GetObject incorrectly returned nil, 0, nil when the
provided context is already canceled.
@rfratto
rfratto requested a review from a team as a code owner October 8, 2024 12:42
bucket := a.bucketFromKey(objectKey)

var lastErr error
lastErr := ctx.Err()

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is what the IBMCloud client does too, though I'm worried there's still an edge case where the context gets canceled in between this line and the first call to retries.Ongoing(). It sounds pretty unlikely so I didn't handle that here.

@rfratto
rfratto requested a review from chaudum October 8, 2024 12:55
@rfratto
rfratto merged commit 5f325aa into grafana:main Oct 8, 2024
@rfratto
rfratto deleted the fix-aws-client branch October 8, 2024 12:59
@loki-gh-app

loki-gh-app Bot commented Oct 8, 2024

Copy link
Copy Markdown
Contributor

This PR must be merged before a backport PR will be created.

3 similar comments
@loki-gh-app

loki-gh-app Bot commented Oct 8, 2024

Copy link
Copy Markdown
Contributor

This PR must be merged before a backport PR will be created.

@loki-gh-app

loki-gh-app Bot commented Oct 8, 2024

Copy link
Copy Markdown
Contributor

This PR must be merged before a backport PR will be created.

@loki-gh-app

loki-gh-app Bot commented Oct 8, 2024

Copy link
Copy Markdown
Contributor

This PR must be merged before a backport PR will be created.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment