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

MF-1478 - TimescaleDB writer and reader add-on #1542

Merged
merged 8 commits into from
Jan 18, 2022

Conversation

corp0529
Copy link
Contributor

@corp0529 corp0529 commented Jan 11, 2022

What does this do?

Adds TimescaleDB as a data store

Which issue(s) does this PR fix/relate to?

Resolves #1478

Have you included tests for your changes?

Yes

Did you document any new/modified functionality?

Yes

@corp0529 corp0529 requested a review from a team as a code owner January 11, 2022 17:33
@manuio
Copy link
Contributor

manuio commented Jan 11, 2022

@corp0529 Looks like you didn't update vendor (go mod vendor). Can you also check why CI is failling?

@drasko
Copy link
Contributor

drasko commented Jan 11, 2022

@corp0529 thanks for this very useful addition! I will review PR carefully in the next few days. @dusanb94, @blokovi, @mteodor and @blokovi please review as well.

@corp0529
Copy link
Contributor Author

@manuio Sure, just fixing the CI now, it was the tests not running properly.

@codecov-commenter
Copy link

codecov-commenter commented Jan 11, 2022

Codecov Report

Merging #1542 (f0573e1) into master (bea09d9) will increase coverage by 0.30%.
The diff coverage is 80.13%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1542      +/-   ##
==========================================
+ Coverage   67.18%   67.48%   +0.30%     
==========================================
  Files         135      139       +4     
  Lines       11041    11319     +278     
==========================================
+ Hits         7418     7639     +221     
- Misses       3015     3050      +35     
- Partials      608      630      +22     
Impacted Files Coverage Δ
consumers/writers/timescale/consumer.go 66.99% <66.99%> (ø)
readers/timescale/messages.go 84.84% <84.84%> (ø)
consumers/writers/timescale/init.go 89.47% <89.47%> (ø)
readers/timescale/init.go 89.47% <89.47%> (ø)
consumers/writers/postgres/consumer.go 65.76% <100.00%> (ø)

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 bea09d9...f0573e1. Read the comment docs.

@corp0529
Copy link
Contributor Author

@manuio @drasko I've run go mod vendor and there's almost 4k changes in the commit. Shall I leave it as a separate commit or squash with the previous one?

@drasko
Copy link
Contributor

drasko commented Jan 11, 2022

@manuio @drasko I've run go mod vendor and there's almost 4k changes in the commit. Shall I leave it as a separate commit or squash with the previous one?

Vendor needs to go in the same PR. Just update your branch with the new commit, all commits are squashed upon merge.

@corp0529
Copy link
Contributor Author

@manuio @drasko I've run go mod vendor and there's almost 4k changes in the commit. Shall I leave it as a separate commit or squash with the previous one?

Vendor needs to go in the same PR. Just update your branch with the new commit, all commits are squashed upon merge.

It's actually showing no changes after go mod vendor. I've tried a clean pull and go mod vendor and no change.
Everything builds and tests run fine.

@manuio
Copy link
Contributor

manuio commented Jan 12, 2022

@corp0529 My bad, looks like there are no new packages used! Thank you for verifying!

@corp0529 corp0529 requested a review from manuio January 12, 2022 14:27
@dborovcanin dborovcanin changed the title MF-1478 - TimescaleDb Reader/Writer Add-on MF-1478 - TimescaleDB writer and reader add-on Jan 13, 2022
@dborovcanin
Copy link
Collaborator

@corp0529 Please resolve all the comments and update the branch. Once you do it, we'll do one more review and it's ready for merge.

drasko
drasko previously approved these changes Jan 17, 2022
Copy link
Contributor

@drasko drasko left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@dborovcanin dborovcanin left a comment

Choose a reason for hiding this comment

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

After one minor remark is resolved, I'll approve. Great job, @corp0529! Thanks a lot for this contribution.

dborovcanin
dborovcanin previously approved these changes Jan 18, 2022
Copy link
Contributor

@manuio manuio left a comment

Choose a reason for hiding this comment

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

LGTM Thank you!

@dborovcanin dborovcanin merged commit 864ad14 into absmach:master Jan 18, 2022
bioinformatx pushed a commit to bioinformatx/mainflux that referenced this pull request Jan 28, 2022
* MF-1478 - TimescaleDb Reader/Writer Add-on

Signed-off-by: John Cleasby <[email protected]>

* pull request 1542 change requests

Signed-off-by: corp0529 <[email protected]>

* pull request 1542 change requests 2

Signed-off-by: corp0529 <[email protected]>

* removed unused separator const from timescale reader and writer cmd

Signed-off-by: corp0529 <[email protected]>

* Fixed naming of timescaleRepo instance

Signed-off-by: corp0529 <[email protected]>

* Fixed indentation and renamed repo to tr

Signed-off-by: corp0529 <[email protected]>
Signed-off-by: skovacevic <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TimescaleDB writer and reader add-on
5 participants