Conversation
masiorski
requested review from
dagguh,
mcmurphy2000,
sebapawlak and
wyrzyk
as code owners
| pullTimeout = Duration.ofMinutes(5) | ||
| ) | ||
|
|
||
| constructor( |
There was a problem hiding this comment.
Please consider builder instead of telescoping constructors and default params. See https://github.com/wyrzyk/java-api-design-checklist/blob/master/CHECKLIST.md#3612-consider-a-builder-when-faced-with-many-constructor-parameters
| parameters = "-p 3306:5432 -v `realpath $pgData`:/var/lib/postgresql/data", | ||
| arguments = "-c 'listen_addresses='*'' -c 'max_connections=$maxConnections'" | ||
| ) | ||
| Thread.sleep(Duration.ofSeconds(15).toMillis()) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I think we do not need this line here, I will remove it, thanks
| ubuntu.install(connection, listOf("lbzip2")) | ||
| time("unzip") { | ||
| connection.execute("tar -I lbzip2 -xf $archive", timeout) | ||
| val cmd = if (archive.contains("SCALED_ISSUES_UNIMODAL_PG")) |
There was a problem hiding this comment.
Is this change related to "Postgres support"?
There was a problem hiding this comment.
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)
| @@ -18,7 +18,11 @@ class FileArchiver { | |||
| ) { | |||
| ubuntu.install(connection, listOf("lbzip2")) | |||
There was a problem hiding this comment.
We don't have to install lbzip2 in case we're not using it.
|
Please update CHANGELOG. |
|
I'm assuming HL-1205 is internal project. Is so there is no point in mentioning it in an open-source repo. You should create a JPREF ticket for this work and reference it. |
dagguh
left a comment
dagguh
left a comment
There was a problem hiding this comment.
Overall looks ok, but please test this.
src/main/kotlin/com/atlassian/performance/tools/infrastructure/api/database/PostgresDatabase.kt
Outdated
Show resolved
Hide resolved
| 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()) |
There was a problem hiding this comment.
This sleep is suspicious too. How do we resolve the TODO?
- resolve TODO
- remove sleep
There was a problem hiding this comment.
sleep is enough here, postgres databases are initialising very quickly, checking logs is something nice to have
CHANGELOG.md
Outdated
| [Unreleased]: https://github.com/atlassian/infrastructure/compare/release-4.14.4...master | ||
| [Unreleased]: https://github.com/atlassian/infrastructure/compare/release-4.14.5...master | ||
|
|
||
| ## [4.14.5] - 2020-02-03 |
There was a problem hiding this comment.
We'll mark the release after it happens, so you can remove this header
There was a problem hiding this comment.
More precisely, remove the entire changelog diff except for the actual change you're shipping:
### Added
- Add support for Postgres database.
src/main/kotlin/com/atlassian/performance/tools/infrastructure/api/database/PostgresDatabase.kt
Show resolved
Hide resolved
| import java.net.URI | ||
| import java.time.Duration | ||
|
|
||
| class PostgresDatabase( |
There was a problem hiding this comment.
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.
4 participants
Please review and release this PR. I'm leaving atlassian in February. I would like to finish PG support before my leave. Regards, Marcin