-
Notifications
You must be signed in to change notification settings - Fork 49
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
feat: add Quickperf for simple performance testing with JDBC #1619
Conversation
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use -- conventional-commit-lint bot |
@@ -0,0 +1,9 @@ | |||
{ | |||
"project": "xxxx", |
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: indent
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.
fixed
samples/quickperf/pom.xml
Outdated
<dependency> | ||
<groupId>com.google.cloud</groupId> | ||
<artifactId>libraries-bom</artifactId> | ||
<version>26.34.0</version> |
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: update to the latest version (26.38.0)
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.
updated and moved to dependencyManagement
samples/quickperf/pom.xml
Outdated
<groupId>junit</groupId> | ||
<artifactId>junit</artifactId> | ||
<version>4.11</version> | ||
<scope>test</scope> |
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.
The convention is to list all compile time dependencies first, and then all test dependencies. So move this further down in the pom file.
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.
moved to test dependency group
samples/quickperf/pom.xml
Outdated
<dependency> | ||
<groupId>com.google.cloud</groupId> | ||
<artifactId>libraries-bom</artifactId> | ||
<version>26.34.0</version> | ||
<type>pom</type> | ||
<scope>import</scope> | ||
</dependency> |
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 should not be here. Importing a pom is something that you should only do in the dependencyManagement
section above.
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.
removed - its already in the dependencyManagement
samples/quickperf/src/main/java/com/google/cloud/jdbc/quickperf/QuickPerf.java
Outdated
Show resolved
Hide resolved
return retVal; | ||
} | ||
|
||
// Getters and setters |
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: remove superfluous comment
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.
removed
samples/quickperf/src/main/java/com/google/cloud/jdbc/quickperf/config/Config.java
Show resolved
Hide resolved
.withExposedPorts(9010) | ||
.waitingFor(Wait.forListeningPort()); | ||
|
||
emulator.setPortBindings(ImmutableList.of("9010:9010")); |
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.
See my comment further above. You don't have to force the emulator to use port 9010, you can also let it run on a random port, and just add localhost:<port>
to the connection URL. The autoConfigEmulator=true
flag will still work as it does now, but will use the host/port in the connection URL instead of the default localhost:9010
.
SpannerOptions options = SpannerOptions.newBuilder() | ||
.setProjectId(projectId) | ||
.setEmulatorHost("localhost:" + emulator.getMappedPort(9010)) | ||
.setCredentials(NoCredentials.getInstance()) | ||
.build(); | ||
|
||
Spanner spanner = options.getService(); | ||
dbAdminClient = spanner.getDatabaseAdminClient(); | ||
|
||
createInstance(projectId, instanceId); | ||
|
||
OperationFuture<Database, CreateDatabaseMetadata> op = dbAdminClient.createDatabase( | ||
instanceId, | ||
databaseId, | ||
ddlList); | ||
|
||
System.out.println("Creating database " + databaseId); | ||
Database dbOperation = op.get(); | ||
System.out.println("Created database [" + databaseId + "]"); |
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 can replace all of this (and also remove the entire createInstance
method) by using the JDBC driver for doing this as well. The autoConfigEmulator=true
flag will automatically create the instance and the database that you specify in the JDBC connection URL, so all you need to do is something like this:
try (Connection connection = DriverManager.getConnection("jdbc:cloudspanner:/projects/%s/instances/%s/databases/%s?autoConfigEmulator=true")) {
try (Statement statement = connection.createStatement()) {
for (String ddl : ddlList) {
statement.addBatch(ddl);
}
statement.executeBatch();
}
}
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.
fixed
…f/ProgressTracker.java Co-authored-by: Knut Olav Løite <koloite@gmail.com>
…f/QuickPerf.java Co-authored-by: Knut Olav Løite <koloite@gmail.com>
…f/QuickPerf.java Co-authored-by: Knut Olav Løite <koloite@gmail.com>
Co-authored-by: Knut Olav Løite <koloite@gmail.com>
…f/ProgressTracker.java Co-authored-by: Knut Olav Løite <koloite@gmail.com>
…f/ProgressTracker.java Co-authored-by: Knut Olav Løite <koloite@gmail.com>
…f/ProgressTracker.java Co-authored-by: Knut Olav Løite <koloite@gmail.com>
samples/quickperf/readme.md
Outdated
|
||
**Run:** | ||
``` | ||
mvn -q exec:java -Dexec.mainClass="com.google.cloud.jdbc.quickperf.QuickPerf" \ |
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.
What I meant was that you can then also remove the -Dexec.mainClass="..."
from the above command as well, making it a bit easier to run the application.
samples/quickperf/readme.md
Outdated
* Set `project` and `instance` in each of the config JSON files located under `exampleconfigs/users/users_config.json` | ||
*`exampleconfigs/users/users_config.json` | ||
*`exampleconfigs/users/groupmgt_config.json` | ||
*`exampleconfigs/users/membership_config.json` | ||
*`exampleconfigs/users/loadtestusers.json` | ||
* |
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 does not appear to format correctly. See https://github.com/googleapis/java-spanner-jdbc/blob/60a355aec261f0174c19688058b22d1a71e19492/samples/quickperf/readme.md#end-to-end-example
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.
fixed
try { | ||
Thread.sleep(sleeptime); | ||
} catch (InterruptedException e) { | ||
e.printStackTrace(); |
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.
That is true, but an InterruptedException
means that the thread should stop executing. So this should either:
- Throw a
RuntimeException
to force an exit of the thread. - Return a value that indicates to the caller that it should stop (so for example return a boolean
true
as in 'should stop', and then have anif
block in the calling loop that checks what was returned, and if true, stops the loop.
if (config.getSamplingQuery() != null) { | ||
thread.runSampling(); | ||
} |
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 understand that. But what happens here is that:
- The application runs the sampling query for
QuickPerfRunner
number 1. That takes let's say 2 seconds. QuickPerfRunner
number 1 is started and starts executing the benchmark.- Step 1 is then repeated for
QuickPerfRunner
number 2. This then also takes 2 seconds. - Etc.... Meaning that if you have 10 threads, it takes 20 seconds before the 10th thread starts. That again means that you can have a relatively significant delay between the time that the first thread starts and the last thread starts, which again can have an impact on the results that you see (you probably also get the same effect at the end of the test run, as the first thread is likely to finish well ahead of the last thread).
fixed markdown issues for indented bullet points
removed -Dexec.args from readme
separated thread init (for sampling) from thread execution
added InterruptedException handling
Adding JDBC quickperf tool for simple performance testing Spanner schemas