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

WIP: Now using event timestamp to retrieve relevant logs. #209

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mattatcha
Copy link
Member

Using the event timestamp should prevent duplicate logs when a container restarts.

@@ -153,67 +153,25 @@ func (p *LogsPump) pumpLogs(event *docker.APIEvents, backlog bool) {
debug("pump.pumpLogs():", id, "ignored: environ ignore")
return
}
if !logDriverSupported(container) {
Copy link
Member

Choose a reason for hiding this comment

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

This drops the following functionality 324db6e.

Copy link
Member Author

Choose a reason for hiding this comment

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

Damn it. I didn't mean to remove that commit.

trying to hit /logs for a container that does not support that endpoint
results in the following error in that container's stdout

"Error running logs job: configured logging reader does not support
reading"
@ebr
Copy link
Contributor

ebr commented Aug 28, 2016

How is it looking for getting this merged (without breaking the work done in #191)?
I'm between a rock and a hard place: would like to run master in hopes for a fix for #206, but I need to run my fork based on #201 in order to avoid duplication.

@michaelshobbs
Copy link
Member

@MattAitchison do you still want to pursue this route?

@mattatcha
Copy link
Member Author

@michaelshobbs yeah I do. I believe its a better route since it should prevent missing log output as described here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants