Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

blobfixture: tolerate non-found errors #141839

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jeffswenson
Copy link
Collaborator

Previously, when the fixture GC was running, registry operations could
fail because they would list metadata blobs then attempt to read them.
Now, the registry tolerates missing blobs with the assumption they were
deleted by a concurrent GC.

Release note: none
Part of: #139159

@jeffswenson jeffswenson requested review from a team as code owners February 21, 2025 15:07
@jeffswenson jeffswenson requested review from golgeek, DarrylWong and dt and removed request for a team February 21, 2025 15:07
@jeffswenson
Copy link
Collaborator Author

This PR depends on #141795.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link

⚪ Sysbench [SQL, 3node, oltp_read_write]
Metric Old Commit New Commit Delta Note Threshold
sec/op 10.92m ±5% 10.84m ±2% ~ p=0.631 n=10 3.0%
errs/op 0.000 ±0% 0.000 ±0% ~ p=1.000 n=10 0.0%
allocs/op 10.20k ±0% 10.21k ±0% ~ p=0.837 n=10 2.0%
B/op 2.207Mi ±0% 2.206Mi ±0% ~ p=0.853 n=10 2.0%
Reproduce

benchdiff binaries:

mkdir -p benchdiff/58b84a0/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/58b84a00047de54ce365afbf071778b456844ebc/bin/pkg_sql_tests benchdiff/58b84a0/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/58b84a0/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/61ad6b7/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/61ad6b723ee22d647e5eeb3e47e3fb69be93785e/bin/pkg_sql_tests benchdiff/61ad6b7/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/61ad6b7/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests

benchdiff command:

benchdiff --run=^BenchmarkSysbench/SQL/3node/oltp_read_write$ --old=61ad6b7 --new=58b84a0 ./pkg/sql/tests
⚪ Sysbench [KV, 1node, local, oltp_read_only]
Metric Old Commit New Commit Delta Note Threshold
sec/op 658.1µ ±1% 658.2µ ±1% ~ p=0.315 n=10 2.0%
errs/op 0.000 ±0% 0.000 ±0% ~ p=1.000 n=10 0.0%
allocs/op 439.0 ±0% 439.0 ±0% ~ p=1.000 n=10 1.5%
B/op 254.1Ki ±0% 254.2Ki ±0% ~ p=0.739 n=10 1.5%
Reproduce

benchdiff binaries:

mkdir -p benchdiff/58b84a0/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/58b84a00047de54ce365afbf071778b456844ebc/bin/pkg_sql_tests benchdiff/58b84a0/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/58b84a0/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/61ad6b7/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/61ad6b723ee22d647e5eeb3e47e3fb69be93785e/bin/pkg_sql_tests benchdiff/61ad6b7/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/61ad6b7/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests

benchdiff command:

benchdiff --run=^BenchmarkSysbench/KV/1node_local/oltp_read_only$ --old=61ad6b7 --new=58b84a0 ./pkg/sql/tests
⚪ Sysbench [KV, 1node, local, oltp_write_only]
Metric Old Commit New Commit Delta Note Threshold
sec/op 1.300m ±0% 1.296m ±1% ~ p=0.481 n=10 2.5%
errs/op 0.000 ±0% 0.000 ±0% ~ p=1.000 n=10 0.0%
allocs/op 1.390k ±0% 1.390k ±0% ~ p=0.495 n=10 1.8%
B/op 288.6Ki ±0% 288.6Ki ±1% ~ p=0.481 n=10 1.8%
Reproduce

benchdiff binaries:

mkdir -p benchdiff/58b84a0/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/58b84a00047de54ce365afbf071778b456844ebc/bin/pkg_sql_tests benchdiff/58b84a0/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/58b84a0/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/61ad6b7/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/61ad6b723ee22d647e5eeb3e47e3fb69be93785e/bin/pkg_sql_tests benchdiff/61ad6b7/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/61ad6b7/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests

benchdiff command:

benchdiff --run=^BenchmarkSysbench/KV/1node_local/oltp_write_only$ --old=61ad6b7 --new=58b84a0 ./pkg/sql/tests
Artifacts

download:

mkdir -p new
gcloud storage cp gs://cockroach-microbench-ci/artifacts/58b84a00047de54ce365afbf071778b456844ebc/13459556616-1/\* new/
mkdir -p old
gcloud storage cp gs://cockroach-microbench-ci/artifacts/61ad6b723ee22d647e5eeb3e47e3fb69be93785e/13459556616-1/\* old/
Legend
  • Neutral: No significant performance change.
  • 🟡 Warning: Slight degradation, likely due to variance, but still within thresholds.
  • 🔴 Regression: Likely performance regression, requiring investigation.
  • 🟢 Improvement: Possible performance gain.

No regressions detected!

built with commit: 58b84a00047de54ce365afbf071778b456844ebc

@jeffswenson jeffswenson force-pushed the jeffswenson-improve-registry-gc branch from 58b84a0 to cd5ef35 Compare February 21, 2025 19:28
Previously, the external storage providers had different behavior when
deleting an object that does not exist. S3 treats deleting a
non-existent file as OK. All other storage providers returned an error.
This change updates all storage providers so they match the S3 behavior.

An alternative design would be to have Delete return ErrFileNotFound. I
was unable to find an efficient way to implement that in the S3 storage
provider. The S3 delete response provides no hint as to whether there
was an object at the path. So generating ErrFileNotFound would require
reading the path before issuing the delete.

Release note: none
Epic: none
Previously, when the fixture GC was running, registry operations could
fail because they would list metadata blobs then attempt to read them.
Now, the registry tolerates missing blobs with the assumption they were
deleted by a concurrent GC.

Release note: none
Part of: cockroachdb#139159
@jeffswenson jeffswenson force-pushed the jeffswenson-improve-registry-gc branch from cd5ef35 to c97ba77 Compare February 21, 2025 19:32
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.

2 participants