-
Notifications
You must be signed in to change notification settings - Fork 944
Go Workloads for Testing #25414
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
base: main
Are you sure you want to change the base?
Go Workloads for Testing #25414
Conversation
There was a problem hiding this comment.
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 adds a Go workload testing framework for Azure Cosmos DB DR drills. The workload creates a database and container, pre-populates items across logical partitions, and continuously performs concurrent read/write/query operations until terminated.
Key changes:
- Configuration management via environment variables for Cosmos DB connection and workload parameters
- Initial setup phase that creates database/container and upserts items across logical partitions
- Continuous workload execution with concurrent random operations (upsert, read, query)
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/data/azcosmos/workloads/workload_config.go | Loads workload configuration from environment variables |
| sdk/data/azcosmos/workloads/workload_utils.go | Provides utilities for client creation and random operations |
| sdk/data/azcosmos/workloads/initial_setup.go | Sets up database, container, and pre-populates test items |
| sdk/data/azcosmos/workloads/r_w_q_workload.go | Runs continuous workload operations |
| sdk/data/azcosmos/workloads/main/main.go | Entry point that orchestrates setup and workload execution |
| sdk/data/azcosmos/workloads/dev.md | Documentation for setting up and running scale tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a couple comments, thanks Tomas! Seems like Copilot caught a couple things as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good, but a small blocking issue about logging and some other suggestions. In addition, as we chatted about offline, we should give the workloads/ directory it's own go.mod file. That should prevent a circular dependency between the Go SDK and client engine AND ensure that users can't accidentally start calling these APIs.
|
|
||
| import ( | ||
| "fmt" | ||
| "log" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine for ad-hoc testing, but when we actually want to add logging, there's a facility built in to the Core SDK for that in the "github.com/Azure/azure-sdk-for-go/sdk/internal/log" package. I used it in a recent commit to the query engine code path: 6d8a427
| var firstErr error | ||
| for e := range errs { | ||
| if firstErr == nil { | ||
| firstErr = e | ||
| } | ||
| } | ||
| return firstErr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use errors.Join to make them into a single error if we want to be able to see all errors.
| return cfg, nil | ||
| } | ||
|
|
||
| func splitAndTrim(s string, sep rune) []string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strings.Split should be able to do this for us. It looks like this code is just splitting, not splitting and trimming. And if we do need splitting and trimming, we can just iterate through the slice created by Split and call strings.Trim
| sudo apt-get update | ||
| sudo apt install golang-go | ||
| sudo apt install neovim |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: apt and apt-get are actually separate tools (though for many commands they have essentially the same behavior. We should just use one OR the other and avoid mixing them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oop, meant to Request Changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small remaining thing about the commented-out log statements, but otherwise LGTM.
| if readLocations != nil { | ||
| availReadEndpointsByLocation, availReadLocations, err := getEndpointsByLocation(readLocations) | ||
| log.Printf("Available read endpoints by location: %v", availReadEndpointsByLocation) | ||
| // log.Printf("Available read endpoints by location: %v", availReadEndpointsByLocation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's either remove these altogether or switch them to use the core logging facility. (Basically, I just don't like committing commented-out code ;))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I will remove these. :)
Description
This is adding a way to startup a workload to use for testing in dr drills. The configurations can be set through environment variables. Initially, the script will create a database and container, and an item for the number of logical partitions specified in the configuration files. After this initial setup, concurrent operations should happen on the precreated items until the process is killed. Idea is for this to be the bare minimum and later we can add more things based on customer usage in go like aad, other operations, etc