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

MYST-298 Rename nodeKey to providerId #137

Merged
merged 17 commits into from Feb 6, 2018

Conversation

Projects
None yet
4 participants
@Waldz
Copy link
Member

commented Feb 2, 2018

No description provided.

@Waldz Waldz requested review from ignasbernotas, tadovas, donce and zolia Feb 2, 2018

@Waldz Waldz force-pushed the feature/MYST-298-rename-node-key branch 3 times, most recently from 49cd13d to e00c0f1 Feb 2, 2018

@tadovas tadovas force-pushed the feature/MYST-298-rename-node-key branch from 61f6f0b to 49cd13d Feb 5, 2018

@@ -123,7 +123,7 @@ func (management *Management) serveNewConnection(connection net.Conn) {
func (management *Management) deliverLines() {
for {
line := <-management.lineReceiver
log.Debug(management.logPrefix, "Line delivering: ", line)
// log.Debug(management.logPrefix, "Line delivering: ", line)

This comment has been minimized.

Copy link
@donce

donce Feb 5, 2018

Contributor

Dead code - we should remove it or use log.Trace for lower logging level.
Same applies for change at line 138.

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 5, 2018

Author Member

Fixed

@@ -95,8 +96,6 @@ func (mApi *mysteriumAPI) FindProposals(nodeKey string) ([]dto_discovery.Service
return nil, err
}

log.Info(mysteriumAPILogPrefix, "FindProposals fetched: ", proposalsResponse.Proposals)

This comment has been minimized.

Copy link
@donce

donce Feb 5, 2018

Contributor

All endpoints adds some logs after it's successful - removing this makes it inconsistent.
We should keep this log or remove all success logs in this class.

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 5, 2018

Author Member

Fixed


return nil
}

func (client *ClientFake) NodeSendStats(nodeKey string, signer identity.Signer) (err error) {
log.Info(mysteriumAPILogPrefix, "Node stats sent: ", nodeKey)
// SendProposalStats heartbeats that service proposal is still active

This comment has been minimized.

Copy link
@donce

donce Feb 5, 2018

Contributor

This comment is false at the moment - we are passing just providerID here and no calculations is done in discovery on whether proposal is active or not. All proposals are active at the memotn. What about this?

// SendProposalStats heartbeats that provider is still active

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 5, 2018

Author Member

We dont have unique ID for proposals, yet. But intentions is to ping proposal

@@ -55,6 +61,7 @@ func (client *ClientFake) FindProposals(nodeKey string) (proposals []dto_discove
return proposals, nil
}

// SendSessionStats heartbeats that session is still active

This comment has been minimized.

Copy link
@donce

donce Feb 5, 2018

Contributor

This should also include that stats are being sent here about upload and download amounts.

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 5, 2018

Author Member

Improved

@tadovas
Copy link
Member

left a comment

LGTM after @donce comments will be addressed

@Waldz Waldz force-pushed the feature/MYST-298-rename-node-key branch from 49cd13d to 61d65ab Feb 5, 2018

@Waldz

This comment has been minimized.

Copy link
Member Author

commented Feb 5, 2018

Comments fixed

@tadovas

This comment has been minimized.

Copy link
Member

commented Feb 5, 2018

@donce its on you. LGTM

@tadovas tadovas dismissed their stale review via 49cd13d Feb 5, 2018

@tadovas tadovas force-pushed the feature/MYST-298-rename-node-key branch from 61d65ab to 49cd13d Feb 5, 2018

@donce donce dismissed their stale review Feb 5, 2018

ok

@shroomist
Copy link
Contributor

left a comment

looks like comment fixes weren't pushed, were they?

@@ -135,7 +135,7 @@ func (management *Management) deliverLines() {
lineConsumed = lineConsumed || consumed
}
if !lineConsumed {
log.Warn(management.logPrefix, "Line not delivered: ", line)
// log.Warn(management.logPrefix, "Line not delivered: ", line)

This comment has been minimized.

Copy link
@shroomist

shroomist Feb 5, 2018

Contributor

why not logging? should this place have a TODO comment?

@donce
Copy link
Contributor

left a comment

See @shroomist note

@Waldz Waldz force-pushed the feature/MYST-298-rename-node-key branch from 49cd13d to 61d65ab Feb 6, 2018

@Waldz Waldz force-pushed the feature/MYST-298-rename-node-key branch from 61d65ab to e69322b Feb 6, 2018

@@ -130,14 +130,14 @@ func (c *Command) handleActions(line string) {

func (c *Command) connect(argsString string) {
if len(argsString) == 0 {
info("Press tab to select identity or create a new one. Connect <your-identity> <node-identity>")
info("Press tab to select identity or create a new one. Connect <your-id> <provider-id>")

This comment has been minimized.

Copy link
@donce

donce Feb 6, 2018

Contributor

Is <your-id> really easier to understand than <your-identity> for CLI user? For me, identity seems like a concept we use publicly, and id is just an internal shortcut for it.

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 6, 2018

Author Member

Fixed

@@ -64,26 +64,27 @@ func (mApi *mysteriumAPI) RegisterProposal(proposal dto_discovery.ServiceProposa
return err
}

func (mApi *mysteriumAPI) NodeSendStats(nodeKey string, signer identity.Signer) error {
func (mApi *mysteriumAPI) PingProposal(proposal dto_discovery.ServiceProposal, signer identity.Signer) error {

This comment has been minimized.

Copy link
@donce

donce Feb 6, 2018

Contributor

New method name is false - it states that it acts on "proposal", but implementation acts on "provider".
Naming it UpdateProviderStatus seems ok for me.

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 6, 2018

Author Member

Tried: RegisterProvider(proposal) and UpdateProviderStatus(proposal) and FindProviders(providerId), looks weird and reverted.

I want to make driving change here:

            -> providerID -> publish Proposal(providerID, serviceType, ID)
 identity
            -> consumerID -> consume Proposal(providerID, serviceType, ID)

This comment has been minimized.

Copy link
@donce

donce Feb 6, 2018

Contributor

I want to make driving change here

Method name should indicate what method does, not what you think it should do in the future.

Tried: RegisterProvider(proposal) and UpdateProviderStatus(proposal) and FindProviders(providerId), looks weird and reverted.

Why FindProviders(providerId) ?? It finds proposals, not providers, so of course it looks weird :D

@donce donce dismissed their stale review Feb 6, 2018

ok

@tadovas

tadovas approved these changes Feb 6, 2018

Copy link
Member

left a comment

LGTM

@Waldz Waldz merged commit 081f6b4 into master Feb 6, 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.