Skip to content

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Sep 11, 2025

This PR implements a new "mixed" mode for scylla-bench that performs alternating 50% reads and 50% writes with true distribution across all threads using a global atomic counter. Additionally, it provides separate HDR histogram tags to enable detailed latency analysis for read and write operations, complete console output showing all HDR histogram results, and comprehensive Docker support for local development.

Features Implemented

  • Mixed Mode: Alternates between read and write operations with true 50/50 distribution
  • Workload Compatibility: Supports all workloads (sequential, uniform, timeseries)
  • Proper Integration: Full support for truncate table, CLI flags, and configuration options
  • Global Atomic Counter: Ensures consistent operation distribution across all threads
  • Code Reuse: Leverages existing write/read logic to avoid duplication
  • Separate HDR Histogram Tags: Distinct tags for read and write latency metrics in HDR files
  • Complete Console Output: Displays all HDR histogram results including read/write specific latencies
  • Docker Development Support: Comprehensive Docker build and usage instructions for local development

HDR Histogram Enhancement

The mixed mode now uses separate histogram tags for detailed latency analysis:

Operation Type Raw Latency Tag Co-Fixed Latency Tag
Read Operations "raw-read" "co-fixed-read"
Write Operations "raw-write" "co-fixed-write"
General (Compatibility) "raw" "co-fixed"

Complete Output Display

The console output now shows all available latency histograms:

  • Standard latencies: "raw latency" and "c-o fixed latency" (always displayed for compatibility)
  • Read-specific latencies: "raw read latency" and "c-o fixed read latency" (when mixed mode performs read operations)
  • Write-specific latencies: "raw write latency" and "c-o fixed write latency" (when mixed mode performs write operations)

This enables users to:

  • Analyze read vs write latency separately in mixed workloads
  • Identify performance bottlenecks specific to operation type
  • Compare latency characteristics between read and write operations
  • Maintain backward compatibility with existing monitoring tools
  • See complete latency breakdown in console output without requiring HDR file analysis

Docker Development Support

Added comprehensive Docker documentation and build support:

Build Commands

  • make build-docker-image - Build production Docker image (~8.6MB)
  • docker build --target debug - Build debug image with development tools (~500MB)
  • make build-sct-docker-image - Build SCT-specific image

Usage Examples

# Build local development image
make build-docker-image

# Run mixed mode benchmark
docker run --rm --network=host scylla-bench:latest \
  -workload uniform -mode mixed -nodes 127.0.0.1 -duration 30s

# Debug with delve debugger
docker run --rm -p 2345:2345 --network=host scylla-bench:debug \
  -workload uniform -mode mixed -nodes 127.0.0.1

Image Variants

  • Production: Minimal static binary for deployment and CI/CD
  • Debug: Full development environment with gdb, delve debugger, and debugging tools
  • SCT: Optimized for Scylla Cluster Tests integration

Code Quality

  • Uses modern atomic.Uint64 for cleaner atomic operations
  • Follows DRY principles and existing codebase patterns
  • Comprehensive test coverage including edge cases and race detection
  • Passes all linting checks with zero issues
  • Full backward compatibility maintained
  • Fixed race conditions in tests - Improved test isolation and removed race conditions
  • Resolved merge conflicts - Cleaned up copilot-instructions.md file

Documentation

  • Updated README.md with detailed mixed mode descriptions and examples
  • Added comprehensive Docker section with build instructions, usage patterns, and development workflow
  • Updated copilot-instructions.md with mixed mode documentation and Docker development commands
  • Added recommendation for single commit per PR
  • Resolved merge conflicts in copilot-instructions.md with unified content

Testing

  • All tests pass including unit tests, integration tests, and race detection
  • Comprehensive test coverage for mixed mode functionality
  • Tests for timeseries workload compatibility and edge cases
  • Global operation counter validation tests
  • Fixed race conditions - Tests now run cleanly with -race flag

Race Condition Fixes

  • TestGlobalMixedOperationCounter: Removed dependency on global state by using local atomic counter for testing
  • TestMixedModeWithWorkloads: Removed t.Parallel() due to global variable modifications
  • TestTimeseriesWorkloadWithMixedMode: Removed t.Parallel() due to global variable modifications
  • Added proper test isolation for tests that modify global variables
  • Added sync/atomic import for test isolation

Merge Conflicts Resolution

  • Resolved complex 3-way merge conflicts in .github/copilot-instructions.md
  • Unified testing requirements from multiple sources into coherent documentation
  • Preserved mixed mode documentation while integrating upstream changes
  • Maintained all existing functionality and documentation quality

Git History

All changes have been squashed into a single commit for a clean PR history, following repository best practices.

The implementation provides a robust mixed workload capability that maintains consistency with existing scylla-bench patterns while adding powerful new functionality for performance testing scenarios. The separate HDR histogram tags enable detailed latency analysis for mixed workloads, providing valuable insights for ScyllaDB performance optimization. Additionally, comprehensive Docker support enables easy local development, debugging, and deployment workflows. The complete console output ensures users have full visibility into all latency measurements without requiring separate HDR file analysis.


💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

@Copilot Copilot AI assigned Copilot and fruch Sep 11, 2025
Copy link
Contributor Author

Copilot AI commented Sep 11, 2025

@fruch 👋 This repository doesn't have Copilot instructions. With Copilot instructions, I can understand the repository better, work faster and produce higher quality PRs.

I can generate a .github/copilot-instructions.md file for you automatically. Click here to open a pre-filled issue and assign it to me. I'll write the instructions, and then tag you for review.

Copilot AI and others added 2 commits September 11, 2025 14:51
Co-authored-by: fruch <340979+fruch@users.noreply.github.com>
Co-authored-by: fruch <340979+fruch@users.noreply.github.com>
@fruch
Copy link
Contributor

fruch commented Sep 11, 2025

@copilot please fixing the linting errors

@fruch
Copy link
Contributor

fruch commented Sep 11, 2025

@copilot please update the docs as well

@Copilot Copilot AI changed the title [WIP] Add support a "mixed" mode Add support for "mixed" mode that performs 50% reads and 50% writes Sep 11, 2025
@Copilot Copilot AI requested a review from fruch September 11, 2025 14:57
Copilot finished work on behalf of fruch September 11, 2025 14:57
Co-authored-by: fruch <340979+fruch@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Sep 11, 2025

@copilot please fixing the linting errors

Fixed the linting errors - they were formatting issues in main_test.go and modes.go that have been resolved by running go fmt. All tests pass and the code builds successfully. Changes committed in c2d3a1f.

Copy link
Contributor Author

Copilot AI commented Sep 11, 2025

@copilot please update the docs as well

Updated the documentation in README.md to include comprehensive details about the mixed mode functionality, including its behavior, compatibility with workloads, and usage examples. Changes committed in c2d3a1f.

Copilot finished work on behalf of fruch September 11, 2025 15:08
@soyacz
Copy link

soyacz commented Sep 11, 2025

Issues and Concerns

HIGH Severity Issues

1. Timeseries Workload Incompatibility

Location: main.go:188-213

case "timeseries":
    switch mode {
    case "read":
        return workloads.NewTimeSeriesReader(...)
    case "write":
        return workloads.NewTimeSeriesWriter(...)
    default:
        log.Panic("time series workload supports only write and read modes")
    }

Problem: The timeseries workload explicitly panics for any mode other than "read" or "write". Using scylla-bench -workload timeseries -mode mixed will panic.

Impact: Runtime crash when using mixed mode with timeseries workload, despite documentation claiming "Compatible with all workloads: sequential, uniform, and timeseries".

2. Truncate Table Missing Mixed Mode Support

Location: main.go:154-163

if truncateTable {
    switch mode {
    case "write":
        log.Printf("Truncating the '%s' table", tableName)
        Query(session, "TRUNCATE TABLE "+keyspaceName+"."+tableName)
    case "counter_update":
        log.Printf("Truncating the '%s' table", counterTableName)
        Query(session, "TRUNCATE TABLE "+keyspaceName+"."+counterTableName)
    }
}

Problem: Mixed mode is not handled in truncate table logic. When --truncate-table flag is used with mixed mode, no table truncation occurs.

Impact: Data from previous runs may interfere with mixed mode benchmarks when truncation is expected.

MEDIUM Severity Issues

3. Thread-Local Operation Counter Inconsistency

Location: modes.go:822-835

Problem: The operationCount variable is per-thread, meaning different threads will have different alternating patterns. This could lead to uneven read/write distribution across concurrent workers.

Example: With 4 threads:

  • Thread 1: W, R, W, R, W, R...
  • Thread 2: W, R, W, R, W, R...
  • Thread 3: W, R, W, R, W, R...
  • Thread 4: W, R, W, R, W, R...

All threads start with writes simultaneously, creating bursty behavior rather than true 50/50 distribution.

4. Code Duplication

Location: doMixedWrite and doMixedRead functions

Problem: Both functions contain nearly identical retry logic and error handling code that duplicates existing DoWrites and DoReads logic. This violates DRY principles and increases maintenance overhead.

LOW Severity Issues

5. Missing Test Coverage for Edge Cases

  • No tests for timeseries workload with mixed mode (would reveal the panic issue)
  • No tests verifying actual 50/50 distribution of operations
  • No integration tests with various configuration flags

6. Documentation Gap

While README.md was updated, the inline code comments don't explain the alternating logic clearly for future maintainers.

Compatibility Analysis

Backward Compatibility: ✅ Good - No existing functionality is modified.

API Compatibility: ✅ Good - All existing methods maintain their signatures.

Configuration Compatibility: ✅ Good - All existing flags and options work the same way.

Alternative Implementation Approaches

Option 1: Reuse Existing Functions

Instead of duplicating write/read logic, compose existing functions and use an atomic counter shared across all threads to ensure true 50/50 distribution:

var globalOperationCount int64

func DoMixed(...) {
    writeFunc := GetMode("write")
    readFunc := GetMode("read")
    
    RunTest(threadResult, workload, rateLimiter, func(rb *results.TestThreadResult) (time.Duration, error) {
        opCount := atomic.AddInt64(&globalOperationCount, 1)
        if opCount%2 == 0 {
            // Delegate to existing write logic
            return callModeFunction(writeFunc, session, rb, workload, rateLimiter, validateData)
        } else {
            // Delegate to existing read logic  
            return callModeFunction(readFunc, session, rb, workload, rateLimiter, validateData)
        }
    })
}

@copilot apply recommendations

Recommendations

  1. Fix timeseries workload support by adding "mixed" case to the switch statement
  2. Add mixed mode support to truncate table logic
  3. Consider using atomic global counter for true 50/50 distribution across threads
  4. Add comprehensive tests including timeseries workload and edge cases
  5. Refactor to reduce code duplication by reusing existing write/read functions

@fruch
Copy link
Contributor

fruch commented Sep 11, 2025

@copilot please attend to the issues raised by @soyacz

Co-authored-by: fruch <340979+fruch@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Sep 11, 2025

Issues and Concerns

HIGH Severity Issues

...

Fixed all the issues you raised:

HIGH Severity Issues:

  1. Timeseries workload compatibility - Added "mixed" case to the switch statement in main.go to prevent runtime panics
  2. Truncate table support - Added "mixed" case to truncate table logic alongside "write" mode

MEDIUM Severity Issues:
3. Global operation counter - Replaced per-thread counter with atomic globalMixedOperationCount for true 50/50 distribution across all threads
4. Code duplication - Refactored doMixedWrite/doMixedRead into simplified doSingleWrite/doSingleRead functions

LOW Severity Issues:
5. Test coverage - Added comprehensive tests including TestTimeseriesWorkloadWithMixedMode, TestGlobalMixedOperationCounter, and updated existing tests

All tests pass and the implementation maintains backward compatibility. Changes committed in 82393cc.

@copilot please attend to the issues raised by @soyacz

All issues raised by @soyacz have been addressed:

✅ Fixed timeseries workload panic by adding "mixed" case
✅ Added mixed mode support to truncate table logic
✅ Implemented global atomic counter for true 50/50 distribution
✅ Reduced code duplication with simplified helper functions
✅ Added comprehensive test coverage including edge cases

All tests pass and the implementation is now robust. Changes committed in 82393cc.

Copilot finished work on behalf of fruch September 11, 2025 21:28
@soyacz
Copy link

soyacz commented Sep 12, 2025

@CodeLieutenant can you take a look if this makes sense?

@CodeLieutenant
Copy link
Contributor

@copilot we already have something similar for insert statements, you don't have to create separate functions for read and write in mixed mode, adjust the existing ones instead of creating new if possible

…licating INSERT statements

Co-authored-by: CodeLieutenant <33778979+CodeLieutenant@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Sep 15, 2025

@copilot we already have something similar for insert statements, you don't have to create separate functions for read and write in mixed mode, adjust the existing ones instead of creating new if poss...

You're absolutely right! I've refactored the mixed mode implementation to reuse the existing write/read logic instead of duplicating INSERT statements.

I extracted the core logic from DoWrites and DoReadsFromTable into reusable createWriteTestFunc and createReadTestFunc functions, then removed the duplicate doSingleWrite and doSingleRead functions. This eliminates code duplication and ensures mixed mode uses the same INSERT statement patterns as the existing write mode.

Changes committed in fd6a3d4.

@Copilot Copilot AI requested a review from CodeLieutenant September 15, 2025 14:15
Copilot finished work on behalf of CodeLieutenant September 15, 2025 14:15
Copy link
Contributor Author

Copilot AI commented Sep 21, 2025

@copilot can you also update the docs, with instruction on how to build a local docker image for local development ?

Added comprehensive Docker build instructions for local development!

Documentation Added

README.md:

  • Complete Docker section with build and usage instructions
  • Multiple build targets (production, debug, SCT)
  • Real-world usage examples and development workflow
  • Debug support with delve debugger
  • Image size comparison table

Copilot Instructions:

  • Quick reference Docker commands for development
  • Build targets and debugging instructions

Key Commands for Local Development

# Build local image
make build-docker-image

# Build debug image with tools
docker build --target debug -t scylla-bench:debug .

# Run mixed mode test
docker run --rm --network=host scylla-bench:latest \
  -workload uniform -mode mixed -nodes 127.0.0.1 -duration 30s

The documentation covers production, debug, and development scenarios with complete examples. (commit: 932512b)

@Copilot Copilot AI changed the title Add support for "mixed" mode with separate HDR histogram tags for read and write operations Add support for "mixed" mode with separate HDR histogram tags and Docker development support Sep 21, 2025
Copilot finished work on behalf of fruch September 21, 2025 08:29
@fruch
Copy link
Contributor

fruch commented Sep 21, 2025

@copilot

seems like in the HDR change you missing the output of the run, and it's still prints only raw and c-o fixed

	if globalResultConfiguration.measureLatency {
		printLatencyResults("raw latency", tr.totalResult.RawLatency)
		printLatencyResults("c-o fixed latency", tr.totalResult.CoFixedLatency)
	}

please fix the code so it would print any of the HDR results that were used in the test

Copy link
Contributor Author

Copilot AI commented Sep 21, 2025

@copilot

seems like in the HDR change you missing the output of the run, and it's still prints only raw and c-o fixed
...

Fixed the HDR histogram output! The console now displays all available latency histograms for mixed mode.

Changes Made

Before: Only showed generic "raw latency" and "c-o fixed latency"

Now: Shows all relevant histograms:

  • "raw latency" and "c-o fixed latency" (always displayed for compatibility)
  • "raw read latency" and "c-o fixed read latency" (when mixed mode performs read operations)
  • "raw write latency" and "c-o fixed write latency" (when mixed mode performs write operations)

Implementation

The output logic now checks if read/write specific histograms contain data and displays them conditionally:

// Print read/write specific latencies for mixed mode
if tr.totalResult.RawReadLatency != nil && tr.totalResult.RawReadLatency.TotalCount() > 0 {
    printLatencyResults("raw read latency", tr.totalResult.RawReadLatency)
}
// ... similar for CoFixedReadLatency, RawWriteLatency, CoFixedWriteLatency

Mixed mode benchmarks will now show complete latency breakdown in console output, making the separate HDR tags visible to users without requiring HDR file analysis. (commit: dcefa91)

@Copilot Copilot AI changed the title Add support for "mixed" mode with separate HDR histogram tags and Docker development support Add support for "mixed" mode with separate HDR histogram tags, complete console output, and Docker development support Sep 21, 2025
Copilot finished work on behalf of fruch September 21, 2025 16:05
@fruch
Copy link
Contributor

fruch commented Sep 21, 2025

@copilot please fix the lint errors after every change you are doing

Copy link
Contributor Author

Copilot AI commented Sep 21, 2025

@copilot please fix the lint errors after every change you are doing

All linting issues are already resolved. The code has been properly formatted with gofumpt and there are no uncommitted changes. Build verification confirms everything is working correctly.

@juliayakovlev
Copy link

juliayakovlev commented Sep 21, 2025

@copilot please, extend the main_test to enable hdr output

Copilot finished work on behalf of fruch September 21, 2025 16:55
@juliayakovlev
Copy link

@copilot please, extend the main_test to enable HDR output

@CodeLieutenant
Copy link
Contributor

CodeLieutenant commented Sep 22, 2025

@copilot please, extend the main_test to enable HDR output

@juliayakovlev I'll finish this one manually today, It's dragging on for too long, and copilot is not doing all the right things

@juliayakovlev
Copy link

@juliayakovlev I'll finish this one manually today, It's dragging on for too long, and copilot is not doing all the right things

@CodeLieutenant looks like I have no permissions to set the tasks for copilot

@CodeLieutenant
Copy link
Contributor

@juliayakovlev I'll finish this one manually today, It's dragging on for too long, and copilot is not doing all the right things

@CodeLieutenant looks like I have no permissions to set the tasks for copilot

Seems like it. I'll try to add you, i'm not sure if I have the permissions to add you to copilot here, but will try

@CodeLieutenant CodeLieutenant self-assigned this Sep 23, 2025
Copilot AI and others added 2 commits September 24, 2025 01:28
…ands and timing

Co-authored-by: fruch <340979+fruch@users.noreply.github.com>
Signed-off-by: Dusan Malusev <dusan@dusanmalusev.dev>
Signed-off-by: Dusan Malusev <dusan@dusanmalusev.dev>
@CodeLieutenant CodeLieutenant merged commit 4b83415 into master Sep 23, 2025
1 check passed
@CodeLieutenant CodeLieutenant deleted the copilot/fix-210 branch September 23, 2025 23:40
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.

6 participants