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

Decouple session storage #380

Merged
merged 9 commits into from Sep 19, 2018

Conversation

Projects
None yet
5 participants
@Waldz
Copy link
Member

commented Sep 18, 2018

updates #382

@Waldz Waldz requested review from tadovas, soffokl, zolia, vkuznecovas and mysteriumnetwork/core Sep 18, 2018

@Waldz Waldz added the enhancement label Sep 18, 2018

@Waldz Waldz force-pushed the Waldz:feature/decouple-session-storage branch 3 times, most recently from aa1ec1e to 2476197 Sep 18, 2018

@@ -25,14 +25,14 @@ import (
)

type clientMap struct {
sessionManager session.Manager
sessions *session.Storage

This comment has been minimized.

Copy link
@vkuznecovas

vkuznecovas Sep 18, 2018

Contributor

Misleading naming. Sessions implies we'll have a collection of sessions, whereas here we have a storage.

IMHO sessions -> sessionStorage

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 18, 2018

Member

clientMap ? what happened to agreement not to use terms "client" "server". Looks like ServiceConsumerMap

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 18, 2018

Author Member

clientMap ? ..

This term client from from Openvpn management notifications.
And this map saves this clientID -> session metadata

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 18, 2018

Author Member

IMHO sessions -> sessionStorage

Strongly dont agree here, as we define interface on caller side.
Caller defines usage and his usecase by interface, so he can rename implementation on his side

@@ -27,46 +27,39 @@ import (
type ServiceConfigProvider func() (ServiceConfiguration, error)

// NewManager returns session manager which maintains a map of session id -> session
func NewManager(serviceConfigProvider ServiceConfigProvider, idGenerator Generator) *manager {
func NewManager(

This comment has been minimized.

Copy link
@vkuznecovas

vkuznecovas Sep 18, 2018

Contributor

if you have to have a multi line function declaration, maybe it's a time for a struct param instead?

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 18, 2018

Author Member

It's just 3 arguments. We make option map just >4 arguments, AFAIK

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 18, 2018

Author Member

Improved arguments to fit one line

@Waldz Waldz force-pushed the Waldz:feature/decouple-session-storage branch from 2476197 to 10c5009 Sep 18, 2018

@tadovas
Copy link
Member

left a comment

Clients, clients everywhere :(

@@ -25,14 +25,14 @@ import (
)

type clientMap struct {
sessionManager session.Manager
sessions *session.Storage

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 18, 2018

Member

clientMap ? what happened to agreement not to use terms "client" "server". Looks like ServiceConsumerMap

@@ -34,10 +34,10 @@ type Validator struct {
}

// NewValidator return Validator instance
func NewValidator(sessionManager session.Manager, extractor identity.Extractor) *Validator {
func NewValidator(sessionStorage *session.Storage, extractor identity.Extractor) *Validator {

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 18, 2018

Member

Depend on interfaces not on structures

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 18, 2018

Author Member

Refactored to 2 different interfaces on caller side

@@ -22,10 +22,21 @@ import (
"github.com/mysteriumnetwork/node/session"
)

const mockedVPNConfig = "config_string"
func mockStorage(sessions ...session.Session) *session.Storage {

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 18, 2018

Member

Doesn't look like mock storage to me, but a real prepopulated storage. Again - missing Storage interface ?

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 18, 2018

Author Member

Mocked storage

}

// Storage maintains a map of session id -> session
type Storage struct {

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 18, 2018

Member

This can be called InMemoryStorage, and caller can define Storage interface with expected operations

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 18, 2018

Author Member

Renamed to StorageMemory

@Waldz Waldz force-pushed the Waldz:feature/decouple-session-storage branch from 10c5009 to 59a203c Sep 18, 2018

@Waldz

This comment has been minimized.

Copy link
Member Author

commented Sep 18, 2018

All comments fixed

@Waldz Waldz force-pushed the Waldz:feature/decouple-session-storage branch 3 times, most recently from 1a39a15 to d6481ee Sep 18, 2018

}

// Add puts given session to storage. Multiple sessions per peerID is possible in case different services are used
func (storage *StorageMemory) Add(sessionInstance Session) error {

This comment has been minimized.

Copy link
@soffokl

soffokl Sep 19, 2018

Member

Looks like no error could be returned here, should we keep it in the function declaration?

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 19, 2018

Author Member

Fixed

@tadovas
Copy link
Member

left a comment

Small notice

@@ -26,47 +26,41 @@ import (
// ServiceConfigProvider defines configuration providing dependency
type ServiceConfigProvider func() (ServiceConfiguration, error)

// IDGenerator defines method for session id generation
type IDGenerator func() SessionID

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 19, 2018

Member

Can we assume, that future implementations will always succeed and never posibliy return error? I.e. true crypto random generator? Maybe it's worth to define error as part of result - it's more relaxed definition.

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 19, 2018

Author Member

I just remove error from SessionMap just because current implementation storageMemory dont return error (because of feadback).
So.. it look that dont do assumtions on future, w;ll implement when needed.

@@ -42,7 +42,6 @@ MESSAGES_RECONFIGURE=()

check_uncleaned_package "github.com/mysteriumnetwork/node/identity" 6
check_uncleaned_package "github.com/mysteriumnetwork/node/tequilapi" 11
check_uncleaned_package "github.com/mysteriumnetwork/node/session" 6

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 19, 2018

Member

Extra karma points for cleaning up packages

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 19, 2018

Author Member

So where is my approval?

@tadovas

This comment has been minimized.

Copy link
Member

commented Sep 19, 2018

Fine by me, as long as @vkuznecovas happy :)

@tadovas
Copy link
Member

left a comment

LGTM

@Waldz Waldz referenced this pull request Sep 19, 2018

Closed

Make Openvpn plugable transport #382

10 of 10 tasks complete

Waldz added some commits Sep 17, 2018

Waldz added some commits Sep 18, 2018

@Waldz Waldz dismissed stale reviews from zolia, tadovas, and vkuznecovas via f1c0fec Sep 19, 2018

@Waldz Waldz force-pushed the Waldz:feature/decouple-session-storage branch from eda5eac to f1c0fec Sep 19, 2018

@Waldz
Copy link
Member Author

left a comment

@mysteriumnetwork/core can You reapprove? 👍

@tadovas
Copy link
Member

left a comment

LGTM

@Waldz Waldz merged commit aaa6b75 into mysteriumnetwork:master Sep 19, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Waldz Waldz deleted the Waldz:feature/decouple-session-storage branch Sep 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.