Skip to content

fix-userver-slo-latency#628

Merged
Shfdis merged 4 commits into
mainfrom
fix-userver-slo-latency
Jun 17, 2026
Merged

fix-userver-slo-latency#628
Shfdis merged 4 commits into
mainfrom
fix-userver-slo-latency

Conversation

@Shfdis

@Shfdis Shfdis commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator

No description provided.

@Shfdis Shfdis added the SLO label Jun 16, 2026
@github-actions github-actions Bot removed the SLO label Jun 16, 2026
@pnv1 pnv1 requested a review from Copilot June 16, 2026 10:00
pnv1
pnv1 previously approved these changes Jun 16, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the userver-based SLO key/value workload runner to manage async work using userver::concurrent::BackgroundTaskStorage (instead of manually tracking tasks), and adds a repository-root .dockerignore to support Docker builds with the repo root as build context.

Changes:

  • Switched read/write workload execution in run.cpp from Semaphore + TaskWithResult vector to BackgroundTaskStorage with an atomic in-flight limiter.
  • Switched initial data generation in create.cpp to BackgroundTaskStorage (while retaining the semaphore limit).
  • Added a root-level .dockerignore (matching tests/slo_workloads/.dockerignore) for Docker build context filtering.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.

File Description
tests/slo_workloads/userver/key_value/run.cpp Reworks read/write request dispatching to use BackgroundTaskStorage and a custom in-flight limiter.
tests/slo_workloads/userver/key_value/create.cpp Reworks initial data upload dispatching to use BackgroundTaskStorage alongside the existing semaphore.
.dockerignore Adds root-level Docker ignore rules for builds using repo root as context.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/slo_workloads/userver/key_value/create.cpp Outdated
Comment thread tests/slo_workloads/userver/key_value/create.cpp Outdated
Comment thread tests/slo_workloads/userver/key_value/run.cpp
Comment thread tests/slo_workloads/userver/key_value/run.cpp
Comment thread tests/slo_workloads/userver/key_value/run.cpp Outdated
Comment thread tests/slo_workloads/userver/key_value/run.cpp Outdated

@robot-vibe-db robot-vibe-db Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

AI Review Summary

Verdict: ❌ 0 critical issues found, but 2 major issues require attention

Critical issues

No critical issues found.

Other findings

  • Major | Medium: RPS tokens consumed but requests dropped when inflight limit is reached (read loop) — tests/slo_workloads/userver/key_value/run.cpp:239
  • Major | Medium: RPS tokens consumed but requests dropped when inflight limit is reached (write loop) — tests/slo_workloads/userver/key_value/run.cpp:319
  • Minor | Medium: inflight counter not decremented if task lambda throws before fetch_sub (read loop) — tests/slo_workloads/userver/key_value/run.cpp:263
  • Minor | Medium: inflight counter not decremented if task lambda throws before fetch_sub (write loop) — tests/slo_workloads/userver/key_value/run.cpp:340
  • Minor | Medium: GetEnv() called on every metrics record (hot path) instead of being cached — tests/slo_workloads/utils/metrics.cpp:146
  • Nit | High: !requirements.txt exclusion override has no effect — .dockerignore:36

This review was generated automatically. Critical issues require attention; other findings are advisory.
If this comment was useful, please give it a 👍 — it helps us improve the review bot.

Comment thread tests/slo_workloads/userver/key_value/run.cpp
Comment thread tests/slo_workloads/userver/key_value/run.cpp
Comment thread tests/slo_workloads/userver/key_value/run.cpp
Comment thread tests/slo_workloads/userver/key_value/run.cpp
Comment thread tests/slo_workloads/utils/metrics.cpp Outdated
Comment thread .dockerignore
@robot-vibe-db

robot-vibe-db Bot commented Jun 17, 2026

Copy link
Copy Markdown

Full analysis log

Analysis performed by claude, claude-opus-4-6.

@Shfdis Shfdis added the SLO label Jun 17, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🌋 SLO Test Results

🟡 2 workload(s) tested — 1 workload(s) exceeded warning thresholds

Commit: 0a47362 · View run

Workload Thresholds Duration Report
cpp-key-value-userver 🟢 OK 10m 2s 📄 Report
cpp-key-value 🟡 Warning 10m 15s 📄 Report

Threshold violations:

cpp-key-value:

  • write_retry_attempts: ▲ 33.8% (≥ 20% warn)

Generated by ydb-slo-action

@github-actions github-actions Bot removed the SLO label Jun 17, 2026
@Shfdis Shfdis merged commit cf50109 into main Jun 17, 2026
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants