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-898 - Add bulk connections endpoint #948

Merged
merged 11 commits into from
Nov 15, 2019
Merged

MF-898 - Add bulk connections endpoint #948

merged 11 commits into from
Nov 15, 2019

Conversation

nwneisen
Copy link
Contributor

Signed-off-by: nwneisen nwneisen@gmail.com

What does this do?

Adds an endpoint to create bulk connections.

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

Relates to #898.

List any changes that modify/break current functionality

None

Have you included tests for your changes?

Yes

Did you document any new/modified functionality?

Yes

Notes

Signed-off-by: nwneisen <nwneisen@gmail.com>
Signed-off-by: nwneisen <nwneisen@gmail.com>
@nwneisen nwneisen requested a review from a team as a code owner November 12, 2019 04:42
things/api/things/http/requests.go Outdated Show resolved Hide resolved
@nwneisen
Copy link
Contributor Author

@manuio I started switching this code over for multiple channels and I realized that I can only have one variadic parameter. I don't see another way besides making the channelIDs parameter a slice but I thought I'd see if you had any ideas before changing everything over.

@manuio
Copy link
Contributor

manuio commented Nov 13, 2019

@nwneisen I'm not sure either... In any case the service endpoint can accept two slices of strings. But for the service method I'm not sure... Maybe a for loop here svc.Connect(ctx, cr.token, cr.ChanID, cr.ThingIDs...); ?
@drasko @nmarcetic what do you think?

@nwneisen
Copy link
Contributor Author

@manuio I was thinking a for loop would be the way to go without getting caught up in too much optimization. The downside is a for loop on the service method would create multiple transactions; one for each channel. This behavior might not be terrible but I think if one transaction fails then it should be recorded to be returned to the user and the rest allowed to continue.

@nmarcetic
Copy link
Collaborator

@nwneisen @manuio IMHO it's OK to have 2 slices, yes can have only 1 variadic param.
@nwneisen For service method you can do the same as you did for things bulk add and do two for loops on repository level :) So you can rollback in a single transaction if something fails.
It's a bit ugly maybe... But it would be hard to accomplish such bulk easy.

Also what if I want to reverse, to connect 100 things to 1 channel? Not 1 thing to 100 channels.
It didn't work like this before, I know, just thinking aloud...

Any ideas @mainflux/maintainers ?

@anovakovic01
Copy link
Contributor

I vote for a single transaction and two slices.

Signed-off-by: Nick Neisen <nwneisen@gmail.com>
Signed-off-by: Nick Neisen <nwneisen@gmail.com>
bootstrap/mocks/things.go Outdated Show resolved Hide resolved
docs/provisioning.md Outdated Show resolved Hide resolved
things/api/logging.go Outdated Show resolved Hide resolved
things/api/metrics.go Outdated Show resolved Hide resolved
things/api/things/http/endpoint_test.go Outdated Show resolved Hide resolved
things/api/things/http/requests.go Show resolved Hide resolved
Signed-off-by: Nick Neisen <nwneisen@gmail.com>
@codecov-io
Copy link

codecov-io commented Nov 14, 2019

Codecov Report

Merging #948 into master will increase coverage by 0.08%.
The diff coverage is 98.64%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #948      +/-   ##
==========================================
+ Coverage   83.63%   83.71%   +0.08%     
==========================================
  Files          75       75              
  Lines        5248     5288      +40     
==========================================
+ Hits         4389     4427      +38     
- Misses        590      592       +2     
  Partials      269      269
Impacted Files Coverage Δ
things/redis/streams.go 92.3% <100%> (+0.05%) ⬆️
things/api/things/http/transport.go 94.05% <100%> (+0.28%) ⬆️
things/api/things/http/endpoint.go 98.58% <100%> (+0.04%) ⬆️
things/api/things/http/responses.go 97.75% <100%> (+0.19%) ⬆️
things/service.go 88.97% <100%> (-1.48%) ⬇️
things/api/things/http/requests.go 88.88% <100%> (+1.38%) ⬆️
things/postgres/channels.go 79.84% <94.44%> (+0.07%) ⬆️

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 afeec81...4ac942d. Read the comment docs.

Signed-off-by: Nick Neisen <nwneisen@gmail.com>
manuio
manuio previously approved these changes Nov 14, 2019
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

things/service.go Outdated Show resolved Hide resolved
Signed-off-by: Nick Neisen <nwneisen@gmail.com>
manuio
manuio previously approved these changes Nov 14, 2019
docs/provisioning.md Outdated Show resolved Hide resolved
Signed-off-by: Nick Neisen <nwneisen@gmail.com>
drasko
drasko previously approved these changes Nov 15, 2019
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

nmarcetic
nmarcetic previously approved these changes Nov 15, 2019
Signed-off-by: nwneisen <nwneisen@gmail.com>
@nwneisen nwneisen dismissed stale reviews from nmarcetic and drasko via 5b5dd43 November 15, 2019 17:27
dborovcanin
dborovcanin previously approved these changes Nov 15, 2019
things/api/logging.go Outdated Show resolved Hide resolved
Signed-off-by: nwneisen <nwneisen@gmail.com>
@nmarcetic
Copy link
Collaborator

LGTM! Good job @nwneisen

@nmarcetic nmarcetic merged commit 894e1b8 into absmach:master Nov 15, 2019
@nwneisen nwneisen deleted the MF-898 branch November 15, 2019 17:44
manuio pushed a commit that referenced this pull request Oct 12, 2020
* Change connect endpoint to use bulk connections

Signed-off-by: nwneisen <nwneisen@gmail.com>

* Add documentation for bulk connect

Signed-off-by: nwneisen <nwneisen@gmail.com>

* Update service and postgres for multiple channels

Signed-off-by: Nick Neisen <nwneisen@gmail.com>

* Update bulk connect endpoint

Signed-off-by: Nick Neisen <nwneisen@gmail.com>

* Fix missed test

Signed-off-by: Nick Neisen <nwneisen@gmail.com>

* Improve test code coverage

Signed-off-by: Nick Neisen <nwneisen@gmail.com>

* Change methods to use shared data type

Signed-off-by: Nick Neisen <nwneisen@gmail.com>

* Fix provisioning example

Signed-off-by: Nick Neisen <nwneisen@gmail.com>

* Change depreciation messages to specify v1.0.0

Signed-off-by: nwneisen <nwneisen@gmail.com>

* Switch singluar log to plural

Signed-off-by: nwneisen <nwneisen@gmail.com>
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.

7 participants