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.

Sign up for GitHub

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

Jump to bottom

Fix parsing columns when --do-create-db=false #70

Merged
merged 1 commit into from
Apr 25, 2019
Merged

Fix parsing columns when --do-create-db=false #70

merged 1 commit into from
Apr 25, 2019

Conversation

LeeHampton
Copy link
Contributor

No description provided.

@LeeHampton LeeHampton requested a review from RobAtticus April 24, 2019 18:46
@LeeHampton
Copy link
Contributor Author

@RobAtticus Before I write a series of new tests for this, I wanted to make sure this refactor looks like a decent path to go down to you.

Basically, I've split the PostCreateDB method into 2 different methods, getFieldAndIndexDefinitions definitions for parsing the generated data headers and coming up with field and index definitions, and createTableAndIndexes for running all of the DB commands based on the field/index definitions and user settings.

This makes it easier to make proper use of the createMetricsTable flag (the bug this ticket actually fixes), and it should make things more testable. I've also renamed a bunch of the old fields to be a little more clear (no more parts variable name...).

@RobAtticus
Copy link
Member

Yup this looks reasonable to me. Separate from this PR we should try and clean up the global state -- seems to be a bit of a mess to keep track of.

@LeeHampton
Copy link
Contributor Author

Definitely agreed on that one @RobAtticus

@LeeHampton LeeHampton force-pushed the lee/fix-post-create branch from 90de05f to ea361de Compare April 25, 2019 17:18
@LeeHampton LeeHampton changed the title [wip] Fix parsing columns when --do-create-db=false; refactor PostCre… Fix parsing columns when --do-create-db=false Apr 25, 2019
@LeeHampton LeeHampton force-pushed the lee/fix-post-create branch from ea361de to 2e17fe0 Compare April 25, 2019 17:24
@codecov-io
Copy link

Codecov Report

Merging #70 into master will increase coverage by 0.47%.
The diff coverage is 32.3%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #70      +/-   ##
==========================================
+ Coverage   53.46%   53.93%   +0.47%     
==========================================
  Files          76       76              
  Lines        3713     3719       +6     
==========================================
+ Hits         1985     2006      +21     
+ Misses       1706     1690      -16     
- Partials       22       23       +1
Impacted Files Coverage Δ
cmd/tsbs_load_timescaledb/creator.go 40.68% <32.3%> (+13.35%) ⬆️

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 addc791...ea361de. Read the comment docs.

@codecov-io
Copy link

codecov-io commented Apr 25, 2019 •
edited
Loading

Codecov Report

Merging #70 into master will increase coverage by 0.47%.
The diff coverage is 32.3%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #70      +/-   ##
==========================================
+ Coverage   53.46%   53.93%   +0.47%     
==========================================
  Files          76       76              
  Lines        3713     3719       +6     
==========================================
+ Hits         1985     2006      +21     
+ Misses       1706     1690      -16     
- Partials       22       23       +1
Impacted Files Coverage Δ
cmd/tsbs_load_timescaledb/creator.go 40.68% <32.3%> (+13.35%) ⬆️

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 addc791...3f5664d. Read the comment docs.

@LeeHampton
Copy link
Contributor Author

@RobAtticus Added tests. This should be ready for review now.

RobAtticus
RobAtticus approved these changes Apr 25, 2019
View reviewed changes
Copy link
Member

@RobAtticus RobAtticus left a comment

Choose a reason for hiding this comment

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

LGTM, if it's not too much trouble I might just suggest adding one more case for 2 fields indexes. Since we use a special value for all and 1 can sometimes be a special case, just want to make sure something works in the general case.

Not strictly necessary though.

@LeeHampton
Fix parsing columns when --do-create-db=false
3f5664d 
This fixes a bug where the PostCreateDB function would exit early when
the user set --do-create-db=false and/or --create-metrics-table=False.
This early exit caused TSBS to skip the updating of some global caches,
which broke assumptions in other parts of the codebase.

This commit also refactors the PostCreateDB function to split the
parsing of columns and the potential creation of tables and indexes into
separate functions. This makes it easier to test the functions in
isolation and cleaner to create the conditional create-table logic that is
at the heart of this bug.

While this does add tests to the parsing function, the create
tables/index function remains untested. This is left for a later PR that
will hopefully clean up global state and provide a more comprehensive
framework for testing IO.
@LeeHampton LeeHampton force-pushed the lee/fix-post-create branch from 2e17fe0 to 3f5664d Compare April 25, 2019 18:21
@LeeHampton
Copy link
Contributor Author

Good call @RobAtticus -- done.

@LeeHampton LeeHampton merged commit 19932e7 into master Apr 25, 2019
@LeeHampton LeeHampton deleted the lee/fix-post-create branch April 25, 2019 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RobAtticus RobAtticus RobAtticus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@LeeHampton @RobAtticus @codecov-io