extend ListServices api to filter by running services, closes #496 #499

ilgooz · 2018-09-25T08:39:25Z

coreapi.Service type now has IsRunning field, relevant APIs accordingly updated to set this value.
update dev-core script.
update database API to filter services by id, add more tests.

* coreapi.Service type know has IsRunning field, relevant APIs accordingly updated to set this value. * update dev-core script. * update database API to filter services by id, add more tests.

ilgooz · 2018-09-25T08:46:46Z

some options about setting isRunning field on Service proto:

implement StatusType on Service proto definition for detailed status info or only keep isRunning state.
we explicitly call service.Status() on GetService() grpc api but we don't do the same for ListServices(). I prefer explictily calling service.Status() for ListServices() too because we can get much more detailed status info, what do you think?

krhubert · 2018-09-25T08:53:37Z

api/api.go

@@ -9,18 +10,23 @@ import (
 type API struct {
 	db        *database.ServiceDB
 	container *container.Container
+	cfg       *config.Config


There is already issue #495 that you've created, about not storing config inside Container.

It should be passed as args/option to New func.

yes, it is better to not access config inside container package. in this case we access it from api package.

actually i'd rather doing all this initilazitions in the core package and propagate it down to initialization of grpc server & api. this way we can reuse everything.

#502

krhubert · 2018-09-25T08:54:39Z

api/api_test.go

@@ -10,7 +10,7 @@ import (
 	"github.com/stretchr/testify/require"
 )

-const testdbname = "db.test"
+const testdbname = "/tmp/db.test"


What is the purpose of changing the path?

Not every OS (windows for example) has /tmp dir (or not every user has access to /tmp dir)

Also if you want tmp dir it's better to use https://golang.org/pkg/os/#TempDir combine with filepath.Join then const path

changes reverted from this PR

krhubert · 2018-09-25T08:59:55Z

database/service_db.go

@@ -64,7 +65,8 @@ func (db *ServiceDB) unmarshal(id string, value []byte) (*service.Service, error
 }

 // All returns every service in database.
-func (db *ServiceDB) All() ([]*service.Service, error) {
+// If ids > 0, it'll only return services with matching ids.


It's better to keep the functionality of All as it's was by original.

You can add an GetByIDs or similar method, to keep API more transparent

antho1404 · 2018-09-25T09:07:53Z

api/api.go

 }

 // Option is a configuration func for MESG.
 type Option func(*API)

 // New creates a new API with given options.
-func New(db *database.ServiceDB, options ...Option) (*API, error) {
-	a := &API{db: db}
+func New(options ...Option) (*API, error) {


The API should be in parameter here, it's a required arguments so it has to be here. If we call New() without any options (what most developers will do without knowing the codebase) we will have some crash in the code.

Of course you add the error in the end but what's the point to remove a required argument and add a error, it will just be more confusing. I like the pattern to use options but for what is optional and not the stuff required, it doesn't make sense.

Also and more important, let's keep the pull requests simple. This has nothing to do here, it's a pull request about extending the API why do we have some stuff in the database. Especially that this modifications has been done in another pull request that was just merged today and stayed open for more than a week so for this kind of things it better to talk about this in the other PR and not try to include that in a non related PR.

changes reverted from this PR

ilgooz · 2018-09-26T07:10:57Z

lets discuss #499 (comment)

antho1404

I like the part to have the status for the API but I don't think we should do a filter like that. We will need to have a filter for this and maybe for name after or the task or whatever so let's not rush on a feature that is not really necessary and will block us.

Also the part to remove the services when we stop the core is something that the cli should not do but the core itself should when it exits.

I think we should do the part for the status of the service but the part for the filter we should take some time to make this reusable kind of like docker manage it's filters and for the part of deleting the services when we stop, this in the end should not even be in the cli so we don't need this api to only have the running services and most of the things in this PR will no longer be needed like the config for example in the API struct

interface/grpc/core/list_services.go

krhubert · 2018-09-26T09:21:37Z

interface/grpc/core/list_services.go

+
+	var errs xerrors.Errors
+
+	for i := 0; i < servicesLen; i++ {


I see this pattern once more - publish errors (also nil) and read it from channel to synchronize finishing of goroutines.

Think of using WaitGroup , because they are standard go pattern of sync gorouteines.

Check out an example with a channel:

buffered - https://play.golang.org/p/l0gW2g-xIR8

unbuffered - https://play.golang.org/p/ou9ZWoyT3PE

+1 for the wait group, I think it makes the code more readable than a for that is waiting for some events in the chan.

@krhubert I don't get the difference between the buffered and unbuffered version. I would go myself on the first one, what will be the advantage of using the second one ?

thanks, i cannot see an advantage of using WaitGroup here except using range syntax while receiving from errC which makes readability slightly better. also check std package example.

lets give a decision about which way to go. i'm neutral about this, i see that using range has an upside for readability but it also introduces more code.

The example you gave it's slightly different. Here they don't wait for goroutines to finish but provide timeout for TLSHandshake execution. WIthout timeout, there will be no point to put TLSHandshake inside goroutine.

yes, i agree that it is slightly different and using only chans seems the easiest way for that case. they run the code inside 2 goroutines for sure and there is a possibility for both of them to complete. lets also check this one.

I also suggest to use a WaitGroup to close the chan and use a simple range to iterate over the errors. (this example from @krhubert https://play.golang.org/p/l0gW2g-xIR8)
I suggest to avoid the classic for i := 0; i < len(arr); i++ { and anything that directly deal with index on array. It's a source of error.

do three of you have an agreement on this? please like this comment, i'll switch to WaitGroup if i see likes >= 2. :)

About the second example, the code is inside the test package and it's quite old.

8e69d43b32b src/net/net_test.go (Brad Fitzpatrick 2016-09-27 20:50:57 +0000 451) for i := 0; i < 2; i++ { 8e69d43b32b src/net/net_test.go (Brad Fitzpatrick 2016-09-27 20:50:57 +0000 452) if err := <-errc; err != nil { 8e69d43b32b src/net/net_test.go (Brad Fitzpatrick 2016-09-27 20:50:57 +0000 453) t.Fatal(err) 8e69d43b32b src/net/net_test.go (Brad Fitzpatrick 2016-09-27 20:50:57 +0000 454) } 8e69d43b32b src/net/net_test.go (Brad Fitzpatrick 2016-09-27 20:50:57 +0000 455) }

Maybe there are no strict reviews for tests or maybe you can allow a little bit not standard code in tests. I don't know, but ...

Such cases of errc usage are only 20 in the whole codebase (and 14 of them are for tests).

➜ net git:(master) gogrep 'err := <-err' | wc -l 20 ➜ net git:(master) gogrep 'err := <-err' | grep _test.go | wc -l 14

It may happen for everyone to produce not "optimal" code. And about your code - the solution is perfectly fine and it works. But it's not the go way - what I mean by that - over the time go developers used to some patterns. WaitGroup is one of it. It just feels more natural for others if they see the pattern like this because everyone used to it and recognize it.

The other example is much simpler from today:

const ( A int = 1 B int = 2 )

The code works and it's nothing wrong with it, but it looks wired because everyone used to

const ( A int = 1 B = 2 )

There are some patterns that everyone understands and it's preferable to use them (for example Option for optional arguments :))

Last one, WaitGroup is not a solution for all such cases (as you saw with timeout chan)

thank you for the research! i still think that using err chans to wait end of the goroutines is not an anti-pattern but i'm ok with the two ways.

… fully implemented for mocks

ilgooz · 2018-09-26T12:22:50Z

/cc @ilgooz
TODO: change to WaitGroup

ilgooz · 2018-09-26T13:50:13Z

@antho1404

I like the part to have the status for the API but I don't think we should do a filter like that. We will need to have a filter for this and maybe for name after or the task or whatever so let's not rush on a feature that is not really necessary and will block us.

Also the part to remove the services when we stop the core is something that the cli should not do but the core itself should when it exits.

I think we should do the part for the status of the service but the part for the filter we should take some time to make this reusable kind of like docker manage it's filters and for the part of deleting the services when we stop, this in the end should not even be in the cli so we don't need this api to only have the running services and most of the things in this PR will no longer be needed like the config for example in the API struct

I also agree with this. I'm seeing this PR as intermediate one. Of course, it'd be better if we did first move the related logic from cli to daemon. Maybe, lets merge this as it is and within another PR, we can rm the filter from api and move the logic to daemon.

antho1404 · 2018-09-27T11:26:17Z

My point was that we don't actually even need the filter, let's not do it if we know that we will do it differently in another step, let's think the best way to do it and then we can implement it.

I would remove this part and only keep in the the status on the service api List and Get which is great but the rest anyway is not related to this PR and also I don't think we should have this kind of features right now especially if we know that this feature will be irrelevant when we move that in the core.

ilgooz · 2018-09-27T11:44:17Z

@antho1404 you're right

commands/commands_mock_test.go

mesg-bot · 2018-10-01T09:55:12Z

This pull request has been mentioned on MESG Community. There might be relevant details there:

https://forum.mesg.com/t/caputre-stdout-in-command-package/70/1

dev-core

mesg-bot · 2018-10-02T08:05:22Z

This pull request has been mentioned on MESG Community. There might be relevant details there:

https://forum.mesg.com/t/caputre-stdout-in-command-package/70/2

interface/grpc/core/list_services.go

commands/service_list_test.go

ilgooz · 2018-10-02T08:15:28Z

good to review

ilgooz · 2018-10-02T11:04:51Z

in progress

ilgooz · 2018-10-02T14:21:24Z

good to review

commands/service_list.go

commands/service_list_test.go

ilgooz · 2018-10-02T15:04:37Z

deleted @krhubert's comment by mistake, it was about removing extra lines while initializing api.API in tests. so, posting this as a reminder.

…updates

extend ListServices api to filter by running services, closes #496

70cbae5

* coreapi.Service type know has IsRunning field, relevant APIs accordingly updated to set this value. * update dev-core script. * update database API to filter services by id, add more tests.

ilgooz requested a review from a team September 25, 2018 08:46

api: check nil db on New()

4884e32

krhubert reviewed Sep 25, 2018

View reviewed changes

antho1404 requested changes Sep 25, 2018

View reviewed changes

ilgooz force-pushed the feature/service-list branch from 5da3a2f to ba843cb Compare September 26, 2018 05:22

ilgooz mentioned this pull request Sep 26, 2018

make grpc server newable, propagate created instances to childs inside core package #502

Closed

revert unrelated changes

fe429fa

ilgooz force-pushed the feature/service-list branch from ba843cb to fe429fa Compare September 26, 2018 05:50

service list API: explicitly use service.Status() to provide status info

fc2a760

ilgooz force-pushed the feature/service-list branch from 546cd0b to fc2a760 Compare September 26, 2018 08:58

antho1404 requested changes Sep 26, 2018

View reviewed changes

interface/grpc/core/list_services.go Show resolved Hide resolved

krhubert reviewed Sep 26, 2018

View reviewed changes

commands: show status on service list, make sure *Executor interfaces…

186b264

… fully implemented for mocks

ilgooz force-pushed the feature/service-list branch from 5f276bc to 186b264 Compare September 26, 2018 09:38

commands: improve tests

b7d151d

ilgooz force-pushed the feature/service-list branch from 6cebf8b to b7d151d Compare September 26, 2018 09:45

ilgooz added 2 commits September 26, 2018 12:56

interface/grpc/core: minor improvements

95fe504

interface/grpc/core: fix/improve TestListServices test

ff85d8a

interface/grpc/core & commands: wait goroutine to end with WaitGroup

f0bae70

ilgooz force-pushed the feature/service-list branch from 70dd27c to f0bae70 Compare September 26, 2018 13:32

undo changes about removing service.ListRunning()

6eb2dbf

mesg-foundation deleted a comment from mesg-bot Sep 28, 2018

antho1404 previously approved these changes Oct 1, 2018

View reviewed changes

commands/commands_mock_test.go Show resolved Hide resolved

ilgooz mentioned this pull request Oct 1, 2018

core_provider.Stop() should get running services from the Core #496

Closed

NicolasMahe requested changes Oct 2, 2018

View reviewed changes

dev-core Outdated Show resolved Hide resolved

krhubert reviewed Oct 2, 2018

View reviewed changes

interface/grpc/core/list_services.go Outdated Show resolved Hide resolved

dev-core: keep the execution chain

c075e5b

ilgooz dismissed antho1404’s stale review via c075e5b October 2, 2018 08:10

krhubert reviewed Oct 2, 2018

View reviewed changes

commands/service_list_test.go Outdated Show resolved Hide resolved

cosmetic updates

0e0f70e

ilgooz added 2 commits October 2, 2018 16:24

merge dev

bfa48fa

commands: drop outputStream and use captureStd() instead

3ae3919

krhubert reviewed Oct 2, 2018

View reviewed changes

commands/service_list.go Outdated Show resolved Hide resolved

krhubert reviewed Oct 2, 2018

View reviewed changes

commands/service_list_test.go Outdated Show resolved Hide resolved

mesg-foundation deleted a comment from krhubert Oct 2, 2018

commands: use gRPC's String() to convert consts to strings, cosmetic …

cf8b0c2

…updates

NicolasMahe approved these changes Oct 3, 2018

View reviewed changes

krhubert approved these changes Oct 3, 2018

View reviewed changes

Merge branch 'dev' into feature/service-list

0f1199f

antho1404 merged commit 0f695e6 into dev Oct 3, 2018

antho1404 deleted the feature/service-list branch October 3, 2018 11:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

extend ListServices api to filter by running services, closes #496 #499

extend ListServices api to filter by running services, closes #496 #499

ilgooz commented Sep 25, 2018 •

edited

Loading

ilgooz commented Sep 25, 2018 •

edited

Loading

krhubert Sep 25, 2018 •

edited

Loading

ilgooz Sep 26, 2018 •

edited

Loading

krhubert Sep 25, 2018 •

edited

Loading

ilgooz Sep 26, 2018

krhubert Sep 25, 2018

ilgooz Sep 26, 2018

antho1404 Sep 25, 2018

ilgooz Sep 26, 2018

ilgooz commented Sep 26, 2018

antho1404 left a comment

krhubert Sep 26, 2018

antho1404 Sep 26, 2018

ilgooz Sep 26, 2018

krhubert Sep 26, 2018

ilgooz Sep 26, 2018

NicolasMahe Sep 26, 2018 •

edited

Loading

ilgooz Sep 26, 2018

krhubert Sep 26, 2018 •

edited

Loading

ilgooz Sep 26, 2018 •

edited

Loading

ilgooz commented Sep 26, 2018

ilgooz commented Sep 26, 2018 •

edited

Loading

antho1404 commented Sep 27, 2018

ilgooz commented Sep 27, 2018

mesg-bot commented Oct 1, 2018

mesg-bot commented Oct 2, 2018

ilgooz commented Oct 2, 2018

ilgooz commented Oct 2, 2018

ilgooz commented Oct 2, 2018

ilgooz commented Oct 2, 2018

extend ListServices api to filter by running services, closes #496 #499

extend ListServices api to filter by running services, closes #496 #499

Conversation

ilgooz commented Sep 25, 2018 • edited Loading

ilgooz commented Sep 25, 2018 • edited Loading

krhubert Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

ilgooz Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

krhubert Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilgooz commented Sep 26, 2018

antho1404 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicolasMahe Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krhubert Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

ilgooz Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

ilgooz commented Sep 26, 2018

ilgooz commented Sep 26, 2018 • edited Loading

antho1404 commented Sep 27, 2018

ilgooz commented Sep 27, 2018

mesg-bot commented Oct 1, 2018

mesg-bot commented Oct 2, 2018

ilgooz commented Oct 2, 2018

ilgooz commented Oct 2, 2018

ilgooz commented Oct 2, 2018

ilgooz commented Oct 2, 2018

ilgooz commented Sep 25, 2018 •

edited

Loading

ilgooz commented Sep 25, 2018 •

edited

Loading

krhubert Sep 25, 2018 •

edited

Loading

ilgooz Sep 26, 2018 •

edited

Loading

krhubert Sep 25, 2018 •

edited

Loading

NicolasMahe Sep 26, 2018 •

edited

Loading

krhubert Sep 26, 2018 •

edited

Loading

ilgooz Sep 26, 2018 •

edited

Loading

ilgooz commented Sep 26, 2018 •

edited

Loading