Skip to content
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

Merged
merged 24 commits into from
Aug 20, 2024

Conversation

srozsnyai
Copy link
Contributor

Adding JDBC quickperf tool for simple performance testing Spanner schemas

@srozsnyai srozsnyai requested review from a team as code owners May 24, 2024 14:30
@product-auto-label product-auto-label bot added size: xl Pull request size is extra large. api: spanner Issues related to the googleapis/java-spanner-jdbc API. labels May 24, 2024
@olavloite olavloite changed the title feat:Adding Quickperf for simple performance testing with JDBC feat: add Quickperf for simple performance testing with JDBC May 28, 2024
Copy link

🤖 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 automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@@ -0,0 +1,9 @@
{
"project": "xxxx",
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: indent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

samples/quickperf/readme.md Show resolved Hide resolved
<dependency>
<groupId>com.google.cloud</groupId>
<artifactId>libraries-bom</artifactId>
<version>26.34.0</version>
Copy link
Collaborator

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)

Copy link
Contributor Author

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

<groupId>junit</groupId>
<artifactId>junit</artifactId>
<version>4.11</version>
<scope>test</scope>
Copy link
Collaborator

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.

Copy link
Contributor Author

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

Comment on lines 44 to 50
<dependency>
<groupId>com.google.cloud</groupId>
<artifactId>libraries-bom</artifactId>
<version>26.34.0</version>
<type>pom</type>
<scope>import</scope>
</dependency>
Copy link
Collaborator

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.

Copy link
Contributor Author

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

return retVal;
}

// Getters and setters
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: remove superfluous comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

.withExposedPorts(9010)
.waitingFor(Wait.forListeningPort());

emulator.setPortBindings(ImmutableList.of("9010:9010"));
Copy link
Collaborator

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.

Comment on lines 116 to 134
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 + "]");
Copy link
Collaborator

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();
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

srozsnyai and others added 13 commits July 1, 2024 11:58
…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 Show resolved Hide resolved

**Run:**
```
mvn -q exec:java -Dexec.mainClass="com.google.cloud.jdbc.quickperf.QuickPerf" \
Copy link
Collaborator

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.

Comment on lines 107 to 112
* 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`
*
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

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();
Copy link
Collaborator

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 an if block in the calling loop that checks what was returned, and if true, stops the loop.

Comment on lines 98 to 100
if (config.getSamplingQuery() != null) {
thread.runSampling();
}
Copy link
Collaborator

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:

  1. The application runs the sampling query for QuickPerfRunner number 1. That takes let's say 2 seconds.
  2. QuickPerfRunner number 1 is started and starts executing the benchmark.
  3. Step 1 is then repeated for QuickPerfRunner number 2. This then also takes 2 seconds.
  4. 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).

srozsnyai and others added 8 commits July 16, 2024 12:15
@olavloite olavloite added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 20, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 20, 2024
@olavloite olavloite added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 20, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 20, 2024
@olavloite olavloite merged commit b6bbd8f into googleapis:main Aug 20, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/java-spanner-jdbc API. size: xl Pull request size is extra large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants