smancke / guble Goto Github PK

The PubSubRouter should become a private structure and be exported under an interface called Router.
This interface should satisfy the current MessageSink and PubSubSource interfaces that will be removed.

At the moment the testcoverage of the whole project is 75%. For the package of the gcm adapter this value is about 33.3%. This should be improved in this ticket. So that we at least have a test coverage about 90% in this package.
UAC:
improve test coverage for package GCM on value >= 90%
Do refactorings where needed

TODO's:

1. Remove the http.Handler mux and use the webserverinterface.
2. Parse the requestUri to get the /:userid/:gcmid/subscribe/*topic format and dirrectly pass to the Subscribe method. Remove the use of httprouter library and handle the subscription in one private method.

Decide which name is better:

• protocol
• gublep

This requires a bit refactoring, like:

• Remove the Publisher name from the field names
• PublishingTime should be just Time

Use of https://github.com/Sirupsen/logrus and add fields wherever possible.

Logging code must be removed from protocol package.

The router is the core of the app. We can have the router store all the other service thus allowing us to pass the router to all other instance and they will require the services as needed for each.

Put the AccessManager, MessageStore, KVStore in the router and allow access to them through the Router interface described in #24. For example the interface should have methods to return all this objects:

• AccessManager() (AccessManager, error),
• MessageStore() (MessageStore, error)
• KVStore() (KVStore, error)

Each method should return the object required or nil thus allowing the services dependent to take action in case a dependent service is not provided.

This must be implemented prior to #16.

Look for other tools that may replace the coverage collection. (hint: ginkgo).

Document the protocol format with the Message struct

Right now the code coverage for store package is 78% .

TODO
Raise the actual coverage

AccessManager should have a IsAllowed method that receives 3 parameters:

• Operation: (READ or WRITE)
• UserID
• Topic

READ operation is when a user wants to subscribe or fetch messages
WRITE operation is when a user wants to send a message.

Remove the coverage commands from .travis.yml and do only testing till we find a replacement for the tools in: #14

this is a task for improving the guble-cli code coverage

Make sure the messages stored are sent out and no more messages are accepted.

Decide which name is better:

• protocol
• gublep

This requires a bit refactoring, like:

• Remove the Publisher name from the field names
• PublishingTime should be just Time

Test localated here:
https://github.com/smancke/guble/blob/master/gubled/integration_test.go#L15

Fails randomly. Needs evaluation.

Atm, we have UserID and ApplicationID hardcoded in the route structure and this is not flexible. The ApplicationID is only used for the GCM connector and UserID doesn't have much use.

We should introduce a mechanism for controlling the versions of or external dependencies.

In the protocol do we need for message time to be supplied mandatory?
It is discarded on the server.

Integrate GoReportCard https://goreportcard.com/ to display the current status of the repo in the README file of the repository.

The channel that belongs to Route should send only message through it. No need to send the route also.

If you send through the route channel Router.C or receive you already have access to the route.
The channel should also be built inside the NewRoute constructor and not receive from exterior. Only if there aren't special cases where this is required.

https://consul.io

Move the AccessManager to it's own package under the server package: server/auth.

Service should remain a simple service handler, starting/stopping services and handling http request.
Remove the dependency injection. The only dependent service should be the router which will provide access to all other objects.

Implement a HealthCheck for MessageStore by polling the filesystem in order to see the available space on disk

Rename the Args structure to Config and move to own module config that initializes itself at load and contains the config in a public variable, thus all modules would have access to the config and they would pick the required values, without the need to pass them around.

RegisterModules should accept a variable number of modules as parameters and remove the registerModule private function as it's not required. append also accepts a variable number of values.

The Router would also handle the access management through the AccessManager thus:

• Subscribe method does a READ operation
• HandleMessage methods does a WRITE operation
• Return errors that are stored in a global variables for invalid authorization

Atm the WebServer is started inside a goroutine:
https://github.com/smancke/guble/blob/refactor/guble-24-public-router-interface/server/web_server.go#L25
It would be ideal if we can create an interface for modules to allow Service to know if something went wrong.

When I try to run guble as user on linux (fedora) (built using go get, not in a docker container), I get this message:

$ guble --log-info
2016/01/12 22:35:43 ERROR (github.com/smancke/guble/store.(*SqliteKVStore).Open:124): error db directory not writeable "/var/lib/guble/kv-store.db": "mkdir /var/lib/guble: permission denied"
2016/01/12 22:35:43 error db directory not writeable "/var/lib/guble/kv-store.db": "mkdir /var/lib/guble: permission denied"
! github.com/smancke/guble/store.(*SqliteKVStore).Open:124
! github.com/smancke/guble/gubled.glob.func1:35
! github.com/smancke/guble/gubled.StartupService:108
! github.com/smancke/guble/gubled.Main:97
! main.main:11
! runtime.main:120
! runtime.goexit:1722
2016/01/12 22:35:43 ERROR (github.com/smancke/guble/gubled.Main.func1:86): mkdir /var/lib/guble: permission denied
2016/01/12 22:35:43 mkdir /var/lib/guble: permission denied
! github.com/smancke/guble/gubled.Main.func1:86
! runtime.call32:437
! runtime.gopanic:423
! github.com/smancke/guble/gubled.glob.func1:36
! github.com/smancke/guble/gubled.StartupService:108
! github.com/smancke/guble/gubled.Main:97
! main.main:11
! runtime.main:120
! runtime.goexit:1722

The problem is clear from that message, but I would prefer to catch this case and output a nice message like

guble needs to be run as root because xyz

This would improve the first experience using guble.

Also, this should be mentioned in the readme, I think

I think we should still implement the queue'd route, not closing the channel immediately if the route is full.

At the moment we store only pointers in the channels which will make the buffer low in size and thus will save the disk, because closing the channel of the subscription, will require the subscription to fetch from the disk again, and if each subscription will fetch messages it will grown more in memory, cause each message in this case will represent different values in memory, there wont be a pointer to the same message. I think reducing the disk usage is a plus, and the queue'd route is easy to implement., already done some work in this direction.

EDIT

Outlining some benefits:

• The logic of delivering a message to the route channel goes inside the route
• The closing mechanism is inside the route also.
• We can decide using route params how the route will act, how much to buffer and if the channel should be closed in case of high buffer.
• The route will have a deliver method that will take care of storing the message in the queue and it will be non-blocking.

Any thoughts?

The main purpose here is: Make sure the messages stored are sent out and no more messages are accepted.

• After the Router has sent all the messages signal the Service to start to listen if all the modules finished.
• Service should listen for each module if the work is done, after calling Stop()
• Stop receiving messages.
• Wait to handle all messages that were received.
• Websocket connection must be stopped gracefully.

Suggestion here is that the Stop method should return a channel that will be closed when work is done. Or the channel could also send an error and we log the error and consider the work is done also.

Change time from ISO 8601 to unix timestamp.
Also this should be done in the guble-js client