timescale / tsbs

Time Series Benchmark Suite, a tool for comparing and evaluating databases for time series data

MIT License

1.29k stars 300 forks source link

Added connection DB flag to tsbs_load_timescaledb #60

Closed antekresic closed 5 years ago

CLAassistant commented 5 years ago

All committers have signed the CLA.

codecov-io commented 5 years ago

Codecov Report

Merging #60 into master will increase coverage by 0.05%. The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master      #60      +/-   ##
==========================================
+ Coverage   52.23%   52.29%   +0.05%     
==========================================
  Files          73       73              
  Lines        3530     3536       +6     
==========================================
+ Hits         1844     1849       +5     
- Misses       1672     1673       +1     
  Partials       14       14

Impacted Files	Coverage Δ
cmd/tsbs_load_timescaledb/creator.go	`33.33% <100%> (+1.15%)`	:arrow_up:
cmd/tsbs_load_timescaledb/main.go	`53.22% <75%> (+1.5%)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 075e29f...ee7d3de. Read the comment docs.

RobAtticus commented 5 years ago

This functionality is already exposed via the --db-name flag, no?:

$ tsbs_load_timescaledb --help
Usage of tsbs_load_timescaledb:
...
  -db-name string
        Name of database (default "benchmark")
...

(The flag is defined/exists in the load pkg since it is shared across loaders)

RobAtticus commented 5 years ago

Ah I think this is supposed to solve a different problem -- connecting when you want to create the original database? May need to work on some naming issues here.

antekresic commented 5 years ago

Yeah, this is regarding issue #39. I thought that as well at first but I guess people need something like this when connecting to the database where they have to use a custom database name.

How about --custom-conn-db? Or should we change the other flag name to make it more distinct from this one?

RobAtticus commented 5 years ago

Let's go with --admin-db-name and a description like:

Database to connect to in order to create additional benchmark databases. 
By default this is the same as the `db-user` (i.e., `postgres` if neither is set), 
but sometimes a user does not have its own database.

A bit of a mouthful, but gets it across. Then maybe update the commit message to give an extended explaining the reason/need for this change.

Appreciate the PR!

antekresic commented 5 years ago

Updated PR based on the recent comment.

Let me know if there is anything else that needs changing.

Thanks!

RobAtticus commented 5 years ago

Hi @antekresic, I think you need to rebase your branch on the latest (rather than merge ours into yours) because now there are some duplicated commits in your PR. And then squash all your commits into one.

antekresic commented 5 years ago

Hey @RobAtticus

Sorry about that, I took a wrong turn rebasing the branch previously. I believe its fixed now.