Update docker compose instructions

[j8r]

The docker-compose file was not working as-is, and instructions were lacking.

I also added a little commit, not related to the above topic, to fix a missing Hash key error for concurrency_1024 and concurrency_2048 I had.

[waghanza]

HI @j8r,

Thanks for your contribution.

However, I do not understand some points :

• Why using docker-compose would be valuable (in this project scope) ?
  • The dump.sql file will be removed (I have started to create a gem to share database structure https://github.com/the-benchmarker/benchmarker-data)
• What is ?.try & stands for ?

Regards,

PS : I'm switching back to ruby, no need in fact to be in crystal

[j8r]

The docker-compose file is very useful to have a quick database up and running, for running benchmark. I can replace it by a docker run command, though - we won't have to install docker-compose.

One another thing lacking, that would be great, is to have this project working without to have to install anything other than Docker.

[j8r]

The Hash[]? get the hash key, or nil if not present. Object#try(&block) executes the block only if the object is not nil.

[j8r]

Do you prefer docker run -it --rm -d -p 5432:5432 -e POSTGRES_DB=benchmark -e POSTGRES_HOST_AUTH_METHOD=trust -v $PWD/.ci/dump.sql:/srv/dump.sql -v /tmp/pg-data:/var/lib/postgresql/data --name pg postgres:12-alpine instead of using a docker-compose?

[waghanza]

Hi @j8r,

You must use

export IP=`docker inspect -f '{{range .NetworkSettings.Networks}}{{.IPAddress}}{{end}}' pg`
export DATABASE_URL="postgresql://postgres@${IP}/benchmark"
sleep 2 
psql -d benchmark -U postgres -h ${IP} < .ci/dump.sql

(I don't know why sleep 2, but it works) instead of

docker exec pg sh -c "echo \"$(cat .ci/dump.sql)\" | psql -U postgres -d benchmark"
export DATABASE_URL="postgresql://postgres@localhost/benchmark"

As you are using localhost on second choice for database hostname, it won't work

Regards,

PS : I'm computing the results right now, see them in ~13 hours

PSS : Thanks for this :tada:, it would facilitate the work on https://github.com/the-benchmarker/benchmarker-data (database models as an external library)

[j8r]

It works for me, I don't understand @waghanza? The port is binded directly to localhost, maybe you have some limitations on your PC.

Furthermore, it is better to execute psql of the container, instead of assuming the user having it.

[j8r]

Ha ok, you may have already a postgresql service running in localhost?! The port can just be changed then.

[waghanza]

I have stopped the service before -> to release the port

Btw, I do not understand why 2 sec. Probably because of my OS (fedora).

However, I don't understand when you say that it is better to run psql on container. Why ?

PS : if it just because of an installation, I can understand, but of client is very lightweight

[j8r]

It is better because it does not requires the user to install it, with its package manager. There is already one on the container, with obviously the perfect supported version - let's use it!

[j8r]

I don't understand either the 2 seconds. Be aware than starting the container take a bit of time to have the PostgreSQL service inside it up and running.

[waghanza]

sure, I have the same "issue" with multiple containers. I mean I can not calculate how time does the container require to be up and running 😛

[j8r]

I add a comment to wait a bit then :+1:

[j8r]

@waghanza what are you doing with the branch O.o ?!

[waghanza]

updating results

[j8r]

That's not related to this PR, please do not edit the commit inside it for this.

[waghanza]

Why ?

I will merge after CI. I've updated results to have fresh ones on master

[j8r]

Because the PR is messy with unrelated modifications, look at the history. Why not just accepting and merging the PR. Then, eventually, do whatever you wish on a PR you create.

That's their purpose. Imagine every admin starts to modify the history and add random commits, this will get quickly messy.

[j8r]

I understand for having up-to-date results, but wouldn't be better to have this changes in a PR named Update benchmark results, rather than this one Update docker compose instructions?

[waghanza]

I understand for having up-to-date results, but wouldn't be better to have this changes in a PR named Update benchmark results, rather than this one Update docker compose instructions?

In can be messy too ... It will then add a lot of commits like that

Please make sure that I have not modified your intentions, is it not the essential on this PR ?

Btw, thanks for your work :heart: