-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add s3 service performance tests #3470
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?
Conversation
* @param availabilityZoneId Availability zone ID for S3 Express buckets | ||
* @param iterations Number of put/get operations to perform | ||
*/ | ||
void RunTest(Aws::S3::S3Client& s3, const TestCase& config, const Aws::String& availabilityZoneId, int iterations = 3); |
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.
we should represent a performance test as a class not as unstructured functions in a namespace, also we should give some structure to them. i.e.
#include <memory>
class PerformanceTest {
public:
virtual ~PerformanceTest() = default;
virtual void Setup() = 0;
virtual void TearDown() = 0;
virtual void Run() = 0;
};
class S3PerformanceTest : public PerformanceTest {
public:
~S3PerformanceTest() override;
void Setup() override {
//Do setup
};
void TearDown() override {
//Do teardown
}
void Run() override {
//Run the test
}
};
int main() {
auto performanceTest = std::make_unique<PerformanceTest>();
performanceTest->Setup();
performanceTest->Run();
performanceTest->TearDown();
}
This is beneficial as when we add more services, we have more subclasses, and the "runner" is aware of the interface and not the actual implementation.
Aws::String commitId = "unknown"; | ||
int iterations = 10; | ||
|
||
for (int i = 1; i < argc; ++i) { |
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.
since this is a test and not SDK code we should feel free to use a real arg parser instead of hand coding one. i personally like cxxopts.
* Get the current SDK version as a formatted string. | ||
* @return SDK version in the format "major.minor.patch" (e.g., "1.11.0") | ||
*/ | ||
static inline Aws::String GetSDKVersionString() { |
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.
why cant you just use GetVersionString
from aws/core/Version.h
Aws::Client::ClientConfiguration cfg; | ||
cfg.region = region; | ||
|
||
Aws::S3::S3Client s3(cfg); |
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.
initializing the client should likely be scoped to the instance of the PerformanceTest
object, main
should just make a decision which performance test object to create and run it
* @param operation The name of the operation that failed | ||
* @param message The error message to log | ||
*/ | ||
static inline void LogError(const Aws::String& service, const Aws::String& operation, const Aws::String& message) { |
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.
instead of writing to std::err, lets use the macro AWS_LOG_ERROR
from aws/core/utils/logging/LogMacros.h
namespace Services { | ||
namespace S3 { | ||
namespace TestConfig { | ||
const Aws::Set<Aws::String> TestOperations = {"PutObject", "GetObject"}; |
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.
Aws::Set
/Aws::Vector
/Aws::String
should not be used in this case because the static, and known at compilation time, they will not be added to at runtime and therefor do not need to be allocator aware.
something like
struct Widget {
const char* name;
const int value;
};
namespace {
const std::array<const char*, 4> my_array{"foo", "bar"};
const std::array<const Widget, 2> m_widgets{{"name", 64}};
}
is more preferable as they are statically sized and defined
#include <performance-tests/services/s3/S3PerformanceTest.h> | ||
#include <performance-tests/services/s3/S3TestConfig.h> | ||
|
||
#include <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.
why std::string
? maybe a unused import
Description of changes:
This PR adds performance tests for the s3 service. It tests operations including PutObject and GetObject with multiple test dimensions: bucket type and object size.
Check all that applies:
Check which platforms you have built SDK on to verify the correctness of this PR.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.