Skip to content

Conversation

@helinwang
Copy link
Contributor

@helinwang helinwang commented Jun 21, 2017

Fixes: #2537 #2506

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add logs to inform recover event.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inMemStore have not much code, but it'll be still more clear if we put it to inmen_store.go corresponding to etcd_store.go, also the interface Store.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't know when to do cli.Close() to release etcd client resources, shall we catch signal INT and then call a DeleteEtcdStore? Or we can leave this to OS in this version.

I've also created an issue to talk about "how to gracefully shutdown master and pservers"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Very good point. Added TODO in the source code as well.

Copy link
Member

@jacquesqiao jacquesqiao Jun 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is inMemStore for test only?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, if endpoints is "", master will use inMemStore and have no fault tolerance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

Copy link
Contributor

@dzhwinter dzhwinter Jun 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a question, why here do not use code of

store, err := master.NewEtcdStore(eps, master.DefaultLockPath, master.DefaultStatePath, *ttlSec)

Copy link
Contributor Author

@helinwang helinwang Jun 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dzhwinter great question!

If we use store, err := ... here, it would be incorrect. Because then we actually created a new store variable, because we are in a new scope. The store would be a different variable than var store master.Store. What happened here is called store is "shadowed".

Please see here for me detail.
https://developmentality.wordpress.com/2014/03/03/go-gotcha-1-variable-shadowing-within-inner-scope-due-to-use-of-operator/

go vet -shadow can detect them:
https://golang.org/cmd/vet/#hdr-Shadowed_variables

:= is called short variable declarations

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so kind of you. get it, thanks!

@helinwang helinwang force-pushed the master_etcd_1 branch 2 times, most recently from bc2ef38 to eb68983 Compare June 23, 2017 19:56
@helinwang helinwang changed the title Master save and load state from etcd Master save and load state from etcd, and register on etcd. Jun 24, 2017
Copy link
Contributor

@typhoonzero typhoonzero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@helinwang helinwang merged commit 93019df into PaddlePaddle:develop Jun 24, 2017
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.

4 participants