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

Bugfix/1257 units requests #1260

Closed
mwitkow wants to merge 1 commit into from
Closed

Bugfix/1257 units requests #1260

mwitkow wants to merge 1 commit into from
+86 −7

Conversation

@mwitkow
Copy link
Contributor

@mwitkow mwitkow commented Jun 19, 2015

No description provided.

miekg
miekg reviewed Jun 19, 2015
View reviewed changes
registry/unit.go Outdated
Copy link
Contributor

@miekg miekg Jun 19, 2015

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

@mwitkow mwitkow Jul 24, 2015

Choose a reason for hiding this comment

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

Done.

@mwitkow mwitkow mentioned this pull request Jun 29, 2015
Closed
bcwaldon
bcwaldon reviewed Jul 9, 2015
View reviewed changes
registry/job.go
Copy link
Contributor

@bcwaldon bcwaldon Jul 9, 2015

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

@mwitkow mwitkow Jul 24, 2015

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

mischief
mischief reviewed Jul 27, 2015
View reviewed changes
registry/unit.go Outdated
Copy link
Contributor

@mischief mischief Jul 27, 2015

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

@mwitkow mwitkow Jul 27, 2015

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

@mwitkow mwitkow Jul 28, 2015

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@jonboulle jonboulle Sep 11, 2015

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.

jonboulle
jonboulle reviewed Sep 11, 2015
View reviewed changes
registry/unit.go Outdated
Copy link
Contributor

@jonboulle jonboulle Sep 11, 2015

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

@mwitkow mwitkow Oct 1, 2015

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

jonboulle commented Sep 11, 2015

@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 jonboulle mentioned this pull request Oct 1, 2015
Closed
@jonboulle jonboulle mentioned this pull request Oct 12, 2015
Merged
@jonboulle
Copy link
Contributor

jonboulle commented Oct 12, 2015

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.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mwitkow @jonboulle @miekg @bcwaldon @mischief