Skip to content

Separate events subsystem#7370

Closed
LK4D4 wants to merge 1 commit intomoby:masterfrom
LK4D4:separate_events_#7342
Closed

Separate events subsystem#7370
LK4D4 wants to merge 1 commit intomoby:masterfrom
LK4D4:separate_events_#7342

Conversation

@LK4D4
Copy link
Copy Markdown
Contributor

@LK4D4 LK4D4 commented Aug 1, 2014

  • Events subsystem merged from server/events.go and
    utils/jsonmessagepublisher.go and moved to events/events.go
  • Only public interface for this subsystem is engine jobs
  • There is two new engine jobs - log_event and subscribers_count
  • There is auxiliary function container.LogEvent for logging events for
    containers

@LK4D4
Copy link
Copy Markdown
Contributor Author

LK4D4 commented Aug 1, 2014

ping @shykes @vieux @unclejack @crosbymichael @tiborvass @erikh
pretty big diff, but without really changing logic.

@tiborvass
Copy link
Copy Markdown
Contributor

tiborvass commented Aug 1, 2014

@LK4D4 Not sure if you can do this for this one; it might be for next time: when you're doing big changes like these, it's easier to review if it's broken down into multiple commits (IMHO).

@LK4D4
Copy link
Copy Markdown
Contributor Author

LK4D4 commented Aug 1, 2014

@tiborvass Yeah, sorry for this :) when I realized how big it is it was too late.
I added short description of changes in first comment and commit messages. Hope it'll help. Thank you.

erikh
erikh reviewed Aug 1, 2014
View reviewed changes
events/events.go Outdated
Copy link
Copy Markdown
Contributor

@erikh erikh Aug 1, 2014

Choose a reason for hiding this comment

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

maybe make this more dynamic?

Copy link
Copy Markdown
Contributor Author

@LK4D4 LK4D4 Aug 1, 2014

Choose a reason for hiding this comment

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

I'm not sure that I follow :) you mean something like table name -> method?

Copy link
Copy Markdown
Contributor

@erikh erikh Aug 1, 2014

Choose a reason for hiding this comment

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

Yeah.

@erikh
Copy link
Copy Markdown
Contributor

erikh commented Aug 1, 2014

LGTM, but I feel like we should either always be using the log_event job or not using it. I'm not sure I have a better solution for you that's simple to implement, however.

@LK4D4
Copy link
Copy Markdown
Contributor Author

LK4D4 commented Aug 1, 2014

Actually now event logging always does through log_event.

@erikh
Copy link
Copy Markdown
Contributor

erikh commented Aug 1, 2014

my mistake, carry on :)

@cyphar
Copy link
Copy Markdown
Contributor

cyphar commented Aug 2, 2014

@LK4D4 You can still break it up. Just rebase -i and then add -i several changes as separate commits.

@LK4D4
Copy link
Copy Markdown
Contributor Author

LK4D4 commented Aug 2, 2014

@cyphar Yeah, I know, but it will be pretty hard to make atomic commits, each not breaking tests. This is actually not very complicated change, but requires changes in many places at once.

@cyphar
Copy link
Copy Markdown
Contributor

cyphar commented Aug 2, 2014

Fair 'nuff.

@LK4D4
Copy link
Copy Markdown
Contributor Author

LK4D4 commented Aug 4, 2014

ping @shykes @vieux

@capoferro capoferro mentioned this pull request Aug 5, 2014
Closed
tiborvass
tiborvass reviewed Aug 5, 2014
View reviewed changes
daemon/container.go Outdated
Copy link
Copy Markdown
Contributor

@tiborvass tiborvass Aug 5, 2014

Choose a reason for hiding this comment

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

I'm wondering whether we want to leave the error handling to the client, or if we want to log to the daemon in case the log_event job failed. Since we never check for the error of LogEvent, I think we could internalize it here. WDYT?

@LK4D4
Copy link
Copy Markdown
Contributor Author

LK4D4 commented Aug 6, 2014

@tiborvass Thank you for review, I've added log message on LogEvent failure and removed returned error.

@LK4D4
Copy link
Copy Markdown
Contributor Author

LK4D4 commented Aug 6, 2014

@tiborvass PTAL

@tiborvass
Copy link
Copy Markdown
Contributor

tiborvass commented Aug 6, 2014

LGTM
Nice work @LK4D4!

Ping @unclejack @crosbymichael

@crosbymichael
Copy link
Copy Markdown
Contributor

crosbymichael commented Aug 6, 2014

I'm not getting any other events except "die". I'm not seeing start, stop, create, ....

@LK4D4
Copy link
Copy Markdown
Contributor Author

LK4D4 commented Aug 6, 2014

@crosbymichael sorry, functionality was lost in some rebase. Fixed, PTAL.

@LK4D4
Separate events subsystem
ceb988b 
* Events subsystem merged from `server/events.go` and
  `utils/jsonmessagepublisher.go` and moved to `events/events.go`
* Only public interface for this subsystem is engine jobs
* There is two new engine jobs - `log_event` and `subscribers_count`
* There is auxiliary function `container.LogEvent` for logging events for
  containers

Docker-DCO-1.1-Signed-off-by: Alexandr Morozov <lk4d4math@gmail.com> (github: LK4D4)
@crosbymichael
Copy link
Copy Markdown
Contributor

crosbymichael commented Aug 6, 2014

LGTM

@crosbymichael
Copy link
Copy Markdown
Contributor

crosbymichael commented Aug 6, 2014

@shykes @tiborvass do you want this merged before or after your stuff?

@tiborvass
Copy link
Copy Markdown
Contributor

tiborvass commented Aug 6, 2014

@crosbymichael If we can, I'd rather merge this after @shykes's stuff is merged.

@shykes
Copy link
Copy Markdown
Contributor

shykes commented Aug 6, 2014

I would prefer after. I will try rebasing this on top of mine. Should be easier than the other way around.

@shykes
Copy link
Copy Markdown
Contributor

shykes commented Aug 6, 2014

Also whoever carries this, please rename the job from log_event to log. Thx.

@crosbymichael
Copy link
Copy Markdown
Contributor

crosbymichael commented Aug 6, 2014

Doesn't log imply that we are writing a message to a log instead of sending an event down this event pipeline?

@shykes
Copy link
Copy Markdown
Contributor

shykes commented Aug 6, 2014

I don't see the distinction between message and event here. As far as I know we only plan on logging 1 type of object - call them event or message or whatever. So I don't see the need to qualify what we're logging.

There's already a log event in master.

@crosbymichael
Copy link
Copy Markdown
Contributor

crosbymichael commented Aug 6, 2014

ok

@shykes
Copy link
Copy Markdown
Contributor

shykes commented Aug 6, 2014

I'll take it. @tiborvass I'm tracking your #7427

@shykes shykes self-assigned this Aug 6, 2014
@shykes
Copy link
Copy Markdown
Contributor

shykes commented Aug 6, 2014

#assignee=shykes

@shykes
Copy link
Copy Markdown
Contributor

shykes commented Aug 6, 2014

I'm carrying it at https://github.com/shykes/docker/tree/7370-on-7427

@shykes shykes mentioned this pull request Aug 6, 2014
Merged
@tiborvass
Copy link
Copy Markdown
Contributor

tiborvass commented Aug 6, 2014

Closing in favor of #7452

@tiborvass tiborvass closed this Aug 6, 2014
@LK4D4 LK4D4 deleted the separate_events_#7342 branch August 7, 2014 02:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@shykes shykes

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@LK4D4 @tiborvass @erikh @cyphar @crosbymichael @shykes