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

HL-1205 Postgres support #55

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
package com.atlassian.performance.tools.infrastructure.api.database

import com.atlassian.performance.tools.infrastructure.DockerImage
import com.atlassian.performance.tools.infrastructure.api.dataset.DatasetPackage
import com.atlassian.performance.tools.ssh.api.SshConnection
import org.apache.logging.log4j.LogManager
import org.apache.logging.log4j.Logger
import java.net.URI
import java.time.Duration

class PostgresDatabase(
Copy link
Contributor

Choose a reason for hiding this comment

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

If you can avoid the FileArchiver changes, perhaps by inlining some code, then this class wouldn't have to be merged it, but could be used in-place.

private val source: DatasetPackage,
private val maxConnections: Int,
private val dataBaseVersion: String
) : Database {
private val logger: Logger = LogManager.getLogger(this::class.java)

private val image: DockerImage = DockerImage(
name = "postgres:$dataBaseVersion",
pullTimeout = Duration.ofMinutes(5)
)

constructor(
Copy link
Contributor

Choose a reason for hiding this comment

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

source: DatasetPackage,
dataBaseVersion: String = "9.6"
dagguh marked this conversation as resolved.
Show resolved Hide resolved
) : this(
source = source,
maxConnections = 200,
dataBaseVersion = dataBaseVersion
)

override fun setup(ssh: SshConnection): String {
val pgData = source.download(ssh)
val containerName = image.run(
ssh = ssh,
parameters = "-p 3306:5432 -v `realpath $pgData`:/var/lib/postgresql/data",
arguments = "-c 'listen_addresses='*'' -c 'max_connections=$maxConnections'"
dagguh marked this conversation as resolved.
Show resolved Hide resolved
)
Thread.sleep(Duration.ofSeconds(15).toMillis())
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be great to wait on a condition instead of thread sleep. We're either loose time waiting, or return too fast. And it can cause flakiness.

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a red flag. @masiorski have you checked it there is any way to actually wait for the image to be ready rather than sleep?

Copy link
Author

Choose a reason for hiding this comment

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

I think we do not need this line here, I will remove it, thanks

return pgData
}

override fun start(jira: URI, ssh: SshConnection) {
// TODO Check logs for the following entry
// LOG: database system is ready to accept connections
Thread.sleep(Duration.ofSeconds(15).toMillis())
Copy link
Contributor

Choose a reason for hiding this comment

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

This sleep is suspicious too. How do we resolve the TODO?

  • resolve TODO
  • remove sleep

Copy link
Author

Choose a reason for hiding this comment

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

sleep is enough here, postgres databases are initialising very quickly, checking logs is something nice to have

}

}
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,11 @@ class FileArchiver {
) {
ubuntu.install(connection, listOf("lbzip2"))
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have to install lbzip2 in case we're not using it.

Copy link
Author

Choose a reason for hiding this comment

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

thanks for pointing this out

time("unzip") {
connection.execute("tar -I lbzip2 -xf $archive", timeout)
val cmd = if (archive.contains("SCALED_ISSUES_UNIMODAL_PG"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change related to "Postgres support"?

Copy link
Author

Choose a reason for hiding this comment

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

yes sir, I keep data sets for PG here - https://s3.console.aws.amazon.com/s3/buckets/jpt-pgdata/?region=eu-central-1&tab=overview
we need to move the archives to the correct S3 bucket (but I do not have rights there, need your help)

"tar -xzvf $archive"
else
"tar -I lbzip2 -xf $archive"
connection.execute(cmd, timeout)
}
}

Expand Down