Skip to content
This repository has been archived by the owner on Jan 30, 2020. It is now read-only.

Bugfix/1257 units requests #1260

Closed
wants to merge 1 commit into from

Conversation

mwitkow
Copy link
Contributor

@mwitkow mwitkow commented Jun 19, 2015

No description provided.

Recursive: true,
}
resp, err := r.kAPI.Get(r.ctx(), key, opts)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

you don't return err so this checking serves no purpose - unless we do want to return an error from this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -254,7 +266,7 @@ func dirToHeartbeat(dir *etcd.Node) (heartbeat string) {
// getUnitFromObject takes a *etcd.Node containing a Unit's jobModel, and
// instantiates and returns a representative *job.Unit, transitively fetching the
// associated UnitFile as necessary
Copy link
Contributor

Choose a reason for hiding this comment

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

Document what unitFunc does. The name unitHashLookupFunc might make more sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@mwitkow mwitkow force-pushed the bugfix/1257-units-requests branch from 0134d24 to efbecf8 Compare July 24, 2015 12:19
@mwitkow
Copy link
Contributor Author

mwitkow commented Jul 24, 2015

Addressed comments. Rebased on master. Rebased all commits into one.
PTAL

func (r *EtcdRegistry) getAllUnitsHashMap() (map[string]*unit.UnitFile, error) {
key := r.prefixed(unitPrefix)
opts := &etcd.GetOptions{
Recursive: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

please add Quorum: true, which was recently added to github.com/coreos/etcd/client.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do. But out of curiosity, what is the canonical etcd Go client?
https://github.com/coreos/go-etcd
or https://github.com/coreos/etcd/client?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

fwiw, etcd/client is now the canonical client, and go-etcd is deprecated

@mwitkow mwitkow force-pushed the bugfix/1257-units-requests branch from efbecf8 to 5bf8532 Compare July 28, 2015 08:15
@mwitkow
Copy link
Contributor Author

mwitkow commented Jul 28, 2015

Done, PTAL :)

@mwitkow mwitkow force-pushed the bugfix/1257-units-requests branch from 5bf8532 to 96f7d67 Compare August 5, 2015 08:48
@mwitkow
Copy link
Contributor Author

mwitkow commented Aug 5, 2015

@mischief Everthing is fixed, and rebase on latest master.

@mwitkow
Copy link
Contributor Author

mwitkow commented Sep 5, 2015

@mischief, can you take a look at this again? Together with the no_scheduler flag, this allow us to scale to ~1000s of machines, where before it would kill etcd ~300.

return r.unitFromEtcdNode(hash, resp.Node)
}

// getAllUnitsHashMap retrieves from the Registry all Units and returns a map of hash to Unit
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, s/Unit/UnitFile/g

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@jonboulle
Copy link
Contributor

@mwitkow-io A few small comments for you; lgtm after that, thanks!

@mwitkow
Copy link
Contributor Author

mwitkow commented Oct 1, 2015

Damn, this PR got drawned in a chunk of emails after vacation. Will update in a sec.

@mwitkow mwitkow force-pushed the bugfix/1257-units-requests branch from 96f7d67 to 091846e Compare October 1, 2015 07:19
@mwitkow
Copy link
Contributor Author

mwitkow commented Oct 1, 2015

@jonboulle, @mischief PTAL.

Again, apologies for the late response.

@jonboulle
Copy link
Contributor

Landing via #1376

@jonboulle jonboulle closed this Oct 12, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants